Re: [PATCH net v2 7/8] xsk: fix xsk_addrs slab leak on multi-buffer error path

[Stanislav Fomichev <sdf.kernel@gmail.com>]

On 04/20, Jason Xing wrote:
> From: Jason Xing <kernelxing@tencent.com>
> 
> When xsk_build_skb() / xsk_build_skb_zerocopy() sees the first
> continuation descriptor, it promotes destructor_arg from an inlined
> address to a freshly allocated xsk_addrs (num_descs = 1). The counter
> is bumped to >= 2 only at the very end of a successful build (by calling
> xsk_inc_num_desc()).
> 
> If the build fails in between (e.g. alloc_page() returns NULL with
> -EAGAIN, or the MAX_SKB_FRAGS overflow hits), we jump to free_err, skip
> calling xsk_inc_num_desc() to increment num_descs and leave the half-built
> skb attached to xs->skb for the app to retry. The skb now has
> 1) destructor_arg = a real xsk_addrs pointer,
> 2) num_descs = 1
> 
> If the app never retries and just close()s the socket, xsk_release()
> calls xsk_drop_skb() -> xsk_consume_skb(), which decides whether to
> free xsk_addrs by testing num_descs > 1:
> 
>     if (unlikely(num_descs > 1))
>         kmem_cache_free(xsk_tx_generic_cache, destructor_arg);
> 
> Because num_descs is exactly 1 the branch is skipped and the
> xsk_addrs object is leaked to the xsk_tx_generic_cache slab.
> 
> Fix it by directly testing if destructor_arg is still addr. Or else it
> is modified and used to store the newly allocated memory from
> xsk_tx_generic_cache regardless of increment of num_desc, which we
> need to handle.
> 
> Closes: https://lore.kernel.org/all/20260419045824.D9E5EC2BCAF@smtp.kernel.org/
> Fixes: 0ebc27a4c67d ("xsk: avoid data corruption on cq descriptor number")
> Signed-off-by: Jason Xing <kernelxing@tencent.com>
> ---
>  net/xdp/xsk.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> index 9236ec32b54a..6b17974ca825 100644
> --- a/net/xdp/xsk.c
> +++ b/net/xdp/xsk.c
> @@ -605,7 +605,7 @@ static void xsk_cq_submit_addr_locked(struct xsk_buff_pool *pool,
>  	spin_lock_irqsave(&pool->cq_prod_lock, flags);
>  	idx = xskq_get_prod(pool->cq);
>  
> -	if (unlikely(num_descs > 1)) {
> +	if (unlikely(!xsk_skb_destructor_is_addr(skb))) {
>  		xsk_addr = (struct xsk_addrs *)skb_shinfo(skb)->destructor_arg;
>  
>  		for (i = 0; i < num_descs; i++) {
> @@ -660,7 +660,7 @@ static void xsk_consume_skb(struct sk_buff *skb)
>  	u32 num_descs = xsk_get_num_desc(skb);
>  	struct xsk_addrs *xsk_addr;
>  
> -	if (unlikely(num_descs > 1)) {
> +	if (unlikely(!xsk_skb_destructor_is_addr(skb))) {
>  		xsk_addr = (struct xsk_addrs *)skb_shinfo(skb)->destructor_arg;
>  		kmem_cache_free(xsk_tx_generic_cache, xsk_addr);
>  	}
> -- 
> 2.41.3
> 

Acked-by: Stanislav Fomichev <sdf@fomichev.me>