On Sun, Dec 10, 2023 at 11:37 PM Hou Tao <houtao@xxxxxxxxxxxxxxx> wrote:
>
> From: Hou Tao <houtao1@xxxxxxxxxx>
>
> There is no rcu-read-lock requirement for ops->map_fd_get_ptr() or
> ops->map_fd_put_ptr(), so doesn't use rcu-read-lock for these two
> callbacks and only uses rcu-read-lock for the underlying update
> operations in bpf_fd_{array,htab}_map_update_elem().
>
> Acked-by: Yonghong Song <yonghong.song@xxxxxxxxx>
> Signed-off-by: Hou Tao <houtao1@xxxxxxxxxx>
> ---
> kernel/bpf/arraymap.c | 2 ++
> kernel/bpf/hashtab.c | 2 ++
> kernel/bpf/syscall.c | 4 ----
> 3 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c
> index 8d365bda9a8b..6cf47bcb7b83 100644
> --- a/kernel/bpf/arraymap.c
> +++ b/kernel/bpf/arraymap.c
> @@ -863,7 +863,9 @@ int bpf_fd_array_map_update_elem(struct bpf_map *map, struct file *map_file,
> map->ops->map_poke_run(map, index, old_ptr, new_ptr);
> mutex_unlock(&array->aux->poke_mutex);
> } else {
> + rcu_read_lock();
> old_ptr = xchg(array->ptrs + index, new_ptr);
> + rcu_read_unlock();
> }
>
> if (old_ptr)
> diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
> index 5b9146fa825f..4c28fd51ac01 100644
> --- a/kernel/bpf/hashtab.c
> +++ b/kernel/bpf/hashtab.c
> @@ -2523,7 +2523,9 @@ int bpf_fd_htab_map_update_elem(struct bpf_map *map, struct file *map_file,
> if (IS_ERR(ptr))
> return PTR_ERR(ptr);
>
> + rcu_read_lock();
> ret = htab_map_update_elem(map, key, &ptr, map_flags);
> + rcu_read_unlock();
> if (ret)
> map->ops->map_fd_put_ptr(map, ptr, false);
>
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index a76467fda558..019d18d33d63 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -183,15 +183,11 @@ static int bpf_map_update_value(struct bpf_map *map, struct file *map_file,
> err = bpf_percpu_cgroup_storage_update(map, key, value,
> flags);
> } else if (IS_FD_ARRAY(map)) {
> - rcu_read_lock();
> err = bpf_fd_array_map_update_elem(map, map_file, key, value,
> flags);
> - rcu_read_unlock();
> } else if (map->map_type == BPF_MAP_TYPE_HASH_OF_MAPS) {
> - rcu_read_lock();
> err = bpf_fd_htab_map_update_elem(map, map_file, key, value,
> flags);
> - rcu_read_unlock();
Sorry. I misunderstood the previous diff.
Dropping rcu_read_lock() around bpf_fd_array_map_update_elem()
is actually mandatory, since it may do mutex_lock
which will splat under rcu CS.
Adding rcu_read_lock() to bpf_fd_htab_map_update_elem()
is necessary just to avoid the WARN.
The RCU CS doesn't provide any protection to any pointer.
It's worth adding a comment.
And
+ rcu_read_lock();
old_ptr = xchg(array->ptrs + index, new_ptr);
+ rcu_read_unlock();
is wrong and unnecessary.
Neither old_ptr nor new_ptr are rcu protected.
This rcu_read_lock() only causes confusion.
pw-bot: cr
