bpf: add bpf_get_stack helper

The special property of return values for helpers bpf_get_stack and bpf_probe_read_str are captured in verifier.
Both helpers return a negative error code or
a length, which is equal to or smaller than the buffer size argument. This additional information in the verifier can avoid the condition such as "retval > bufsize" in the bpf program. For example, for the code blow,

    usize = bpf_get_stack(ctx, raw_data, max_len, BPF_F_USER_STACK);
    if (usize < 0 || usize > max_len)
        return 0;
The verifier may have the following errors:
    52: (85) call bpf_get_stack#65
     R0=map_value(id=0,off=0,ks=4,vs=1600,imm=0) R1_w=ctx(id=0,off=0,imm=0)
     R2_w=map_value(id=0,off=0,ks=4,vs=1600,imm=0) R3_w=inv800 R4_w=inv256
     R6=ctx(id=0,off=0,imm=0) R7=map_value(id=0,off=0,ks=4,vs=1600,imm=0)
     R9_w=inv800 R10=fp0,call_-1
    53: (bf) r8 = r0
    54: (bf) r1 = r8
    55: (67) r1 <<= 32
    56: (bf) r2 = r1
    57: (77) r2 >>= 32
    58: (25) if r2 > 0x31f goto pc+33
     R0=inv(id=0) R1=inv(id=0,smax_value=9223372032559808512,
                         umax_value=18446744069414584320,
                         var_off=(0x0; 0xffffffff00000000))
     R2=inv(id=0,umax_value=799,var_off=(0x0; 0x3ff))
     R6=ctx(id=0,off=0,imm=0) R7=map_value(id=0,off=0,ks=4,vs=1600,imm=0)
     R8=inv(id=0) R9=inv800 R10=fp0,call_-1
    59: (1f) r9 -= r8
    60: (c7) r1 s>>= 32
    61: (bf) r2 = r7
    62: (0f) r2 += r1
    math between map_value pointer and register with unbounded
    min value is not allowed

The failure is due to llvm compiler optimization where register "r2", which is a copy of "r1", is tested for condition while later on "r1" is used for map_ptr operation. The verifier is not able to track such inst sequence effectively.

Without the "usize > max_len" condition, there is no llvm optimization and the below generated code passed verifier:

    52: (85) call bpf_get_stack#65
     R0=map_value(id=0,off=0,ks=4,vs=1600,imm=0) R1_w=ctx(id=0,off=0,imm=0)
     R2_w=map_value(id=0,off=0,ks=4,vs=1600,imm=0) R3_w=inv800 R4_w=inv256
     R6=ctx(id=0,off=0,imm=0) R7=map_value(id=0,off=0,ks=4,vs=1600,imm=0)
     R9_w=inv800 R10=fp0,call_-1
    53: (b7) r1 = 0
    54: (bf) r8 = r0
    55: (67) r8 <<= 32
    56: (c7) r8 s>>= 32
    57: (6d) if r1 s> r8 goto pc+24
     R0=inv(id=0,umax_value=800) R1=inv0 R6=ctx(id=0,off=0,imm=0)
     R7=map_value(id=0,off=0,ks=4,vs=1600,imm=0)
     R8=inv(id=0,umax_value=800,var_off=(0x0; 0x3ff)) R9=inv800
     R10=fp0,call_-1
    58: (bf) r2 = r7
    59: (0f) r2 += r8
    60: (1f) r9 -= r8
    61: (bf) r1 = r6

Signed-off-by Yonghong Song <[email protected]>

---
 kernel/bpf/verifier.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

kernel/bpf/verifier.c +2 -0

index aba9425..3c8bb92 100644
@@ -164,6 +164,8 @@ struct bpf_call_arg_meta {
 	bool pkt_access;
 	int regno;
 	int access_size;
+	s64 msize_smax_value;
+	u64 msize_umax_value;
 };
 
 static DEFINE_MUTEX(bpf_verifier_lock);

kernel/bpf/verifier.c +23 -0

+			/* remember the mem_size which may be used later
+			 * to refine return values.
+			 */
+			meta->msize_smax_value = reg->smax_value;
+			meta->msize_umax_value = reg->umax_value;
+		}
 	}
 
 	return err;

@@ -2333,6 +2343,21 @@ static int prepare_func_exit(struct bpf_verifier_env *env, int *insn_idx)
 	return 0;
 }
 
