Re: [PATCH v4 bpf-next 6/6] bpf: Refactor RCU enforcement in the verifier.

[David Vernet <void@manifault.com>]

On Thu, Mar 02, 2023 at 01:23:44PM -0800, Alexei Starovoitov wrote:

[...]

> > > +		if (type_is_trusted(env, reg, off)) {
> > > +			flag |= PTR_TRUSTED;
> > > +		} else if (in_rcu_cs(env) && !type_may_be_null(reg->type)) {
> > > +			if (type_is_rcu(env, reg, off)) {
> > > +				/* ignore __rcu tag and mark it MEM_RCU */
> > > +				flag |= MEM_RCU;
> > > +			} else if (flag & MEM_RCU) {
> > > +				/* __rcu tagged pointers can be NULL */
> > > +				flag |= PTR_MAYBE_NULL;
> > 
> > I'm not quite understanding the distinction between manually-specified
> > RCU-safe types being non-nullable, vs. __rcu pointers being nullable.
> > Aren't they functionally the exact same thing, with the exception being
> > that gcc doesn't support __rcu, so we've decided to instead manually
> > specify them for some types that we know we need until __rcu is the
> > default mechanism?  If the plan is to remove these macros once gcc
> > supports __rcu, this could break some programs that are expecting the
> > fields to be non-NULL, no?
> 
> BTF_TYPE_SAFE_RCU is a workaround for now.
> We can make it exactly like __rcu, but it would split
> the natural dereference of task->cgroups->dfl_cgrp into
> two derefs with extra !=NULL check in-between which is ugly and unnecessary.
> 
> > I see why we're doing this in the interim -- task->cgroups,
> > css->dfl_cgrp, task->cpus_ptr, etc can never be NULL. The problem is
> > that I think those are implementation details that are separate from the
> > pointers being RCU safe. This seems rather like we need a separate
> > non-nullable tag, or something to that effect.
> 
> Right. It is certainly an implementation detail.
> We'd need a new __not_null_mostly tag or __not_null_after_init.
> (similar to __read_mostly and __ro_after_init).
> Where non-null property is true when bpf get to see these structures.
> 
> The current allowlist is incomplete and far from perfect.
> I suspect we'd need to add a bunch more during this release cycle.
> This patch is aggressive in deprecation of old ptr_to_btf_id.
> Some breakage is expected. Hence the timing to do it right now
> at the beginning of the cycle.

Thanks for explaining. This all sounds good -- I'm certainly in favor of
being aggressive in deprecating the old ptr_to_btf_id approach. I was
really just worried that we'd break progs when we got rid of
BTF_TYPE_SAFE_RCU and started to use __rcu once gcc supported it, but as
you said we can just add another type tag at that time. And if we need
to add another RCU-safe pointer that is NULL-able before we have the gcc
support we need, we can always just add something like a
BTF_TYPE_SAFE_NULLABLE_RCU in the interim. Clearly a temporary solution,
but really not a bad one at all.

> 
> > >  		flag &= ~PTR_TRUSTED;
> > 
> > Do you know what else is left for us to fix to be able to just set
> > PTR_UNTRUSTED here?
> 
> All "ctx->" derefs. check_ctx_access() returns old school PTR_TO_BTF_ID.
> We can probably mark all of them as trusted, but need to audit a lot of code.
> I've also played with forcing helpers with ARG_PTR_TO_BTF_ID to be trusted,
> but still too much selftest breakage to even look at.
> 
> The patch also has:
> +                       if (BTF_INFO_KIND(mtype->info) == BTF_KIND_UNION &&
> +                           btf_type_vlen(mtype) != 1)
> +                               /*
> +                                * walking unions yields untrusted pointers
> +                                * with exception of __bpf_md_ptr and other
> +                                * unions with a single member
> +                                */
> +                               *flag |= PTR_UNTRUSTED;
> this is in particular to make skb->dev deref to return untrusted.
> In this past we allowed skb->dev->ifindex to go via PTR_TO_BTF_ID and PROBE_MEM.
> It's safe, but not clean. And we have no safe way to get trusted 'dev' to pass into helpers.
> It's time to clean this all up as well, but it will require rearranging fields in sk_buff.
> Lots of work ahead.

SGTM, thanks