Hi,
On 12/10/2023 1:36 PM, Alexei Starovoitov wrote:
> On Fri, Dec 8, 2023 at 2:32 AM 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.
>>
>> For bpf_fd_array_map_update_elem(), accessing array->ptrs doesn't need
>> rcu-read-lock because array->ptrs will not be freed until the map-in-map
>> is released. For bpf_fd_htab_map_update_elem(), htab_map_update_elem()
>> requires rcu-read-lock to be held, so only use rcu_read_lock() during
>> the invocation of htab_map_update_elem().
>>
>> Signed-off-by: Hou Tao <houtao1@xxxxxxxxxx>
>> ---
>> kernel/bpf/hashtab.c | 2 ++
>> kernel/bpf/syscall.c | 4 ----
>> 2 files changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
>> index b777bd8d4f8d..50b539c11b29 100644
>> --- a/kernel/bpf/hashtab.c
>> +++ b/kernel/bpf/hashtab.c
>> @@ -2525,7 +2525,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 6b9d7990d95f..fd9b73e02c7a 100644
>> --- a/kernel/bpf/syscall.c
>> +++ b/kernel/bpf/syscall.c
>> @@ -190,15 +190,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();
> I feel it's inconsistent to treat an array of FDs differently than
> hashmap of FDs.
> The patch is correct, but the users shouldn't be exposed
> to array vs hashtab implementation details.
I see. Will add rcu_read_lock/rcu_read_unlock() in
bpf_fd_array_map_update_elem() as well, so fd array will be consistent
with fd htab.
