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?
> 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.
> if (!local_storage) {
> rcu_read_unlock();
> return;
