On Fri, Nov 1, 2024 at 12:16 PM Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote: > > > @@ -6693,7 +6709,21 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env, > > > > if (ret < 0) > > return ret; > > - > > + /* For raw_tp progs, we allow dereference of PTR_MAYBE_NULL > > + * trusted PTR_TO_BTF_ID, these are the ones that are possibly > > + * arguments to the raw_tp. Since internal checks in for trusted > > + * reg in check_ptr_to_btf_access would consider PTR_MAYBE_NULL > > + * modifier as problematic, mask it out temporarily for the > > + * check. Don't apply this to pointers with ref_obj_id > 0, as > > + * those won't be raw_tp args. > > + * > > + * We may end up applying this relaxation to other trusted > > + * PTR_TO_BTF_ID with maybe null flag, since we cannot > > + * distinguish PTR_MAYBE_NULL tagged for arguments vs normal > > + * tagging, but that should expand allowed behavior, and not > > + * cause regression for existing behavior. > > + */ > > Yeah, I'm not sure why this has to be raw tp-specific?.. What's wrong > with the same behavior for BPF iterator programs, for example? > > It seems nicer if we can avoid this temporary masking and instead > support this as a generic functionality? Or are there complications? > > > + mask = mask_raw_tp_reg(env, reg); > > if (ret != PTR_TO_BTF_ID) { > > /* just mark; */ > > > > @@ -6754,8 +6784,13 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env, > > clear_trusted_flags(&flag); > > } > > > > - if (atype == BPF_READ && value_regno >= 0) > > + if (atype == BPF_READ && value_regno >= 0) { > > mark_btf_ld_reg(env, regs, value_regno, ret, reg->btf, btf_id, flag); > > + /* We've assigned a new type to regno, so don't undo masking. */ > > + if (regno == value_regno) > > + mask = false; > > + } > > + unmask_raw_tp_reg(reg, mask); Kumar, I chatted with Andrii offline. All other cases of mask/unmask should probably stay raw_tp specific, but it seems we can make this particular case to be generic. Something like the following: diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 797cf3ed32e0..bbd4c03460e3 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -6703,7 +6703,11 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env, */ flag = PTR_UNTRUSTED; + } else if (reg->type == (PTR_TO_BTF_ID | PTR_TRUSTED | PTR_MAYBE_NULL)) { + flag |= PTR_MAYBE_NULL; + goto trusted; } else if (is_trusted_reg(reg) || is_rcu_reg(reg)) { +trusted: With the idea that trusted_or_null stays that way for all prog types and bpf_iter__task->task deref stays trusted_or_null instead of being downgraded to ptr_to_btf_id without any flags. So progs can do few less != null checks. Need to think it through.