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
