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.
>
> 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 ?
>
> 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.
OK. Will remove.
>
> pw-bot: cr