+static void do_refine_retval_range(struct bpf_reg_state *regs, int ret_type,
+				   int func_id,
+				   struct bpf_call_arg_meta *meta)
+{
+	struct bpf_reg_state *ret_reg = &regs[BPF_REG_0];
+
+	if (ret_type != RET_INTEGER ||
+	    (func_id != BPF_FUNC_get_stack &&
+	     func_id != BPF_FUNC_probe_read_str))
+		return;
+
+	ret_reg->smax_value = meta->msize_smax_value;
+	ret_reg->umax_value = meta->msize_umax_value;
+}
+
 static int check_helper_call(struct bpf_verifier_env *env, int func_id, int insn_idx)
 {
 	const struct bpf_func_proto *fn = NULL;

@@ -2456,6 +2481,8 @@ static int check_helper_call(struct bpf_verifier_env *env, int func_id, int insn
 		return -EINVAL;
 	}
 
+	do_refine_retval_range(regs, fn->ret_type, func_id, &meta);
+
 	err = check_map_func_compatibility(env, meta.map_ptr, func_id);
 	if (err)
 		return err;

Signature · 1 line

2.9.5

Quoted 79 lines · Yonghong Song

The special property of return values for helpers bpf_get_stack and bpf_probe_read_str are captured in verifier.
Both helpers return a negative error code or
a length, which is equal to or smaller than the buffer size argument. This additional information in the verifier can avoid the condition such as "retval > bufsize" in the bpf program. For example, for the code blow,

     usize = bpf_get_stack(ctx, raw_data, max_len, BPF_F_USER_STACK);
     if (usize < 0 || usize > max_len)
         return 0;
The verifier may have the following errors:
     52: (85) call bpf_get_stack#65
      R0=map_value(id=0,off=0,ks=4,vs=1600,imm=0) R1_w=ctx(id=0,off=0,imm=0)
      R2_w=map_value(id=0,off=0,ks=4,vs=1600,imm=0) R3_w=inv800 R4_w=inv256
      R6=ctx(id=0,off=0,imm=0) R7=map_value(id=0,off=0,ks=4,vs=1600,imm=0)
      R9_w=inv800 R10=fp0,call_-1
     53: (bf) r8 = r0
     54: (bf) r1 = r8
     55: (67) r1 <<= 32
     56: (bf) r2 = r1
     57: (77) r2 >>= 32
     58: (25) if r2 > 0x31f goto pc+33
      R0=inv(id=0) R1=inv(id=0,smax_value=9223372032559808512,
                          umax_value=18446744069414584320,
                          var_off=(0x0; 0xffffffff00000000))
      R2=inv(id=0,umax_value=799,var_off=(0x0; 0x3ff))
      R6=ctx(id=0,off=0,imm=0) R7=map_value(id=0,off=0,ks=4,vs=1600,imm=0)
      R8=inv(id=0) R9=inv800 R10=fp0,call_-1
     59: (1f) r9 -= r8
     60: (c7) r1 s>>= 32
     61: (bf) r2 = r7
     62: (0f) r2 += r1
     math between map_value pointer and register with unbounded
     min value is not allowed

The failure is due to llvm compiler optimization where register "r2", which is a copy of "r1", is tested for condition while later on "r1" is used for map_ptr operation. The verifier is not able to track such inst sequence effectively.

Without the "usize > max_len" condition, there is no llvm optimization and the below generated code passed verifier:

     52: (85) call bpf_get_stack#65
      R0=map_value(id=0,off=0,ks=4,vs=1600,imm=0) R1_w=ctx(id=0,off=0,imm=0)
      R2_w=map_value(id=0,off=0,ks=4,vs=1600,imm=0) R3_w=inv800 R4_w=inv256
      R6=ctx(id=0,off=0,imm=0) R7=map_value(id=0,off=0,ks=4,vs=1600,imm=0)
      R9_w=inv800 R10=fp0,call_-1
     53: (b7) r1 = 0
     54: (bf) r8 = r0
     55: (67) r8 <<= 32
     56: (c7) r8 s>>= 32
     57: (6d) if r1 s> r8 goto pc+24
      R0=inv(id=0,umax_value=800) R1=inv0 R6=ctx(id=0,off=0,imm=0)
      R7=map_value(id=0,off=0,ks=4,vs=1600,imm=0)
      R8=inv(id=0,umax_value=800,var_off=(0x0; 0x3ff)) R9=inv800
      R10=fp0,call_-1
     58: (bf) r2 = r7
     59: (0f) r2 += r8
     60: (1f) r9 -= r8
     61: (bf) r1 = r6

Signed-off-by Yonghong Song <[email protected]>

---
  kernel/bpf/verifier.c | 27 +++++++++++++++++++++++++++
  1 file changed, 27 insertions(+)

kernel/bpf/verifier.c +2 -0

index aba9425..3c8bb92 100644
@@ -164,6 +164,8 @@ struct bpf_call_arg_meta {
  	bool pkt_access;
  	int regno;
  	int access_size;
+	s64 msize_smax_value;
+	u64 msize_umax_value;
  };
 
  static DEFINE_MUTEX(bpf_verifier_lock);

kernel/bpf/verifier.c static int check_func_arg(struct bpf_verifier_env *env, u32 regno, · line 2033

Quoted 5 lines

kernel/bpf/verifier.c +2 -0

@@ -2027,6 +2029,14 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 regno,
  		err = check_helper_mem_access(env, regno - 1,
  					      reg->umax_value,
  					      zero_size_allowed, meta);
+
+		if (!err && !!meta) {

Please drop !! in the above.

Also what happens when
if (!tnum_is_const(reg->var_off))
meta = NULL;
?
it seems two new fields of meta will stay zero initialized that later do_refine_retval_range() will set R0->umax_value = 0 which seems incorrect.

Quoted 44 lines

kernel/bpf/verifier.c +23 -0

+			/* remember the mem_size which may be used later
+			 * to refine return values.
+			 */
+			meta->msize_smax_value = reg->smax_value;
+			meta->msize_umax_value = reg->umax_value;
+		}
  	}
 
  	return err;

@@ -2333,6 +2343,21 @@ static int prepare_func_exit(struct bpf_verifier_env *env, int *insn_idx)
  	return 0;
  }
 
