Re: [PATCH v2 bpf-next 1/2] bpf: Allow bpf_local_storage to be used by sleepable programs

[Martin KaFai Lau <kafai@fb.com>]

On Thu, Dec 09, 2021 at 03:18:21AM +0100, KP Singh wrote:
> On Thu, Dec 9, 2021 at 3:00 AM Martin KaFai Lau <kafai@fb.com> wrote:
> >
> > On Mon, Dec 06, 2021 at 03:19:08PM +0000, KP Singh wrote:
> > [ ... ]
> >
> > > diff --git a/kernel/bpf/bpf_inode_storage.c b/kernel/bpf/bpf_inode_storage.c
> > > index 96ceed0e0fb5..20604d904d14 100644
> > > --- a/kernel/bpf/bpf_inode_storage.c
> > > +++ b/kernel/bpf/bpf_inode_storage.c
> > > @@ -17,6 +17,7 @@
> > >  #include <linux/bpf_lsm.h>
> > >  #include <linux/btf_ids.h>
> > >  #include <linux/fdtable.h>
> > > +#include <linux/rcupdate_trace.h>
> > >
> > >  DEFINE_BPF_STORAGE_CACHE(inode_cache);
> > >
> > > @@ -44,7 +45,8 @@ static struct bpf_local_storage_data *inode_storage_lookup(struct inode *inode,
> > >       if (!bsb)
> > >               return NULL;
> > >
> > > -     inode_storage = rcu_dereference(bsb->storage);
> > > +     inode_storage =
> > > +             rcu_dereference_check(bsb->storage, bpf_rcu_lock_held());
> > >       if (!inode_storage)
> > >               return NULL;
> > >
> > > @@ -97,7 +99,8 @@ void bpf_inode_storage_free(struct inode *inode)
> > >        * local_storage->list was non-empty.
> > >        */
> > >       if (free_inode_storage)
> > > -             kfree_rcu(local_storage, rcu);
> > > +             call_rcu_tasks_trace(&local_storage->rcu,
> > > +                                  bpf_local_storage_free_rcu);
> > It is not clear to me why bpf_inode_storage_free() needs this change
> > but not in bpf_task_storage_free() and bpf_sk_storage_free().
> > Could you explain the reason here?
> 
> I think I carried this forward from my older version and messed it up
> while applying diffs, I tested on the linux-next branch which has it
> for the other storages as well.
> 
> We will need to free all these under trace RCU. Will fix it in v3.
For sk, bpf_sk_storage_free() is called when sk is about to be kfree.
My understanding is the sleepable bpf_lsm should not be running
with this sk in parallel at this point when the sk has already reached
the bpf_sk_storage_free().  iow, call_rcu_tasks_trace should not
be needed here.  The existing kfree_rcu() is for the
bpf_local_storage_map_free.

I was not sure for inode since the inode's storage life time
is not obvious to me, so the earlier question.

After another thought, the synchronize_rcu_mult changes in
bpf_local_storage_map_free() is also not needed.  The first
existing synchronize_rcu() is for the bpf_sk_storage_clone().
The second one is for the bpf_(sk|task|inode)_storage_free().