On Wed, Dec 08, 2021 at 11:00:04PM -0800, Martin KaFai Lau wrote: > 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(). KP, if the above comment makes sense, do you want to respin v3 ? or I can also help to respin and keep your SOB? Thanks.
