On Fri, Jul 21, 2023 at 04:44:22PM +0000, Matt Bobrowski wrote:
> 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?
No, that's correct. You only need the aforementioned patch if you need
the pointer to be a trusted or RCU pointer.
> > > 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@xxxxxxxxx/
> >
> > > 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?
If you only need to call bpf_d_path() then yes, you shouldn't need the
patch.
