On Wed, Dec 13, 2023 at 5:57 PM Hou Tao <houtao@xxxxxxxxxxxxxxx> wrote:
>
> Hi,
>
> On 12/14/2023 9:10 AM, Alexei Starovoitov wrote:
> > 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.
>
> Acquiring mutex_lock is only possible for program fd array, but
> bpf_fd_array_map_update_elem() has already been called above to handle
> program fd array and there is no rcu_read_lock() being acquired.
ahh. right. That explains why we don't have a splat now. good.
> >
> > 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.
>
> Yes. There is no spin-lock support in fd htab, the update operation for
> fd htab is taken under bucket lock. So the RCU CS is only to make the
> WARN_ON_ONCE() in htab_map_update_elem() happy.
>
> To ensure I fully understand what you mean, let me rephrase the things
> that need to done:
> 1) Repost v3 based on v1
> 2) In v3, add comments in bpf_fd_htab_map_update_elem() to explain why
> the RCU CS is needed. Is that correct ?
Yes.
Thanks
