[PATCH bpf-next v11 5/8] bpf: Copy map value using copy_map_value_long for percpu_cgroup_storage maps

Leon Hwang posted 8 patches 6 days, 9 hours ago
There is a newer version of this series
[PATCH bpf-next v11 5/8] bpf: Copy map value using copy_map_value_long for percpu_cgroup_storage maps
Posted by Leon Hwang 6 days, 9 hours ago
Copy map value using 'copy_map_value_long()'. It's to keep consistent
style with the way of other percpu maps.

No functional change intended.

Signed-off-by: Leon Hwang <leon.hwang@linux.dev>
---
 kernel/bpf/local_storage.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/kernel/bpf/local_storage.c b/kernel/bpf/local_storage.c
index c93a756e035c..2ab4b60ffe61 100644
--- a/kernel/bpf/local_storage.c
+++ b/kernel/bpf/local_storage.c
@@ -200,8 +200,7 @@ int bpf_percpu_cgroup_storage_copy(struct bpf_map *_map, void *key,
 	 */
 	size = round_up(_map->value_size, 8);
 	for_each_possible_cpu(cpu) {
-		bpf_long_memcpy(value + off,
-				per_cpu_ptr(storage->percpu_buf, cpu), size);
+		copy_map_value_long(_map, value + off, per_cpu_ptr(storage->percpu_buf, cpu));
 		off += size;
 	}
 	rcu_read_unlock();
@@ -234,8 +233,7 @@ int bpf_percpu_cgroup_storage_update(struct bpf_map *_map, void *key,
 	 */
 	size = round_up(_map->value_size, 8);
 	for_each_possible_cpu(cpu) {
-		bpf_long_memcpy(per_cpu_ptr(storage->percpu_buf, cpu),
-				value + off, size);
+		copy_map_value_long(_map, per_cpu_ptr(storage->percpu_buf, cpu), value + off);
 		off += size;
 	}
 	rcu_read_unlock();
-- 
2.51.2
Re: [PATCH bpf-next v11 5/8] bpf: Copy map value using copy_map_value_long for percpu_cgroup_storage maps
Posted by bot+bpf-ci@kernel.org 6 days, 8 hours ago
> diff --git a/kernel/bpf/local_storage.c b/kernel/bpf/local_storage.c
> index c93a756e0..2ab4b60ff 100644
> --- a/kernel/bpf/local_storage.c
> +++ b/kernel/bpf/local_storage.c
> @@ -200,8 +200,7 @@ int bpf_percpu_cgroup_storage_copy(struct bpf_map *_map, void *key,
>  	 */
>  	size = round_up(_map->value_size, 8);
>  	for_each_possible_cpu(cpu) {
> -		bpf_long_memcpy(value + off,
> -				per_cpu_ptr(storage->percpu_buf, cpu), size);
> +		copy_map_value_long(_map, value + off, per_cpu_ptr(storage->percpu_buf, cpu));
>  		off += size;
>  	}

Should this be calling check_and_init_map_value() after the copy,
similar to how it's done in other percpu maps? For example, in
bpf_percpu_array_copy():

    for_each_possible_cpu(cpu) {
        copy_map_value_long(map, value + off, per_cpu_ptr(pptr, cpu));
        check_and_init_map_value(map, value + off);
        off += size;
    }

And in percpu_hash lookup:

    for_each_possible_cpu(cpu) {
        copy_map_value_long(&htab->map, value + off, per_cpu_ptr(pptr, cpu));
        check_and_init_map_value(&htab->map, value + off);
        off += roundup_value_size;
    }

When copy_map_value_long() skips special fields like bpf_spin_lock or
bpf_timer (which happens when map->record is non-NULL), those fields
are left uninitialized in the destination buffer. The check_and_init_
map_value() call initializes these special fields via bpf_obj_init().

While BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE is deprecated and existing
users don't appear to use special fields, the inconsistency with other
percpu map implementations could be confusing. Also, the commit message
states "No functional change intended", but copy_map_value_long()
behaves differently from bpf_long_memcpy() when map->record is
non-NULL.

> @@ -234,8 +233,7 @@ int bpf_percpu_cgroup_storage_update(struct bpf_map *_map, void *key,
>  	 */
>  	size = round_up(_map->value_size, 8);
>  	for_each_possible_cpu(cpu) {
> -		bpf_long_memcpy(per_cpu_ptr(storage->percpu_buf, cpu),
> -				value + off, size);
> +		copy_map_value_long(_map, per_cpu_ptr(storage->percpu_buf, cpu), value + off);
>  		off += size;
>  	}

Same question here for the update path.


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/19674301388