[PATCH RESEND bpf-next 0/2] bpf: Use GFP_KERNEL in bpf_event_entry_gen()

[PATCH RESEND bpf-next 0/2] bpf: Use GFP_KERNEL in bpf_event_entry_gen()

[Hou Tao]

From: Hou Tao <houtao1@huawei.com>

Hi,

The simple patch set aims to replace GFP_ATOMIC in bpf_event_entry_gen().
These two patches in the patchset were preparatory patches in "Fix the
release of inner map" patchset [1] and are not needed for v2, so re-post
it to bpf-next tree.

Patch #1 reduces the scope of rcu_read_lock when updating fd map and
patch #2 replaces GFP_ATOMIC by GFP_KERNEL. Please see individual
patches for more details.

Comments are always welcome.

Regards,
Tao

[1]: https://lore.kernel.org/bpf/20231107140702.1891778-1-houtao@huaweicloud.com


Hou Tao (2):
  bpf: Reduce the scope of rcu_read_lock when updating fd map
  bpf: Use GFP_KERNEL in bpf_event_entry_gen()

 kernel/bpf/arraymap.c | 2 +-
 kernel/bpf/hashtab.c  | 2 ++
 kernel/bpf/syscall.c  | 4 ----
 3 files changed, 3 insertions(+), 5 deletions(-)

-- 
2.29.2

[PATCH RESEND bpf-next 1/2] bpf: Reduce the scope of rcu_read_lock when updating fd map

[Hou Tao]

From: Hou Tao <houtao1@huawei.com>

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@huawei.com>
---
 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();
 	} else if (map->map_type == BPF_MAP_TYPE_REUSEPORT_SOCKARRAY) {
 		/* rcu_read_lock() is not needed */
 		err = bpf_fd_reuseport_array_update_elem(map, key, value,
-- 
2.29.2

[PATCH RESEND bpf-next 2/2] bpf: Use GFP_KERNEL in bpf_event_entry_gen()

[Hou Tao]

From: Hou Tao <houtao1@huawei.com>

rcu_read_lock() is no longer held when invoking bpf_event_entry_gen()
which is called by perf_event_fd_array_get_ptr(), so using GFP_KERNEL
instead of GFP_ATOMIC to reduce the possibility of failures due to
out-of-memory.

Signed-off-by: Hou Tao <houtao1@huawei.com>
---
 kernel/bpf/arraymap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c
index 4a4a67956e21..bac7420b29c7 100644
--- a/kernel/bpf/arraymap.c
+++ b/kernel/bpf/arraymap.c
@@ -1195,7 +1195,7 @@ static struct bpf_event_entry *bpf_event_entry_gen(struct file *perf_file,
 {
 	struct bpf_event_entry *ee;
 
-	ee = kzalloc(sizeof(*ee), GFP_ATOMIC);
+	ee = kzalloc(sizeof(*ee), GFP_KERNEL);
 	if (ee) {
 		ee->event = perf_file->private_data;
 		ee->perf_file = perf_file;
-- 
2.29.2

Re: [PATCH RESEND bpf-next 1/2] bpf: Reduce the scope of rcu_read_lock when updating fd map

[Yonghong Song]

On 12/8/23 2:33 AM, Hou Tao wrote:
> From: Hou Tao <houtao1@huawei.com>
>
> 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@huawei.com>

Acked-by: Yonghong Song <yonghong.song@linux.dev>

Re: [PATCH RESEND bpf-next 2/2] bpf: Use GFP_KERNEL in bpf_event_entry_gen()

[Yonghong Song]

On 12/8/23 2:33 AM, Hou Tao wrote:
> From: Hou Tao <houtao1@huawei.com>
>
> rcu_read_lock() is no longer held when invoking bpf_event_entry_gen()
> which is called by perf_event_fd_array_get_ptr(), so using GFP_KERNEL
> instead of GFP_ATOMIC to reduce the possibility of failures due to
> out-of-memory.
>
> Signed-off-by: Hou Tao <houtao1@huawei.com>

Acked-by: Yonghong Song <yonghong.song@linux.dev>

Re: [PATCH RESEND bpf-next 1/2] bpf: Reduce the scope of rcu_read_lock when updating fd map

[Alexei Starovoitov]

On Fri, Dec 8, 2023 at 2:32 AM Hou Tao <houtao@huaweicloud.com> wrote:
>
> From: Hou Tao <houtao1@huawei.com>
>
> 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@huawei.com>
> ---
>  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.

Re: [PATCH RESEND bpf-next 1/2] bpf: Reduce the scope of rcu_read_lock when updating fd map

[Hou Tao]

Hi,

On 12/10/2023 1:36 PM, Alexei Starovoitov wrote:
> On Fri, Dec 8, 2023 at 2:32 AM Hou Tao <houtao@huaweicloud.com> wrote:
>> From: Hou Tao <houtao1@huawei.com>
>>
>> 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@huawei.com>
>> ---
>>  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.