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.
>
> > diff --git a/kernel/bpf/bpf_task_storage.c b/kernel/bpf/bpf_task_storage.c
> > index bb69aea1a777..1def13ad5c72 100644
> > --- a/kernel/bpf/bpf_task_storage.c
> > +++ b/kernel/bpf/bpf_task_storage.c
> > @@ -17,6 +17,7 @@
> > #include <uapi/linux/btf.h>
> > #include <linux/btf_ids.h>
> > #include <linux/fdtable.h>
> > +#include <linux/rcupdate_trace.h>
> >
> > DEFINE_BPF_STORAGE_CACHE(task_cache);
> >
> > @@ -59,7 +60,8 @@ task_storage_lookup(struct task_struct *task, struct bpf_map *map,
> > struct bpf_local_storage *task_storage;
> > struct bpf_local_storage_map *smap;
> >
> > - task_storage = rcu_dereference(task->bpf_storage);
> > + task_storage =
> > + rcu_dereference_check(task->bpf_storage, bpf_rcu_lock_held());
> > if (!task_storage)
> > return NULL;
> >
> > @@ -77,7 +79,8 @@ void bpf_task_storage_free(struct task_struct *task)
> >
> > rcu_read_lock();
> >
> > - local_storage = rcu_dereference(task->bpf_storage);
> > + local_storage =
> > + rcu_dereference_check(task->bpf_storage, bpf_rcu_lock_held());
> This change is unnecessary. There is a rcu_read_lock() above.
Thanks, agreed.
>
> > if (!local_storage) {
> > rcu_read_unlock();
> > return;
>
