Re: [PATCH bpf-next 1/2] bpf: Limit local storage value size to max kmalloc_nolock() allocation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] bpf: non-standard multi-line comment style in local storage files
--

commit bbb69a62fc0fe47ec15aa691029d15c86b6ea57a
Author: Jonathan Bastari Thamrin <thamrinjonathan@gmail.com>

bpf: Limit local storage value size to max kmalloc_nolock() allocation

This commit updates BPF_LOCAL_STORAGE_MAX_VALUE_SIZE to be bounded by
KMALLOC_MAX_CACHE_SIZE so that local storage maps using kmalloc_nolock()
reject oversized values with E2BIG at map creation time. It also introduces
BPF_CGROUP_STORAGE_MAX_VALUE_SIZE to preserve the previous limit for
BPF_MAP_TYPE_CGROUP_STORAGE, which allocates elements with kmalloc().

Link: https://lore.kernel.org/bpf/CAADnVQKeFF--bgnZZSU12UY0muuwYA=7EdzLyOi837oZs+bXTA@mail.gmail.com/

> diff --git a/include/linux/bpf_local_storage.h b/include/linux/bpf_local_storage.h
> index 6c643ac510799..e1ce75b0fb0ac 100644
> --- a/include/linux/bpf_local_storage.h
> +++ b/include/linux/bpf_local_storage.h
> @@ -117,13 +117,13 @@ bpf_local_storage_unpin_owner(struct bpf_local_storage *local_storage)
>  	last = refcount_dec_and_test(&local_storage->owner_refcnt);
>  }
>  
> -/* U16_MAX is much more than enough for sk local storage
> - * considering a tcp_sock is ~2k.
> +/* Elements come from kmalloc_nolock() where the max size
> + * is KMALLOC_MAX_CACHE_SIZE.
>   */
> -#define BPF_LOCAL_STORAGE_MAX_VALUE_SIZE				       \
> +#define BPF_LOCAL_STORAGE_MAX_VALUE_SIZE                                       \
>  	min_t(u32,                                                             \
> -	      (KMALLOC_MAX_SIZE - MAX_BPF_STACK -                              \
> -	       sizeof(struct bpf_local_storage_elem)),                         \
> +	      (KMALLOC_MAX_CACHE_SIZE -                                        \
> +	       offsetof(struct bpf_local_storage_elem, sdata.data)),           \
>  	      (U16_MAX - sizeof(struct bpf_local_storage_elem)))
>  
>  #define SELEM(_SDATA)                                                          \
> diff --git a/kernel/bpf/local_storage.c b/kernel/bpf/local_storage.c
> index 83cd527a2542b..22a9841d4fd55 100644
> --- a/kernel/bpf/local_storage.c
> +++ b/kernel/bpf/local_storage.c
> @@ -18,6 +18,15 @@
>  #define LOCAL_STORAGE_CREATE_FLAG_MASK					\
>  	(BPF_F_NUMA_NODE | BPF_F_ACCESS_MASK)
>  
> +/* Cgroup storage allocates with kmalloc() where the max size is
> + * larger than kmalloc_nolock() used by other local storages.
> + */
> +#define BPF_CGROUP_STORAGE_MAX_VALUE_SIZE                                      \
> +	min_t(u32,                                                             \
> +	      (KMALLOC_MAX_SIZE - MAX_BPF_STACK -                              \
> +	       sizeof(struct bpf_local_storage_elem)),                         \
> +	      (U16_MAX - sizeof(struct bpf_local_storage_elem)))
> +
>  struct bpf_cgroup_storage_map {
>  	struct bpf_map map;
>  
> @@ -296,12 +305,12 @@ static int cgroup_storage_get_next_key(struct bpf_map *_map, void *key,
>  
>  static struct bpf_map *cgroup_storage_map_alloc(union bpf_attr *attr)
>  {
> -	__u32 max_value_size = BPF_LOCAL_STORAGE_MAX_VALUE_SIZE;
> +	__u32 max_value_size = BPF_CGROUP_STORAGE_MAX_VALUE_SIZE;
>  	int numa_node = bpf_map_attr_numa_node(attr);
>  	struct bpf_cgroup_storage_map *map;
>  
>  	/* percpu is bound by PCPU_MIN_UNIT_SIZE, non-percu
> -	 * is the same as other local storages.
> +	 * uses the cgroup storage limit above
>  	 */

[Severity: Low]
This isn't a bug, but in BPF code, multi-line comments place the opening /*
on its own line with the comment text starting on the following line. Could
the multi-line comments above BPF_LOCAL_STORAGE_MAX_VALUE_SIZE in
include/linux/bpf_local_storage.h, as well as above
BPF_CGROUP_STORAGE_MAX_VALUE_SIZE and in cgroup_storage_map_alloc() in
kernel/bpf/local_storage.c, be formatted to follow this style?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104-local-storage-max-v1-0-c836731bd184@gmail.com?part=1