Re: [PATCH net-next v5 7/7] net: skb: isolate skb data area allocations into a separate bucket

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The block comment above kmalloc_reserve() in net/core/skbuff.c
  (line 603) still says "kmalloc_reserve is a wrapper around…

--

net: skb: isolate skb data area allocations into a separate bucket

This patch moves skb data area allocations in kmalloc_reserve() and
kmalloc_pfmemalloc() off the general kmalloc caches and onto a dedicated
kmem_buckets set named "skb_data". The set is created in skb_init() with
both the normal and the accounted kmalloc types, so GFP_KERNEL_ACCOUNT
users such as AF_UNIX are isolated as well.

Link: https://github.com/google/security-research/blob/master/pocs/linux/kernelctf/CVE-2023-4207_lts_cos_mitigation_2/docs/exploit.md [0]

> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 4aea06d5167d1..a83689661fc28 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c

[ ... ]

> @@ -593,7 +595,8 @@ static void *kmalloc_pfmemalloc(size_t obj_size, gfp_t flags, int node)
>  	if (!obj_size)
>  		return kmem_cache_alloc_node(net_hotdata.skb_small_head_cache,
>  					     flags, node);
> -	return kmalloc_node_track_caller(obj_size, flags, node);
> +	return kmem_buckets_alloc_node_track_caller(skb_data_buckets, obj_size,
> +						    flags, node);
>  }
>  
>  /*

[Severity: Low]
This isn't a bug, but the block comment that follows here, above
kmalloc_reserve() in net/core/skbuff.c, still says:

 * kmalloc_reserve is a wrapper around kmalloc_node_track_caller that tells
 * the caller if emergency pfmemalloc reserves are being used. ...

After this patch, neither kmalloc_reserve() nor kmalloc_pfmemalloc() calls
kmalloc_node_track_caller(). Both now use
kmem_buckets_alloc_node_track_caller(skb_data_buckets, ...).

Should the comment be updated to name the bucket allocator? As it stands,
it points readers at the general kmalloc caches instead of the skb_data
caches.

> @@ -634,7 +637,7 @@ static void *kmalloc_reserve(unsigned int *size, gfp_t flags, int node,
>  	 * Try a regular allocation, when that fails and we're not entitled
>  	 * to the reserves, fail.
>  	 */
> -	obj = kmalloc_node_track_caller(obj_size,
> +	obj = kmem_buckets_alloc_node_track_caller(skb_data_buckets, obj_size,
>  					flags | __GFP_NOMEMALLOC | __GFP_NOWARN,
>  					node);
>  	if (likely(obj))

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261002231120.late.500-kees%40kernel.org