On Fri, 1 Nov 2024 at 17:56, Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> 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?
> >
We _can_ do this for all programs. The thought process here was to
leave existing raw_tp programs unbroken if possible if we're marking
their arguments as PTR_MAYBE_NULL, since most of them won't be
performing any NULL checks at all.
> > > + 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.
Ok. But don't allow passing such pointers into helpers, right?
We do that for raw_tp to preserve compat, but it would just exacerbate
the issue if we start doing it everywhere.
So it's just that dereferencing a _or_null pointer becomes an ok thing to do?
Let me mull over this for a bit.
I'm not sure whether not doing the NULL check is better or worse
though. On one hand everything will work without checking for NULL, on
the other hand people may also assume the verifier isn't complaining
because the pointer is valid, and then they read data from the pointer
which always ends up being zero, meaning different things for
different kinds of fields.
Just thinking out loud, but one of the other concerns would be that
we're encouraging people not to do these NULL checks, which means a
potential page fault penalty everytime that pointer _is_ NULL, instead
of a simple branch, which would certainly be a bit expensive. If this
becomes the common case, I think the prog execution latency penalty
will be big. It is something to consider.
