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@xxxxxx> 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().
