Re: BPF/Question: PTR_TRUSTED vs PTR_UNTRUSTED

[Matt Bobrowski <mattbobrowski@google.com>]

On Thu, Jul 20, 2023 at 10:16:22AM -0500, David Vernet wrote:
> > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > > index fa43dc8e85b9..8b8ccde342f9 100644
> > > --- a/kernel/bpf/verifier.c
> > > +++ b/kernel/bpf/verifier.c
> > > @@ -5857,6 +5857,7 @@ BTF_TYPE_SAFE_RCU(struct task_struct) {
> > >         struct css_set __rcu *cgroups;
> > >         struct task_struct __rcu *real_parent;
> > >         struct task_struct *group_leader;
> > > + struct fs_struct *fs;
> > >  };
> > 
> > Oh, right. So, if we explicitly dereference the struct fs_struct
> > member of struct task_struct within a RCU read-side critical section,
> > the BPF verifier considers the pointer to struct fs_struct as being
> > safe and trusted. Is that right?
> 
> With the above patch, yes.

After conducting some further tests today, it turns out that making
amendments to the struct task_struct BTF_TYPE_SAFE_RCU definition
perhaps isn't actually necessary? As of commit afeebf9f57a49 ("bpf:
Undo strict enforcement for walking untagged fields"), if a trusted
pointer (in this case being struct task_struct obtained via
bpf_get_current_task_btf()) is dereferenced within a RCU read-side
critical section, then the pointer that is yielded as a result of the
walk/dereference operation is a PTR_TO_BTF_ID. It is neither trusted
or untrusted and therefore carries the same level of semantics as a
dereferenced pointer before any trust status for pointers was
introduced within the BPF verifier.

Have I misunderstood something here?

> > Why is it that we need to explicitly add it to such lists so that
> > they're considered to be trusted and cannot simply perform the
> > bpf_rcu_read_lock/unlock() dance from within the BPF program? Also,
> > should we not add the field to BTF_TYPE_SAFE_RCU_OR_NULL() instead of
> > BTF_TYPE_SAFE_RCU(), as struct fs_struct could perhaps be NULL in some
> > circumstances?
> 
> I recommend doing some git log / git blame digging. All of this
> information was captured in prior discussions. For example, in the patch
> [0] which added these structs.
> 
> [0]: https://lore.kernel.org/bpf/20230303041446.3630-7-alexei.starovoitov@gmail.com/
> 
> > Are you OK with me carrying this recommended patch to the mailing
> > list?
> 
> Of course

Based on what I've mentioned above, perhaps sending through a patch no
longer is necessary?

/M