+static void do_refine_retval_range(struct bpf_reg_state *regs, int ret_type,
+				   int func_id,
+				   struct bpf_call_arg_meta *meta)
+{
+	struct bpf_reg_state *ret_reg = &regs[BPF_REG_0];
+
+	if (ret_type != RET_INTEGER ||
+	    (func_id != BPF_FUNC_get_stack &&
+	     func_id != BPF_FUNC_probe_read_str))
+		return;
+
+	ret_reg->smax_value = meta->msize_smax_value;
+	ret_reg->umax_value = meta->msize_umax_value;
+}
+
  static int check_helper_call(struct bpf_verifier_env *env, int func_id, int insn_idx)
  {
  	const struct bpf_func_proto *fn = NULL;

@@ -2456,6 +2481,8 @@ static int check_helper_call(struct bpf_verifier_env *env, int func_id, int insn
  		return -EINVAL;
  	}
 
+	do_refine_retval_range(regs, fn->ret_type, func_id, &meta);
+
  	err = check_map_func_compatibility(env, meta.map_ptr, func_id);
  	if (err)
  		return err;

Signature · 1 line

2.9.5

When helpers like bpf_get_stack returns an int value and later on used for arithmetic computation, the LSH and ARSH operations are often required to get proper sign extension into 64-bit. For example, without this patch:

    54: R0=inv(id=0,umax_value=800)
    54: (bf) r8 = r0
    55: R0=inv(id=0,umax_value=800) R8_w=inv(id=0,umax_value=800)
    55: (67) r8 <<= 32
    56: R8_w=inv(id=0,umax_value=3435973836800,var_off=(0x0; 0x3ff00000000))
    56: (c7) r8 s>>= 32
    57: R8=inv(id=0)
With this patch:
    54: R0=inv(id=0,umax_value=800)
    54: (bf) r8 = r0
    55: R0=inv(id=0,umax_value=800) R8_w=inv(id=0,umax_value=800)
    55: (67) r8 <<= 32
    56: R8_w=inv(id=0,umax_value=3435973836800,var_off=(0x0; 0x3ff00000000))
    56: (c7) r8 s>>= 32
    57: R8=inv(id=0, umax_value=800,var_off=(0x0; 0x3ff))

With better range of "R8", later on when "R8" is added to other register, e.g., a map pointer or scalar-value register, the better register range can be derived and verifier failure may be avoided.

In our later example,

    ......
    usize = bpf_get_stack(ctx, raw_data, max_len, BPF_F_USER_STACK);
    if (usize < 0)
        return 0;
    ksize = bpf_get_stack(ctx, raw_data + usize, max_len - usize, 0);
    ......

Without improving ARSH value range tracking, the register representing "max_len - usize" will have smin_value equal to S64_MIN and will be rejected by verifier.

Signed-off-by Yonghong Song <[email protected]>

---
 kernel/bpf/verifier.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

kernel/bpf/verifier.c +15 -0

index 3c8bb92..01c215d 100644
@@ -2975,6 +2975,32 @@ static int adjust_scalar_min_max_vals(struct bpf_verifier_env *env,
 		/* We may learn something more from the var_off */
 		__update_reg_bounds(dst_reg);
 		break;
+	case BPF_ARSH:
+		if (umax_val >= insn_bitness) {
+			/* Shifts greater than 31 or 63 are undefined.
+			 * This includes shifts by a negative number.
+			 */
+			mark_reg_unknown(env, regs, insn->dst_reg);
+			break;
+		}
+		if (dst_reg->smin_value < 0)
+			dst_reg->smin_value >>= umin_val;
+		else
+			dst_reg->smin_value >>= umax_val;
+		if (dst_reg->smax_value < 0)
+			dst_reg->smax_value >>= umax_val;
+		else

Quoted 67 lines · Yonghong Song

When helpers like bpf_get_stack returns an int value and later on used for arithmetic computation, the LSH and ARSH operations are often required to get proper sign extension into 64-bit. For example, without this patch:

     54: R0=inv(id=0,umax_value=800)
     54: (bf) r8 = r0
     55: R0=inv(id=0,umax_value=800) R8_w=inv(id=0,umax_value=800)
     55: (67) r8 <<= 32
     56: R8_w=inv(id=0,umax_value=3435973836800,var_off=(0x0; 0x3ff00000000))
     56: (c7) r8 s>>= 32
     57: R8=inv(id=0)
With this patch:
     54: R0=inv(id=0,umax_value=800)
     54: (bf) r8 = r0
     55: R0=inv(id=0,umax_value=800) R8_w=inv(id=0,umax_value=800)
     55: (67) r8 <<= 32
     56: R8_w=inv(id=0,umax_value=3435973836800,var_off=(0x0; 0x3ff00000000))
     56: (c7) r8 s>>= 32
     57: R8=inv(id=0, umax_value=800,var_off=(0x0; 0x3ff))

With better range of "R8", later on when "R8" is added to other register, e.g., a map pointer or scalar-value register, the better register range can be derived and verifier failure may be avoided.

In our later example,

     ......
     usize = bpf_get_stack(ctx, raw_data, max_len, BPF_F_USER_STACK);
     if (usize < 0)
         return 0;
     ksize = bpf_get_stack(ctx, raw_data + usize, max_len - usize, 0);
     ......

Without improving ARSH value range tracking, the register representing "max_len - usize" will have smin_value equal to S64_MIN and will be rejected by verifier.

Signed-off-by Yonghong Song <[email protected]>

---
  kernel/bpf/verifier.c | 26 ++++++++++++++++++++++++++
  1 file changed, 26 insertions(+)

kernel/bpf/verifier.c +20 -0

index 3c8bb92..01c215d 100644
@@ -2975,6 +2975,32 @@ static int adjust_scalar_min_max_vals(struct bpf_verifier_env *env,
  		/* We may learn something more from the var_off */
  		__update_reg_bounds(dst_reg);
  		break;
+	case BPF_ARSH:
+		if (umax_val >= insn_bitness) {
+			/* Shifts greater than 31 or 63 are undefined.
+			 * This includes shifts by a negative number.
+			 */
+			mark_reg_unknown(env, regs, insn->dst_reg);
+			break;
+		}
+		if (dst_reg->smin_value < 0)
+			dst_reg->smin_value >>= umin_val;
+		else
+			dst_reg->smin_value >>= umax_val;
+		if (dst_reg->smax_value < 0)
+			dst_reg->smax_value >>= umax_val;
+		else
+			dst_reg->smax_value >>= umin_val;
+		if (src_known)
+			dst_reg->var_off = tnum_rshift(dst_reg->var_off,
+						       umin_val);
+		else

kernel/bpf/verifier.c static int adjust_scalar_min_max_vals(struct bpf_verifier_env *env, · line 3002

Quoted 5 lines

kernel/bpf/verifier.c +5 -0

+			dst_reg->var_off = tnum_rshift(tnum_unknown, umin_val);
+		dst_reg->umin_value >>= umax_val;
+		dst_reg->umax_value >>= umin_val;
+		/* We may learn something more from the var_off */
+		__update_reg_bounds(dst_reg);

I'm struggling to understand how these bounds are computed. Could you add examples in the comments?

In particular if dst_reg is unknown (tnum.mask == -1) the above tnum_rshift() will clear upper bits and will make it 64-bit positive, but that doesn't seem correct.
What am I missing?

Quoted 68 lines · Alexei Starovoitov

Quoted 67 lines · Yonghong Song

When helpers like bpf_get_stack returns an int value and later on used for arithmetic computation, the LSH and ARSH operations are often required to get proper sign extension into 64-bit. For example, without this patch:

     54: R0=inv(id=0,umax_value=800)
     54: (bf) r8 = r0
     55: R0=inv(id=0,umax_value=800) R8_w=inv(id=0,umax_value=800)
     55: (67) r8 <<= 32
     56: R8_w=inv(id=0,umax_value=3435973836800,var_off=(0x0; 0x3ff00000000))
     56: (c7) r8 s>>= 32
     57: R8=inv(id=0)
With this patch:
     54: R0=inv(id=0,umax_value=800)
     54: (bf) r8 = r0
     55: R0=inv(id=0,umax_value=800) R8_w=inv(id=0,umax_value=800)
     55: (67) r8 <<= 32
     56: R8_w=inv(id=0,umax_value=3435973836800,var_off=(0x0; 0x3ff00000000))
     56: (c7) r8 s>>= 32
     57: R8=inv(id=0, umax_value=800,var_off=(0x0; 0x3ff))

With better range of "R8", later on when "R8" is added to other register, e.g., a map pointer or scalar-value register, the better register range can be derived and verifier failure may be avoided.

In our later example,

     ......
     usize = bpf_get_stack(ctx, raw_data, max_len, BPF_F_USER_STACK);
     if (usize < 0)
         return 0;
     ksize = bpf_get_stack(ctx, raw_data + usize, max_len - usize, 0);
     ......

Without improving ARSH value range tracking, the register representing "max_len - usize" will have smin_value equal to S64_MIN and will be rejected by verifier.

Signed-off-by Yonghong Song <[email protected]>

---
  kernel/bpf/verifier.c | 26 ++++++++++++++++++++++++++
  1 file changed, 26 insertions(+)

kernel/bpf/verifier.c +20 -0

index 3c8bb92..01c215d 100644
@@ -2975,6 +2975,32 @@ static int adjust_scalar_min_max_vals(struct bpf_verifier_env *env,
  		/* We may learn something more from the var_off */
  		__update_reg_bounds(dst_reg);
  		break;
+	case BPF_ARSH:
+		if (umax_val >= insn_bitness) {
+			/* Shifts greater than 31 or 63 are undefined.
+			 * This includes shifts by a negative number.
+			 */
+			mark_reg_unknown(env, regs, insn->dst_reg);
+			break;
+		}
+		if (dst_reg->smin_value < 0)
+			dst_reg->smin_value >>= umin_val;
+		else
+			dst_reg->smin_value >>= umax_val;
+		if (dst_reg->smax_value < 0)
+			dst_reg->smax_value >>= umax_val;
+		else
+			dst_reg->smax_value >>= umin_val;
+		if (src_known)
+			dst_reg->var_off = tnum_rshift(dst_reg->var_off,
+						       umin_val);
+		else

Quoted 8 lines

kernel/bpf/verifier.c static int adjust_scalar_min_max_vals(struct bpf_verifier_env *env, · line 3002

Quoted 5 lines

kernel/bpf/verifier.c +5 -0

+			dst_reg->var_off = tnum_rshift(tnum_unknown, umin_val);
+		dst_reg->umin_value >>= umax_val;
+		dst_reg->umax_value >>= umin_val;
+		/* We may learn something more from the var_off */
+		__update_reg_bounds(dst_reg);

I'm struggling to understand how these bounds are computed. Could you add examples in the comments?

Okay, let me try to add some comments for better understanding.

Quoted 4 lines

In particular if dst_reg is unknown (tnum.mask == -1) the above tnum_rshift() will clear upper bits and will make it 64-bit positive, but that doesn't seem correct.
What am I missing?

Considering this is arith shift, we probably should just have dst_reg->var_off = tnum_unknown to be conservative.

I could miss something here as well. Let me try to write more detailed explanation, hopefully to cover all corner cases.

Quoted 87 lines · Yonghong Song

Quoted 68 lines · Alexei Starovoitov

Quoted 67 lines · Yonghong Song

When helpers like bpf_get_stack returns an int value and later on used for arithmetic computation, the LSH and ARSH operations are often required to get proper sign extension into 64-bit. For example, without this patch:

      54: R0=inv(id=0,umax_value=800)
      54: (bf) r8 = r0
      55: R0=inv(id=0,umax_value=800) R8_w=inv(id=0,umax_value=800)
      55: (67) r8 <<= 32
      56: R8_w=inv(id=0,umax_value=3435973836800,var_off=(0x0; 0x3ff00000000))
      56: (c7) r8 s>>= 32
      57: R8=inv(id=0)
With this patch:
      54: R0=inv(id=0,umax_value=800)
      54: (bf) r8 = r0
      55: R0=inv(id=0,umax_value=800) R8_w=inv(id=0,umax_value=800)
      55: (67) r8 <<= 32
      56: R8_w=inv(id=0,umax_value=3435973836800,var_off=(0x0; 0x3ff00000000))
      56: (c7) r8 s>>= 32
      57: R8=inv(id=0, umax_value=800,var_off=(0x0; 0x3ff))

With better range of "R8", later on when "R8" is added to other register, e.g., a map pointer or scalar-value register, the better register range can be derived and verifier failure may be avoided.

In our later example,

      ......
      usize = bpf_get_stack(ctx, raw_data, max_len, BPF_F_USER_STACK);
      if (usize < 0)
          return 0;
      ksize = bpf_get_stack(ctx, raw_data + usize, max_len - usize, 0);
      ......

Without improving ARSH value range tracking, the register representing "max_len - usize" will have smin_value equal to S64_MIN and will be rejected by verifier.

Signed-off-by Yonghong Song <[email protected]>

---
   kernel/bpf/verifier.c | 26 ++++++++++++++++++++++++++
   1 file changed, 26 insertions(+)

kernel/bpf/verifier.c +20 -0

index 3c8bb92..01c215d 100644
@@ -2975,6 +2975,32 @@ static int adjust_scalar_min_max_vals(struct bpf_verifier_env *env,
   		/* We may learn something more from the var_off */
   		__update_reg_bounds(dst_reg);
   		break;
+	case BPF_ARSH:
+		if (umax_val >= insn_bitness) {
+			/* Shifts greater than 31 or 63 are undefined.
+			 * This includes shifts by a negative number.
+			 */
+			mark_reg_unknown(env, regs, insn->dst_reg);
+			break;
+		}
+		if (dst_reg->smin_value < 0)
+			dst_reg->smin_value >>= umin_val;
+		else
+			dst_reg->smin_value >>= umax_val;
+		if (dst_reg->smax_value < 0)
+			dst_reg->smax_value >>= umax_val;
+		else
+			dst_reg->smax_value >>= umin_val;
+		if (src_known)
+			dst_reg->var_off = tnum_rshift(dst_reg->var_off,
+						       umin_val);
+		else

Quoted 8 lines

kernel/bpf/verifier.c static int adjust_scalar_min_max_vals(struct bpf_verifier_env *env, · line 3002

Quoted 5 lines

kernel/bpf/verifier.c +5 -0

+			dst_reg->var_off = tnum_rshift(tnum_unknown, umin_val);
+		dst_reg->umin_value >>= umax_val;
+		dst_reg->umax_value >>= umin_val;
+		/* We may learn something more from the var_off */
+		__update_reg_bounds(dst_reg);

I'm struggling to understand how these bounds are computed. Could you add examples in the comments?

Okay, let me try to add some comments for better understanding.

Quoted 4 lines

In particular if dst_reg is unknown (tnum.mask == -1) the above tnum_rshift() will clear upper bits and will make it 64-bit positive, but that doesn't seem correct.
What am I missing?

Quoted 5 lines

Considering this is arith shift, we probably should just have dst_reg->var_off = tnum_unknown to be conservative.

I could miss something here as well. Let me try to write more detailed explanation, hopefully to cover all corner cases.

Is there a use case for !src_known ?

I think test_verifier should have 100% line coverage of verifier.c and every 'if' condition in the verifier needs to have real use case behind it.
It's still on my todo list to get rid of [su][min|max]_value tracking that was introduced without solid justification.

Quoted 91 lines · Alexei Starovoitov

Quoted 90 lines · Yonghong Song

Quoted 68 lines · Alexei Starovoitov

Quoted 67 lines · Yonghong Song

When helpers like bpf_get_stack returns an int value and later on used for arithmetic computation, the LSH and ARSH operations are often required to get proper sign extension into 64-bit. For example, without this patch:

      54: R0=inv(id=0,umax_value=800)
      54: (bf) r8 = r0
      55: R0=inv(id=0,umax_value=800) R8_w=inv(id=0,umax_value=800)
      55: (67) r8 <<= 32
      56: R8_w=inv(id=0,umax_value=3435973836800,var_off=(0x0; 0x3ff00000000))
      56: (c7) r8 s>>= 32
      57: R8=inv(id=0)
With this patch:
      54: R0=inv(id=0,umax_value=800)
      54: (bf) r8 = r0
      55: R0=inv(id=0,umax_value=800) R8_w=inv(id=0,umax_value=800)
      55: (67) r8 <<= 32
      56: R8_w=inv(id=0,umax_value=3435973836800,var_off=(0x0; 0x3ff00000000))
      56: (c7) r8 s>>= 32
      57: R8=inv(id=0, umax_value=800,var_off=(0x0; 0x3ff))

With better range of "R8", later on when "R8" is added to other register, e.g., a map pointer or scalar-value register, the better register range can be derived and verifier failure may be avoided.

In our later example,

      ......
      usize = bpf_get_stack(ctx, raw_data, max_len, BPF_F_USER_STACK);
      if (usize < 0)
          return 0;
      ksize = bpf_get_stack(ctx, raw_data + usize, max_len - usize, 0);
      ......

Without improving ARSH value range tracking, the register representing "max_len - usize" will have smin_value equal to S64_MIN and will be rejected by verifier.

Signed-off-by Yonghong Song <[email protected]>

---
   kernel/bpf/verifier.c | 26 ++++++++++++++++++++++++++
   1 file changed, 26 insertions(+)

kernel/bpf/verifier.c +20 -0

index 3c8bb92..01c215d 100644
@@ -2975,6 +2975,32 @@ static int adjust_scalar_min_max_vals(struct bpf_verifier_env *env,
   		/* We may learn something more from the var_off */
   		__update_reg_bounds(dst_reg);
   		break;
+	case BPF_ARSH:
+		if (umax_val >= insn_bitness) {
+			/* Shifts greater than 31 or 63 are undefined.
+			 * This includes shifts by a negative number.
+			 */
+			mark_reg_unknown(env, regs, insn->dst_reg);
+			break;
+		}
+		if (dst_reg->smin_value < 0)
+			dst_reg->smin_value >>= umin_val;
+		else
+			dst_reg->smin_value >>= umax_val;
+		if (dst_reg->smax_value < 0)
+			dst_reg->smax_value >>= umax_val;
+		else
+			dst_reg->smax_value >>= umin_val;
+		if (src_known)
+			dst_reg->var_off = tnum_rshift(dst_reg->var_off,
+						       umin_val);
+		else

Quoted 8 lines

kernel/bpf/verifier.c static int adjust_scalar_min_max_vals(struct bpf_verifier_env *env, · line 3002

Quoted 5 lines

kernel/bpf/verifier.c +5 -0

+			dst_reg->var_off = tnum_rshift(tnum_unknown, umin_val);
+		dst_reg->umin_value >>= umax_val;
+		dst_reg->umax_value >>= umin_val;
+		/* We may learn something more from the var_off */
+		__update_reg_bounds(dst_reg);

I'm struggling to understand how these bounds are computed. Could you add examples in the comments?

Okay, let me try to add some comments for better understanding.

Quoted 4 lines

In particular if dst_reg is unknown (tnum.mask == -1) the above tnum_rshift() will clear upper bits and will make it 64-bit positive, but that doesn't seem correct.
What am I missing?

Considering this is arith shift, we probably should just have dst_reg->var_off = tnum_unknown to be conservative.

Quoted 4 lines

Quoted 2 lines

I could miss something here as well. Let me try to write more detailed explanation, hopefully to cover all corner cases.

Is there a use case for !src_known ?

For typical bpf programs, the shift amount should always be known... If src_known is true, it must be dealing custom packets or custom data structures in tracing, etc.

kernel/bpf/verifier.c +9 -0

index 3c8bb92..01c215d 100644
@@ -2975,6 +2975,32 @@ static int adjust_scalar_min_max_vals(struct bpf_verifier_env *env,
 		/* We may learn something more from the var_off */
 		__update_reg_bounds(dst_reg);
 		break;
+	case BPF_ARSH:
+		if (umax_val >= insn_bitness) {
+			/* Shifts greater than 31 or 63 are undefined.
+			 * This includes shifts by a negative number.
+			 */
+			mark_reg_unknown(env, regs, insn->dst_reg);
+			break;
+		}
+		if (dst_reg->smin_value < 0)

kernel/bpf/verifier.c +3 -0

+		/* We may learn something more from the var_off */
+		__update_reg_bounds(dst_reg);
+		break;
 	default:
 		mark_reg_unknown(env, regs, insn->dst_reg);
 		break;

Quoted 17 lines · Yonghong Song

kernel/bpf/verifier.c +9 -0

index 3c8bb92..01c215d 100644
@@ -2975,6 +2975,32 @@ static int adjust_scalar_min_max_vals(struct bpf_verifier_env *env,
  		/* We may learn something more from the var_off */
  		__update_reg_bounds(dst_reg);
  		break;
+	case BPF_ARSH:
+		if (umax_val >= insn_bitness) {
+			/* Shifts greater than 31 or 63 are undefined.
+			 * This includes shifts by a negative number.
+			 */
+			mark_reg_unknown(env, regs, insn->dst_reg);
+			break;
+		}
+		if (dst_reg->smin_value < 0)

kernel/bpf/verifier.c static int adjust_scalar_min_max_vals(struct bpf_verifier_env *env, · line 2996

Quoted 10 lines

kernel/bpf/verifier.c +10 -0

+			dst_reg->smin_value >>= umin_val;
+		else
+			dst_reg->smin_value >>= umax_val;
+		if (dst_reg->smax_value < 0)
+			dst_reg->smax_value >>= umax_val;
+		else
+			dst_reg->smax_value >>= umin_val;
+		if (src_known)
+			dst_reg->var_off = tnum_rshift(dst_reg->var_off,
+						       umin_val);

tnum_rshift is an unsigned shift, it won't do what you want here. I think you could write a tnum_arshift that looks something like this
 (UNTESTED!):

    struct tnum tnum_arshift(struct tnum a, u8 shift)
    {
        return TNUM(((s64)a.value) >> shift, ((s64)a.mask) >> shift); }
Theory: if value sign bit is 1 then number is known negative so populate
 upper bits with known 1s.  If mask sign bit is 1 then number might be negative so populate upper bits with unknown.  Otherwise, number is known positive so populate upper bits with known 0s.

kernel/bpf/verifier.c static int adjust_scalar_min_max_vals(struct bpf_verifier_env *env, · line 2998

Quoted 2 lines

kernel/bpf/verifier.c +2 -0

+		else
+			dst_reg->var_off = tnum_rshift(tnum_unknown, umin_val);

Applying the above here, tnum_arshift(tnum_unknown, ...) would always just
 return tnum_unknown, so just do "dst_reg->var_off = tnum_unknown;".
The reason for the corresponding logic in the BPF_RSH case is that a right
 logical shift _always_ populates upper bits with zeroes.

In any case these 'else' branches are currently never taken because they
 fall foul of the check Alexei added just before the switch, if (!src_known &&
        opcode != BPF_ADD && opcode != BPF_SUB && opcode != BPF_AND) { __mark_reg_unknown(dst_reg);
        return 0;
    }
So I can guarantee you haven't tested this code :-)

kernel/bpf/verifier.c static int adjust_scalar_min_max_vals(struct bpf_verifier_env *env, · line 3000

Quoted 3 lines

kernel/bpf/verifier.c +2 -0

+		dst_reg->umin_value >>= umax_val;
+		dst_reg->umax_value >>= umin_val;

FWIW I think the way to handle umin/umax here is to blow them away and
 just rely on inferring new ubounds from the sbounds (i.e. the inverse of what we do just above in case BPF_RSH) since BPF_ARSH is essentially an operation on the signed value.  I don't think there is a need to support cases where the unsigned bounds of a signed shift of a value that may cross the sign boundary at (1<<63) are needed to verify a program.
(Unlike in the unsigned shift case, it is at least _possible_ for there to
 be information from the ubounds that we can't get from the sbounds - but it's a contrived case that isn't likely to be useful in real programs.)

-Ed

Quoted 6 lines

kernel/bpf/verifier.c +3 -0

+		/* We may learn something more from the var_off */
+		__update_reg_bounds(dst_reg);
+		break;
  	default:
  		mark_reg_unknown(env, regs, insn->dst_reg);
  		break;

There is no functionality change in this patch. The common-purpose trace functions, including perf_event polling and ksym lookup, are moved from trace_output_user.c and bpf_load.c to selftests/bpf/trace_helpers.c so that these function can be reused later in selftests.

Signed-off-by Yonghong Song <[email protected]>

The test attached a raw_tracepoint program to sched/sched_switch. It tested to get stack for user space, kernel space and user space with build_id request. It also tested to get user and kernel stack into the same buffer with back-to-back bpf_get_stack helper calls.

Whenever the kernel stack is available, the user space application will check to ensure that the kernel function for raw_tracepoint ___bpf_prog_run is part of the stack.

Signed-off-by Yonghong Song <[email protected]>

---
 tools/testing/selftests/bpf/Makefile               |   3 +-
 tools/testing/selftests/bpf/test_get_stack_rawtp.c | 102 ++++++++++++++++++
 tools/testing/selftests/bpf/test_progs.c           | 115 +++++++++++++++++++++
 3 files changed, 219 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/bpf/test_get_stack_rawtp.c

tools/testing/selftests/bpf/Makefile +2 -1

index 0b72cc7..54e9e74 100644
@@ -32,7 +32,7 @@ TEST_GEN_FILES = test_pkt_access.o test_xdp.o test_l4lb.o test_tcp_estats.o test
 	test_l4lb_noinline.o test_xdp_noinline.o test_stacktrace_map.o \
 	sample_map_ret0.o test_tcpbpf_kern.o test_stacktrace_build_id.o \
 	sockmap_tcp_msg_prog.o connect4_prog.o connect6_prog.o test_adjust_tail.o \
-	test_btf_haskv.o test_btf_nokv.o
+	test_btf_haskv.o test_btf_nokv.o test_get_stack_rawtp.o
 
 # Order correspond to 'make run_tests' order
 TEST_PROGS := test_kmod.sh \

@@ -56,6 +56,7 @@ $(TEST_GEN_PROGS_EXTENDED): $(OUTPUT)/libbpf.a
 $(OUTPUT)/test_dev_cgroup: cgroup_helpers.c
 $(OUTPUT)/test_sock: cgroup_helpers.c
 $(OUTPUT)/test_sock_addr: cgroup_helpers.c
+$(OUTPUT)/test_progs: trace_helpers.c
 
 .PHONY: force
 

tools/testing/selftests/bpf/test_get_stack_rawtp.c new file +81 -0

new file mode 100644
index 0000000..ba1dcf9
@@ -0,0 +1,102 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/bpf.h>
+#include "bpf_helpers.h"
+
+/* Permit pretty deep stack traces */
+#define MAX_STACK_RAWTP 100
+struct stack_trace_t {
+	int pid;
+	int kern_stack_size;
+	int user_stack_size;
+	int user_stack_buildid_size;
+	__u64 kern_stack[MAX_STACK_RAWTP];
+	__u64 user_stack[MAX_STACK_RAWTP];
+	struct bpf_stack_build_id user_stack_buildid[MAX_STACK_RAWTP];
+};
+
+struct bpf_map_def SEC("maps") perfmap = {
+	.type = BPF_MAP_TYPE_PERF_EVENT_ARRAY,
+	.key_size = sizeof(int),
+	.value_size = sizeof(__u32),
+	.max_entries = 2,
+};
+
+struct bpf_map_def SEC("maps") stackdata_map = {
+	.type = BPF_MAP_TYPE_PERCPU_ARRAY,
+	.key_size = sizeof(__u32),
+	.value_size = sizeof(struct stack_trace_t),
+	.max_entries = 1,
+};
+
+/* Allocate per-cpu space twice the needed. For the code below
+ *   usize = bpf_get_stack(ctx, raw_data, max_len, BPF_F_USER_STACK);
+ *   if (usize < 0)
+ *     return 0;
+ *   ksize = bpf_get_stack(ctx, raw_data + usize, max_len - usize, 0);
+ *
+ * If we have value_size = MAX_STACK_RAWTP * sizeof(__u64),
+ * verifier will complain that access "raw_data + usize"
+ * with size "max_len - usize" may be out of bound.
+ * The maximum "raw_data + usize" is "raw_data + max_len"
+ * and the maximum "max_len - usize" is "max_len", verifier
+ * concludes that the maximum buffer access range is
+ * "raw_data[0...max_len * 2 - 1]" and hence reject the program.
+ *
+ * Doubling the to-be-used max buffer size can fix this verifier
+ * issue and avoid complicated C programming massaging.
+ * This is an acceptable workaround since there is one entry here.
+ */
+struct bpf_map_def SEC("maps") rawdata_map = {
+	.type = BPF_MAP_TYPE_PERCPU_ARRAY,
+	.key_size = sizeof(__u32),
+	.value_size = MAX_STACK_RAWTP * sizeof(__u64) * 2,
+	.max_entries = 1,
+};
+
+SEC("tracepoint/sched/sched_switch")
+int bpf_prog1(void *ctx)
+{
+	int max_len, max_buildid_len, usize, ksize, total_size;
+	struct stack_trace_t *data;
+	void *raw_data;
+	__u32 key = 0;
+
+	data = bpf_map_lookup_elem(&stackdata_map, &key);
+	if (!data)
+		return 0;
+
+	max_len = MAX_STACK_RAWTP * sizeof(__u64);
+	max_buildid_len = MAX_STACK_RAWTP * sizeof(struct bpf_stack_build_id);
+	data->pid = bpf_get_current_pid_tgid();
+	data->kern_stack_size = bpf_get_stack(ctx, data->kern_stack,
+					      max_len, 0);
+	data->user_stack_size = bpf_get_stack(ctx, data->user_stack, max_len,
+					    BPF_F_USER_STACK);
+	data->user_stack_buildid_size = bpf_get_stack(
+		ctx, data->user_stack_buildid, max_buildid_len,
+		BPF_F_USER_STACK | BPF_F_USER_BUILD_ID);
+	bpf_perf_event_output(ctx, &perfmap, 0, data, sizeof(*data));
+
+	/* write both kernel and user stacks to the same buffer */

Quoted 134 lines · Yonghong Song

The test attached a raw_tracepoint program to sched/sched_switch. It tested to get stack for user space, kernel space and user space with build_id request. It also tested to get user and kernel stack into the same buffer with back-to-back bpf_get_stack helper calls.

Whenever the kernel stack is available, the user space application will check to ensure that the kernel function for raw_tracepoint ___bpf_prog_run is part of the stack.

Signed-off-by Yonghong Song <[email protected]>

---
  tools/testing/selftests/bpf/Makefile               |   3 +-
  tools/testing/selftests/bpf/test_get_stack_rawtp.c | 102 ++++++++++++++++++
  tools/testing/selftests/bpf/test_progs.c           | 115 +++++++++++++++++++++
  3 files changed, 219 insertions(+), 1 deletion(-)

create mode 100644 tools/testing/selftests/bpf/test_get_stack_rawtp.c

tools/testing/selftests/bpf/Makefile +2 -1

index 0b72cc7..54e9e74 100644
@@ -32,7 +32,7 @@ TEST_GEN_FILES = test_pkt_access.o test_xdp.o test_l4lb.o test_tcp_estats.o test
  	test_l4lb_noinline.o test_xdp_noinline.o test_stacktrace_map.o \
  	sample_map_ret0.o test_tcpbpf_kern.o test_stacktrace_build_id.o \
  	sockmap_tcp_msg_prog.o connect4_prog.o connect6_prog.o test_adjust_tail.o \
-	test_btf_haskv.o test_btf_nokv.o
+	test_btf_haskv.o test_btf_nokv.o test_get_stack_rawtp.o
 
  # Order correspond to 'make run_tests' order
  TEST_PROGS := test_kmod.sh \

@@ -56,6 +56,7 @@ $(TEST_GEN_PROGS_EXTENDED): $(OUTPUT)/libbpf.a
  $(OUTPUT)/test_dev_cgroup: cgroup_helpers.c
  $(OUTPUT)/test_sock: cgroup_helpers.c
  $(OUTPUT)/test_sock_addr: cgroup_helpers.c
+$(OUTPUT)/test_progs: trace_helpers.c
 
  .PHONY: force
 

tools/testing/selftests/bpf/test_get_stack_rawtp.c new file +89 -0

new file mode 100644
index 0000000..ba1dcf9
@@ -0,0 +1,102 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/bpf.h>
+#include "bpf_helpers.h"
+
+/* Permit pretty deep stack traces */
+#define MAX_STACK_RAWTP 100
+struct stack_trace_t {
+	int pid;
+	int kern_stack_size;
+	int user_stack_size;
+	int user_stack_buildid_size;
+	__u64 kern_stack[MAX_STACK_RAWTP];
+	__u64 user_stack[MAX_STACK_RAWTP];
+	struct bpf_stack_build_id user_stack_buildid[MAX_STACK_RAWTP];
+};
+
+struct bpf_map_def SEC("maps") perfmap = {
+	.type = BPF_MAP_TYPE_PERF_EVENT_ARRAY,
+	.key_size = sizeof(int),
+	.value_size = sizeof(__u32),
+	.max_entries = 2,
+};
+
+struct bpf_map_def SEC("maps") stackdata_map = {
+	.type = BPF_MAP_TYPE_PERCPU_ARRAY,
+	.key_size = sizeof(__u32),
+	.value_size = sizeof(struct stack_trace_t),
+	.max_entries = 1,
+};
+
+/* Allocate per-cpu space twice the needed. For the code below
+ *   usize = bpf_get_stack(ctx, raw_data, max_len, BPF_F_USER_STACK);
+ *   if (usize < 0)
+ *     return 0;
+ *   ksize = bpf_get_stack(ctx, raw_data + usize, max_len - usize, 0);
+ *
+ * If we have value_size = MAX_STACK_RAWTP * sizeof(__u64),
+ * verifier will complain that access "raw_data + usize"
+ * with size "max_len - usize" may be out of bound.
+ * The maximum "raw_data + usize" is "raw_data + max_len"
+ * and the maximum "max_len - usize" is "max_len", verifier
+ * concludes that the maximum buffer access range is
+ * "raw_data[0...max_len * 2 - 1]" and hence reject the program.
+ *
+ * Doubling the to-be-used max buffer size can fix this verifier
+ * issue and avoid complicated C programming massaging.
+ * This is an acceptable workaround since there is one entry here.
+ */
+struct bpf_map_def SEC("maps") rawdata_map = {
+	.type = BPF_MAP_TYPE_PERCPU_ARRAY,
+	.key_size = sizeof(__u32),
+	.value_size = MAX_STACK_RAWTP * sizeof(__u64) * 2,
+	.max_entries = 1,
+};
+
+SEC("tracepoint/sched/sched_switch")
+int bpf_prog1(void *ctx)
+{
+	int max_len, max_buildid_len, usize, ksize, total_size;
+	struct stack_trace_t *data;
+	void *raw_data;
+	__u32 key = 0;
+
+	data = bpf_map_lookup_elem(&stackdata_map, &key);
+	if (!data)
+		return 0;
+
+	max_len = MAX_STACK_RAWTP * sizeof(__u64);
+	max_buildid_len = MAX_STACK_RAWTP * sizeof(struct bpf_stack_build_id);
+	data->pid = bpf_get_current_pid_tgid();
+	data->kern_stack_size = bpf_get_stack(ctx, data->kern_stack,
+					      max_len, 0);
+	data->user_stack_size = bpf_get_stack(ctx, data->user_stack, max_len,
+					    BPF_F_USER_STACK);
+	data->user_stack_buildid_size = bpf_get_stack(
+		ctx, data->user_stack_buildid, max_buildid_len,
+		BPF_F_USER_STACK | BPF_F_USER_BUILD_ID);
+	bpf_perf_event_output(ctx, &perfmap, 0, data, sizeof(*data));
+
+	/* write both kernel and user stacks to the same buffer */
+	raw_data = bpf_map_lookup_elem(&rawdata_map, &key);
+	if (!raw_data)
+		return 0;
+
+	usize = bpf_get_stack(ctx, raw_data, max_len, BPF_F_USER_STACK);
+	if (usize < 0)
+		return 0;
+

tools/testing/selftests/bpf/test_get_stack_rawtp.c line 91

Quoted 2 lines

tools/testing/selftests/bpf/test_get_stack_rawtp.c new file +2 -0

+	ksize = bpf_get_stack(ctx, raw_data + usize, max_len - usize, 0);
+	if (ksize < 0)

may be instead of teaching verifier about ARSH (which doesn't look straighforward) such use case can be done as:
u32 max_len, usize, ksize;
ksize = bpf_get_stack(ctx, raw_data + usize, max_len - usize, 0); if ((int)ksize < 0)

That's certainly suboptimal and very much non obvious to program developers, but at least it can unblock the bpf_get_stack part landing and proper ARSH support can be added later? Just a thought.

tools/testing/selftests/bpf/test_get_stack_rawtp.c line 99

Quoted 8 lines

tools/testing/selftests/bpf/test_get_stack_rawtp.c new file +8 -0

+		return 0;
+
+	total_size = usize + ksize;
+	if (total_size > 0 && total_size <= max_len)
+		bpf_perf_event_output(ctx, &perfmap, 0, raw_data, total_size);
+
+	return 0;
+}

the rest of the test looks great. Thank you for adding such exhaustive test.

The test_stacktrace_map and test_stacktrace_build_id are enhanced to call bpf_get_stack in the helper to get the stack trace as well. The stack traces from bpf_get_stack and bpf_get_stackid are compared to ensure that for the same stack as represented as the same hash, their ip addresses or build id's must be the same.

Signed-off-by Yonghong Song <[email protected]>

---
 tools/testing/selftests/bpf/test_progs.c           | 63 +++++++++++++++++++---
 .../selftests/bpf/test_stacktrace_build_id.c       | 20 ++++++-
 tools/testing/selftests/bpf/test_stacktrace_map.c  | 20 +++++--
 3 files changed, 92 insertions(+), 11 deletions(-)

tools/testing/selftests/bpf/test_progs.c +0 -0

index dad4c3f..06b922a 100644

Quoted 22 lines · Yonghong Song

The test_stacktrace_map and test_stacktrace_build_id are enhanced to call bpf_get_stack in the helper to get the stack trace as well. The stack traces from bpf_get_stack and bpf_get_stackid are compared to ensure that for the same stack as represented as the same hash, their ip addresses or build id's must be the same.

Signed-off-by Yonghong Song <[email protected]>

---
  tools/testing/selftests/bpf/test_progs.c           | 63 +++++++++++++++++++---
  .../selftests/bpf/test_stacktrace_build_id.c       | 20 ++++++-
  tools/testing/selftests/bpf/test_stacktrace_map.c  | 20 +++++--
  3 files changed, 92 insertions(+), 11 deletions(-)

tools/testing/selftests/bpf/test_progs.c +0 -0

index dad4c3f..06b922a 100644
@@ -897,11 +897,40 @@ static int compare_map_keys(int map1_fd, int map2_fd)
  	return 0;
  }
 

tools/testing/selftests/bpf/test_progs.c static int compare_map_keys(int map1_fd, int map2_fd) · line 903

Quoted 4 lines

tools/testing/selftests/bpf/test_progs.c +4 -0

+static int compare_stack_ips(int smap_fd, int amap_fd, int stack_trace_len)
+{
+	__u32 key, next_key, *cur_key_p, *next_key_p;
+	char val_buf1[stack_trace_len], val_buf2[stack_trace_len];

the kernel is trying to get rid of VLAs. test_progs.c already uses them, but if possible let's not add more uses of them.

Other than that looks great.