| Message ID | 20191119195712.3692027-1-yhs@fb.com |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | bpf: allow s32/u32 return types in verifier for bpf helpers | expand |
On Tue, Nov 19, 2019 at 11:57:12AM -0800, Yonghong Song wrote: > #define TNUM(_v, _m) (struct tnum){.value = _v, .mask = _m} > -/* A completely unknown value */ > +/* completely unknown 32-bit and 64-bit values */ > +const struct tnum tnum_unknown32 = { .value = 0, .mask = 0xffffffffULL }; > const struct tnum tnum_unknown = { .value = 0, .mask = -1 }; > > struct tnum tnum_const(u64 value) > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index a344b08aef77..945827351758 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -1024,6 +1024,15 @@ static void __mark_reg_unbounded(struct bpf_reg_state *reg) > reg->umax_value = U64_MAX; > } > > +/* Reset the min/max bounds of a sub register */ > +static void __mark_subreg_unbounded(struct bpf_reg_state *subreg) > +{ > + subreg->smin_value = S32_MIN; > + subreg->smax_value = S32_MAX; > + subreg->umin_value = 0; > + subreg->umax_value = U32_MAX; > +} when int32 is returned the above feels correct, but I think it conflicts with definition of tnum_unknown32, since it says that upper 32-bit should be zero. The typical verifier action after processing alu32 insn: if (BPF_CLASS(insn->code) != BPF_ALU64) { /* 32-bit ALU ops are (32,32)->32 */ coerce_reg_to_size(dst_reg, 4); } __reg_deduce_bounds(dst_reg); __reg_bound_offset(dst_reg); And that is correct behavior for alu32, but here the helper is returning 'int', so if the verifier says subreg->smin_value = S32_MIN; it means that upper bits will be non-zero. The helper can return (u64)-1 with all 64-bits being set to 1. If next insn after w0 = call helper; is w0 += imm; the verifier will do above coerce+deduce logic and clear upper bits. That's correct, but without extra alu32 operation on w0 the state of r0 is technically correct, but doesn't match r0->var_reg which is tnum_unknown32. I wonder whether it should be tnum_unknown instead with above __mark_subreg_unbounded() ? >+ > /* Mark a register as having a completely unknown (scalar) value. */ > static void __mark_reg_unknown(struct bpf_reg_state *reg) > { > @@ -1038,6 +1047,20 @@ static void __mark_reg_unknown(struct bpf_reg_state *reg) > __mark_reg_unbounded(reg); > } > > +/* Mark a sub register as having a completely unknown (scalar) value. */ > +static void __mark_subreg_unknown(struct bpf_reg_state *subreg) > +{ > + /* > + * Clear type, id, off, and union(map_ptr, range) and > + * padding between 'type' and union > + */ > + memset(subreg, 0, offsetof(struct bpf_reg_state, var_off)); > + subreg->type = SCALAR_VALUE; > + subreg->var_off = tnum_unknown32; > + subreg->frameno = 0; > + __mark_subreg_unbounded(subreg); > +}
On 11/19/19 2:00 PM, Alexei Starovoitov wrote: > On Tue, Nov 19, 2019 at 11:57:12AM -0800, Yonghong Song wrote: >> #define TNUM(_v, _m) (struct tnum){.value = _v, .mask = _m} >> -/* A completely unknown value */ >> +/* completely unknown 32-bit and 64-bit values */ >> +const struct tnum tnum_unknown32 = { .value = 0, .mask = 0xffffffffULL }; >> const struct tnum tnum_unknown = { .value = 0, .mask = -1 }; >> >> struct tnum tnum_const(u64 value) >> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c >> index a344b08aef77..945827351758 100644 >> --- a/kernel/bpf/verifier.c >> +++ b/kernel/bpf/verifier.c >> @@ -1024,6 +1024,15 @@ static void __mark_reg_unbounded(struct bpf_reg_state *reg) >> reg->umax_value = U64_MAX; >> } >> >> +/* Reset the min/max bounds of a sub register */ >> +static void __mark_subreg_unbounded(struct bpf_reg_state *subreg) >> +{ >> + subreg->smin_value = S32_MIN; >> + subreg->smax_value = S32_MAX; >> + subreg->umin_value = 0; >> + subreg->umax_value = U32_MAX; >> +} > > when int32 is returned the above feels correct, but I think it conflicts with > definition of tnum_unknown32, since it says that upper 32-bit should be zero. > The typical verifier action after processing alu32 insn: > if (BPF_CLASS(insn->code) != BPF_ALU64) { > /* 32-bit ALU ops are (32,32)->32 */ > coerce_reg_to_size(dst_reg, 4); > } > > __reg_deduce_bounds(dst_reg); > __reg_bound_offset(dst_reg); > > And that is correct behavior for alu32, but here the helper is returning > 'int', so if the verifier says subreg->smin_value = S32_MIN; > it means that upper bits will be non-zero. > The helper can return (u64)-1 with all 64-bits being set to 1. > If next insn after w0 = call helper; is w0 += imm; > the verifier will do above coerce+deduce logic and clear upper bits. > That's correct, but without extra alu32 operation on w0 the state > of r0 is technically correct, but doesn't match r0->var_reg > which is tnum_unknown32. > I wonder whether it should be tnum_unknown instead with above > __mark_subreg_unbounded() ? tnum_unknown should work since subreg {smin,smax,umin,umax}_value all in 32-bit range. The mask (-1) should work as upper 32-bit unsigned value is always 0. Will make the change and send another revision. > >> + >> /* Mark a register as having a completely unknown (scalar) value. */ >> static void __mark_reg_unknown(struct bpf_reg_state *reg) >> { >> @@ -1038,6 +1047,20 @@ static void __mark_reg_unknown(struct bpf_reg_state *reg) >> __mark_reg_unbounded(reg); >> } >> >> +/* Mark a sub register as having a completely unknown (scalar) value. */ >> +static void __mark_subreg_unknown(struct bpf_reg_state *subreg) >> +{ >> + /* >> + * Clear type, id, off, and union(map_ptr, range) and >> + * padding between 'type' and union >> + */ >> + memset(subreg, 0, offsetof(struct bpf_reg_state, var_off)); >> + subreg->type = SCALAR_VALUE; >> + subreg->var_off = tnum_unknown32; >> + subreg->frameno = 0; >> + __mark_subreg_unbounded(subreg); >> +} >
On 11/19/19 2:09 PM, Yonghong Song wrote: > > > On 11/19/19 2:00 PM, Alexei Starovoitov wrote: >> On Tue, Nov 19, 2019 at 11:57:12AM -0800, Yonghong Song wrote: >>> #define TNUM(_v, _m) (struct tnum){.value = _v, .mask = _m} >>> -/* A completely unknown value */ >>> +/* completely unknown 32-bit and 64-bit values */ >>> +const struct tnum tnum_unknown32 = { .value = 0, .mask = 0xffffffffULL }; >>> const struct tnum tnum_unknown = { .value = 0, .mask = -1 }; >>> >>> struct tnum tnum_const(u64 value) >>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c >>> index a344b08aef77..945827351758 100644 >>> --- a/kernel/bpf/verifier.c >>> +++ b/kernel/bpf/verifier.c >>> @@ -1024,6 +1024,15 @@ static void __mark_reg_unbounded(struct bpf_reg_state *reg) >>> reg->umax_value = U64_MAX; >>> } >>> >>> +/* Reset the min/max bounds of a sub register */ >>> +static void __mark_subreg_unbounded(struct bpf_reg_state *subreg) >>> +{ >>> + subreg->smin_value = S32_MIN; >>> + subreg->smax_value = S32_MAX; >>> + subreg->umin_value = 0; >>> + subreg->umax_value = U32_MAX; >>> +} >> >> when int32 is returned the above feels correct, but I think it conflicts with >> definition of tnum_unknown32, since it says that upper 32-bit should be zero. >> The typical verifier action after processing alu32 insn: >> if (BPF_CLASS(insn->code) != BPF_ALU64) { >> /* 32-bit ALU ops are (32,32)->32 */ >> coerce_reg_to_size(dst_reg, 4); >> } >> >> __reg_deduce_bounds(dst_reg); >> __reg_bound_offset(dst_reg); >> >> And that is correct behavior for alu32, but here the helper is returning >> 'int', so if the verifier says subreg->smin_value = S32_MIN; >> it means that upper bits will be non-zero. >> The helper can return (u64)-1 with all 64-bits being set to 1. >> If next insn after w0 = call helper; is w0 += imm; >> the verifier will do above coerce+deduce logic and clear upper bits. >> That's correct, but without extra alu32 operation on w0 the state >> of r0 is technically correct, but doesn't match r0->var_reg >> which is tnum_unknown32. >> I wonder whether it should be tnum_unknown instead with above >> __mark_subreg_unbounded() ? > > tnum_unknown should work since subreg {smin,smax,umin,umax}_value > all in 32-bit range. The mask (-1) should work as upper 32-bit unsigned > value is always 0. > > Will make the change and send another revision. An update for this bug. Investigated further with Alexei and found actually we have a LLVM bug where some LShift and Rshift is removed incorrectly at +alu32 mode. The following LLVM patch https://reviews.llvm.org/D70511/new/ has been merged which fixed LLVM issue. There are some kernel changes needed to make test_progs passing, which I will send out shortly. This patch set can be abandoned. > >> >>> + >>> /* Mark a register as having a completely unknown (scalar) value. */ >>> static void __mark_reg_unknown(struct bpf_reg_state *reg) >>> { >>> @@ -1038,6 +1047,20 @@ static void __mark_reg_unknown(struct bpf_reg_state *reg) >>> __mark_reg_unbounded(reg); >>> } >>> >>> +/* Mark a sub register as having a completely unknown (scalar) value. */ >>> +static void __mark_subreg_unknown(struct bpf_reg_state *subreg) >>> +{ >>> + /* >>> + * Clear type, id, off, and union(map_ptr, range) and >>> + * padding between 'type' and union >>> + */ >>> + memset(subreg, 0, offsetof(struct bpf_reg_state, var_off)); >>> + subreg->type = SCALAR_VALUE; >>> + subreg->var_off = tnum_unknown32; >>> + subreg->frameno = 0; >>> + __mark_subreg_unbounded(subreg); >>> +} >>
diff --git a/include/linux/bpf.h b/include/linux/bpf.h index e913dd5946ae..a727706d1e3c 100644 --- a/include/linux/bpf.h +++ b/include/linux/bpf.h @@ -224,7 +224,9 @@ enum bpf_arg_type { /* type of values returned from helper functions */ enum bpf_return_type { - RET_INTEGER, /* function returns integer */ + RET_INTEGER = 0, /* function returns s32/u32 */ + RET_INT32 = 0, + RET_INT64 = 1, /* function returns s64/u64 */ RET_VOID, /* function doesn't return anything */ RET_PTR_TO_MAP_VALUE, /* returns a pointer to map elem value */ RET_PTR_TO_MAP_VALUE_OR_NULL, /* returns a pointer to map elem value or NULL */ diff --git a/include/linux/tnum.h b/include/linux/tnum.h index c17af77f3fae..0f11f3af9ec5 100644 --- a/include/linux/tnum.h +++ b/include/linux/tnum.h @@ -19,7 +19,8 @@ struct tnum { /* Constructors */ /* Represent a known constant as a tnum. */ struct tnum tnum_const(u64 value); -/* A completely unknown value */ +/* Completely unknown 32-bit and 64-bit values */ +extern const struct tnum tnum_unknown32; extern const struct tnum tnum_unknown; /* A value that's unknown except that @min <= value <= @max */ struct tnum tnum_range(u64 min, u64 max); diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c index 5e28718928ca..86c757a1801c 100644 --- a/kernel/bpf/helpers.c +++ b/kernel/bpf/helpers.c @@ -149,7 +149,7 @@ BPF_CALL_0(bpf_ktime_get_ns) const struct bpf_func_proto bpf_ktime_get_ns_proto = { .func = bpf_ktime_get_ns, .gpl_only = true, - .ret_type = RET_INTEGER, + .ret_type = RET_INT64, }; BPF_CALL_0(bpf_get_current_pid_tgid) @@ -165,7 +165,7 @@ BPF_CALL_0(bpf_get_current_pid_tgid) const struct bpf_func_proto bpf_get_current_pid_tgid_proto = { .func = bpf_get_current_pid_tgid, .gpl_only = false, - .ret_type = RET_INTEGER, + .ret_type = RET_INT64, }; BPF_CALL_0(bpf_get_current_uid_gid) @@ -185,7 +185,7 @@ BPF_CALL_0(bpf_get_current_uid_gid) const struct bpf_func_proto bpf_get_current_uid_gid_proto = { .func = bpf_get_current_uid_gid, .gpl_only = false, - .ret_type = RET_INTEGER, + .ret_type = RET_INT64, }; BPF_CALL_2(bpf_get_current_comm, char *, buf, u32, size) @@ -323,7 +323,7 @@ BPF_CALL_0(bpf_get_current_cgroup_id) const struct bpf_func_proto bpf_get_current_cgroup_id_proto = { .func = bpf_get_current_cgroup_id, .gpl_only = false, - .ret_type = RET_INTEGER, + .ret_type = RET_INT64, }; #ifdef CONFIG_CGROUP_BPF diff --git a/kernel/bpf/tnum.c b/kernel/bpf/tnum.c index ca52b9642943..7248e83adf45 100644 --- a/kernel/bpf/tnum.c +++ b/kernel/bpf/tnum.c @@ -10,7 +10,8 @@ #include <linux/tnum.h> #define TNUM(_v, _m) (struct tnum){.value = _v, .mask = _m} -/* A completely unknown value */ +/* completely unknown 32-bit and 64-bit values */ +const struct tnum tnum_unknown32 = { .value = 0, .mask = 0xffffffffULL }; const struct tnum tnum_unknown = { .value = 0, .mask = -1 }; struct tnum tnum_const(u64 value) diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index a344b08aef77..945827351758 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -1024,6 +1024,15 @@ static void __mark_reg_unbounded(struct bpf_reg_state *reg) reg->umax_value = U64_MAX; } +/* Reset the min/max bounds of a sub register */ +static void __mark_subreg_unbounded(struct bpf_reg_state *subreg) +{ + subreg->smin_value = S32_MIN; + subreg->smax_value = S32_MAX; + subreg->umin_value = 0; + subreg->umax_value = U32_MAX; +} + /* Mark a register as having a completely unknown (scalar) value. */ static void __mark_reg_unknown(struct bpf_reg_state *reg) { @@ -1038,6 +1047,20 @@ static void __mark_reg_unknown(struct bpf_reg_state *reg) __mark_reg_unbounded(reg); } +/* Mark a sub register as having a completely unknown (scalar) value. */ +static void __mark_subreg_unknown(struct bpf_reg_state *subreg) +{ + /* + * Clear type, id, off, and union(map_ptr, range) and + * padding between 'type' and union + */ + memset(subreg, 0, offsetof(struct bpf_reg_state, var_off)); + subreg->type = SCALAR_VALUE; + subreg->var_off = tnum_unknown32; + subreg->frameno = 0; + __mark_subreg_unbounded(subreg); +} + static void mark_reg_unknown(struct bpf_verifier_env *env, struct bpf_reg_state *regs, u32 regno) { @@ -4040,7 +4063,7 @@ static void do_refine_retval_range(struct bpf_reg_state *regs, int ret_type, { struct bpf_reg_state *ret_reg = ®s[BPF_REG_0]; - if (ret_type != RET_INTEGER || + if ((ret_type != RET_INTEGER && ret_type != RET_INT64) || (func_id != BPF_FUNC_get_stack && func_id != BPF_FUNC_probe_read_str)) return; @@ -4109,7 +4132,7 @@ static int check_reference_leak(struct bpf_verifier_env *env) static int check_helper_call(struct bpf_verifier_env *env, int func_id, int insn_idx) { const struct bpf_func_proto *fn = NULL; - struct bpf_reg_state *regs; + struct bpf_reg_state *regs, *reg0; struct bpf_call_arg_meta meta; bool changes_data; int i, err; @@ -4210,13 +4233,18 @@ static int check_helper_call(struct bpf_verifier_env *env, int func_id, int insn check_reg_arg(env, caller_saved[i], DST_OP_NO_MARK); } - /* helper call returns 64-bit value. */ - regs[BPF_REG_0].subreg_def = DEF_NOT_SUBREG; + /* helper call returns 64-bit value internally. */ + reg0 = regs + BPF_REG_0; + reg0->subreg_def = DEF_NOT_SUBREG; /* update return register (already marked as written above) */ - if (fn->ret_type == RET_INTEGER) { + if (fn->ret_type == RET_INT64) { /* sets type to SCALAR_VALUE */ mark_reg_unknown(env, regs, BPF_REG_0); + } else if (fn->ret_type == RET_INTEGER) { + /* sets type to SCALAR_VALUE */ + __mark_subreg_unknown(reg0); + __set_unknown_reg_precise(env, reg0); } else if (fn->ret_type == RET_VOID) { regs[BPF_REG_0].type = NOT_INIT; } else if (fn->ret_type == RET_PTR_TO_MAP_VALUE_OR_NULL || diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c index ffc91d4935ac..0bfce8499212 100644 --- a/kernel/trace/bpf_trace.c +++ b/kernel/trace/bpf_trace.c @@ -500,7 +500,7 @@ BPF_CALL_2(bpf_perf_event_read, struct bpf_map *, map, u64, flags) static const struct bpf_func_proto bpf_perf_event_read_proto = { .func = bpf_perf_event_read, .gpl_only = true, - .ret_type = RET_INTEGER, + .ret_type = RET_INT64, .arg1_type = ARG_CONST_MAP_PTR, .arg2_type = ARG_ANYTHING, }; @@ -673,7 +673,7 @@ BPF_CALL_0(bpf_get_current_task) static const struct bpf_func_proto bpf_get_current_task_proto = { .func = bpf_get_current_task, .gpl_only = true, - .ret_type = RET_INTEGER, + .ret_type = RET_INT64, }; BPF_CALL_2(bpf_current_task_under_cgroup, struct bpf_map *, map, u32, idx) diff --git a/net/core/filter.c b/net/core/filter.c index 49ded4a7588a..b9c45a986c73 100644 --- a/net/core/filter.c +++ b/net/core/filter.c @@ -1998,7 +1998,7 @@ static const struct bpf_func_proto bpf_csum_diff_proto = { .func = bpf_csum_diff, .gpl_only = false, .pkt_access = true, - .ret_type = RET_INTEGER, + .ret_type = RET_INT64, .arg1_type = ARG_PTR_TO_MEM_OR_NULL, .arg2_type = ARG_CONST_SIZE_OR_ZERO, .arg3_type = ARG_PTR_TO_MEM_OR_NULL, @@ -2021,7 +2021,7 @@ BPF_CALL_2(bpf_csum_update, struct sk_buff *, skb, __wsum, csum) static const struct bpf_func_proto bpf_csum_update_proto = { .func = bpf_csum_update, .gpl_only = false, - .ret_type = RET_INTEGER, + .ret_type = RET_INT64, .arg1_type = ARG_PTR_TO_CTX, .arg2_type = ARG_ANYTHING, }; @@ -4108,7 +4108,7 @@ BPF_CALL_1(bpf_skb_cgroup_id, const struct sk_buff *, skb) static const struct bpf_func_proto bpf_skb_cgroup_id_proto = { .func = bpf_skb_cgroup_id, .gpl_only = false, - .ret_type = RET_INTEGER, + .ret_type = RET_INT64, .arg1_type = ARG_PTR_TO_CTX, }; @@ -4133,7 +4133,7 @@ BPF_CALL_2(bpf_skb_ancestor_cgroup_id, const struct sk_buff *, skb, int, static const struct bpf_func_proto bpf_skb_ancestor_cgroup_id_proto = { .func = bpf_skb_ancestor_cgroup_id, .gpl_only = false, - .ret_type = RET_INTEGER, + .ret_type = RET_INT64, .arg1_type = ARG_PTR_TO_CTX, .arg2_type = ARG_ANYTHING, }; @@ -4179,7 +4179,7 @@ BPF_CALL_1(bpf_get_socket_cookie, struct sk_buff *, skb) static const struct bpf_func_proto bpf_get_socket_cookie_proto = { .func = bpf_get_socket_cookie, .gpl_only = false, - .ret_type = RET_INTEGER, + .ret_type = RET_INT64, .arg1_type = ARG_PTR_TO_CTX, }; @@ -4191,7 +4191,7 @@ BPF_CALL_1(bpf_get_socket_cookie_sock_addr, struct bpf_sock_addr_kern *, ctx) static const struct bpf_func_proto bpf_get_socket_cookie_sock_addr_proto = { .func = bpf_get_socket_cookie_sock_addr, .gpl_only = false, - .ret_type = RET_INTEGER, + .ret_type = RET_INT64, .arg1_type = ARG_PTR_TO_CTX, }; @@ -4203,7 +4203,7 @@ BPF_CALL_1(bpf_get_socket_cookie_sock_ops, struct bpf_sock_ops_kern *, ctx) static const struct bpf_func_proto bpf_get_socket_cookie_sock_ops_proto = { .func = bpf_get_socket_cookie_sock_ops, .gpl_only = false, - .ret_type = RET_INTEGER, + .ret_type = RET_INT64, .arg1_type = ARG_PTR_TO_CTX, }; @@ -5931,7 +5931,7 @@ static const struct bpf_func_proto bpf_tcp_gen_syncookie_proto = { .func = bpf_tcp_gen_syncookie, .gpl_only = true, /* __cookie_v*_init_sequence() is GPL */ .pkt_access = true, - .ret_type = RET_INTEGER, + .ret_type = RET_INT64, .arg1_type = ARG_PTR_TO_SOCK_COMMON, .arg2_type = ARG_PTR_TO_MEM, .arg3_type = ARG_CONST_SIZE,
Currently, for all helpers with integer return type, the verifier permits a single return type, RET_INTEGER, which represents 64-bit return value from the helper, and the verifier will assign 64-bit value ranges for these return values. Such an assumption is different from what compiler sees and the generated code with llvm alu32 mode, and may lead verification failure. For example, with latest llvm, selftest test_progs failed for program raw_tracepoint/kfree_skb. The source code looks like below: static __always_inline uint64_t read_str_var(...) { len = bpf_probe_read_user_str(payload, STROBE_MAX_STR_LEN, value->ptr); if (len > STROBE_MAX_STR_LEN) return 0; ... return len; } for (int i = 0; i < STROBE_MAX_STRS; ++i) { payload += read_str_var(cfg, i, tls_base, &value, data, payload); } In the above, "cfg" refers to map "strobemeta_cfgs" and the signature for bpf_probe_read_user_str() is: static int (*bpf_probe_read_user_str)(void *dst, __u32 size, const void *unsafe_ptr) = (void *) 114; in tools/lib/bpf/bpf_helper_defs.h. The code path causing verification failure looks like below: 193: call bpf_probe_read_user_str 194: if (w0 > 0x1) goto pc + 2 195: *(u16 *)(r7 +80) = r0 196: r6 = r0 ... 199: r8 = *(u64 *)(r10 -416) 200: r8 += r6 R1 unbounded memory access, make sure to bounds check any array access into a map After insn 193, the current verifier assumes r0 can be any 64-bit value. R0=inv(id=0) At insn 194, the compiler assumes bpf_probe_read_user_str() will return an __s32 value hences uses subregister w0 at insn 194 and at branch false target, the w0 range can be refined to unsigned values <= 1. But since insn 193 marks r0 with non-empty upper 32bit value, the new umax_value becomes 0xffffffff00000001. R0_w=inv(id=0,umax_value=18446744069414584321) At insn 196, the register r0 is assigned to r6. At insn 199, map pointer to strobemeta_cfgs map is restored and further refined at insn 200 and 201. At insn 200, the verifier complains r8 + r6 unbounded memory access since r6 has R6_rw=invP(id=0,umax_value=18446744069414584321) The pattern can be roughly described by the following steps: . helper return 32bit value, but return value marked conservatively. . sub-registeer w0 is refined. . r0 is used and the refined w0 range is lost. . later use of r0 may trigger some kind of unbound failure. To remove such failure, r0 range at insn 193 should be more aligned with what user expect based on function prototype in bpf_helper_defs.h, i.e., its value range should be with 32-bit value. This patches distinguished 32-bit from 64-bit return values in verifier with proper return value ranges in r0. In the above case, after insn 193, the verifier will give r0 range as R0_w=inv(id=0,smin_value=-2147483648,smax_value=2147483647,umax_value=4294967295,var_off=(0x0; 0xffffffff)) after insn 194, the new r0 range will be R0_w=inv(id=0,umax_value=1,var_off=(0x0; 0x1)) This better register range avoids above verification failure. Signed-off-by: Yonghong Song <yhs@fb.com> --- include/linux/bpf.h | 4 +++- include/linux/tnum.h | 3 ++- kernel/bpf/helpers.c | 8 ++++---- kernel/bpf/tnum.c | 3 ++- kernel/bpf/verifier.c | 38 +++++++++++++++++++++++++++++++++----- kernel/trace/bpf_trace.c | 4 ++-- net/core/filter.c | 16 ++++++++-------- 7 files changed, 54 insertions(+), 22 deletions(-)