Hello Eduard Zingerman,
The patch 1ade23711971: "bpf: Inline calls to bpf_loop when callback
is known" from Jun 21, 2022, leads to the following Smatch static
checker warning:
kernel/bpf/verifier.c:14420 inline_bpf_loop()
error: dereferencing freed memory 'env->prog'
kernel/bpf/verifier.c
14350 static struct bpf_prog *inline_bpf_loop(struct bpf_verifier_env *env,
14351 int position,
14352 s32 stack_base,
14353 u32 callback_subprogno,
14354 u32 *cnt)
14355 {
14356 s32 r6_offset = stack_base + 0 * BPF_REG_SIZE;
14357 s32 r7_offset = stack_base + 1 * BPF_REG_SIZE;
14358 s32 r8_offset = stack_base + 2 * BPF_REG_SIZE;
14359 int reg_loop_max = BPF_REG_6;
14360 int reg_loop_cnt = BPF_REG_7;
14361 int reg_loop_ctx = BPF_REG_8;
14362
14363 struct bpf_prog *new_prog;
14364 u32 callback_start;
14365 u32 call_insn_offset;
14366 s32 callback_offset;
14367
14368 /* This represents an inlined version of bpf_iter.c:bpf_loop,
14369 * be careful to modify this code in sync.
14370 */
14371 struct bpf_insn insn_buf[] = {
14372 /* Return error and jump to the end of the patch if
14373 * expected number of iterations is too big.
14374 */
14375 BPF_JMP_IMM(BPF_JLE, BPF_REG_1, BPF_MAX_LOOPS, 2),
14376 BPF_MOV32_IMM(BPF_REG_0, -E2BIG),
14377 BPF_JMP_IMM(BPF_JA, 0, 0, 16),
14378 /* spill R6, R7, R8 to use these as loop vars */
14379 BPF_STX_MEM(BPF_DW, BPF_REG_10, BPF_REG_6, r6_offset),
14380 BPF_STX_MEM(BPF_DW, BPF_REG_10, BPF_REG_7, r7_offset),
14381 BPF_STX_MEM(BPF_DW, BPF_REG_10, BPF_REG_8, r8_offset),
14382 /* initialize loop vars */
14383 BPF_MOV64_REG(reg_loop_max, BPF_REG_1),
14384 BPF_MOV32_IMM(reg_loop_cnt, 0),
14385 BPF_MOV64_REG(reg_loop_ctx, BPF_REG_3),
14386 /* loop header,
14387 * if reg_loop_cnt >= reg_loop_max skip the loop body
14388 */
14389 BPF_JMP_REG(BPF_JGE, reg_loop_cnt, reg_loop_max, 5),
14390 /* callback call,
14391 * correct callback offset would be set after patching
14392 */
14393 BPF_MOV64_REG(BPF_REG_1, reg_loop_cnt),
14394 BPF_MOV64_REG(BPF_REG_2, reg_loop_ctx),
14395 BPF_CALL_REL(0),
14396 /* increment loop counter */
14397 BPF_ALU64_IMM(BPF_ADD, reg_loop_cnt, 1),
14398 /* jump to loop header if callback returned 0 */
14399 BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, -6),
14400 /* return value of bpf_loop,
14401 * set R0 to the number of iterations
14402 */
14403 BPF_MOV64_REG(BPF_REG_0, reg_loop_cnt),
14404 /* restore original values of R6, R7, R8 */
14405 BPF_LDX_MEM(BPF_DW, BPF_REG_6, BPF_REG_10, r6_offset),
14406 BPF_LDX_MEM(BPF_DW, BPF_REG_7, BPF_REG_10, r7_offset),
14407 BPF_LDX_MEM(BPF_DW, BPF_REG_8, BPF_REG_10, r8_offset),
14408 };
14409
14410 *cnt = ARRAY_SIZE(insn_buf);
14411 new_prog = bpf_patch_insn_data(env, position, insn_buf, *cnt);
The bpf_patch_insn_data() function sometimes frees the old "env->prog"
and returns "new_prog".
14412 if (!new_prog)
14413 return new_prog;
14414
14415 /* callback start is known only after patching */
14416 callback_start = env->subprog_info[callback_subprogno].start;
14417 /* Note: insn_buf[12] is an offset of BPF_CALL_REL instruction */
14418 call_insn_offset = position + 12;
14419 callback_offset = callback_start - call_insn_offset - 1;
--> 14420 env->prog->insnsi[call_insn_offset].imm = callback_offset;
Presumably somewhere there is a "env->prog = new_prog;" but I couldn't
spot it in bpf_patch_insn_data(). But it feels like it would be more
readable to say:
new_prog->insnsi[call_insn_offset].imm = callback_offset;
14421
14422 return new_prog;
14423 }
regards,
dan carpenter
