linux-kernel - Re: [REPOST PATCH] drivers: net: mvpp2: attempt to refill rx before allocating skb

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <aDVvR8XXPMEUzztC@lore-desk>
Date: Tue, 27 May 2025 09:52:39 +0200
From: Lorenzo Bianconi <lorenzo.bianconi@...hat.com>
To: Marios Makassikis <mmakassikis@...ebox.fr>
Cc: netdev@...r.kernel.org, lorenzo@...nel.org, mcroce@...rosoft.com,
	marcin.s.wojtas@...il.com, linux@...linux.org.uk,
	andrew+netdev@...n.ch, davem@...emloft.net, edumazet@...gle.com,
	kuba@...nel.org, pabeni@...hat.com, linux-kernel@...r.kernel.org
Subject: Re: [REPOST PATCH] drivers: net: mvpp2: attempt to refill rx before
 allocating skb

> on mvpp2_rx_refill() failure, the freshly allocated skb is freed,
> the rx error counter is incremented and the descriptor currently
> being processed is rearmed through mvpp2_bm_pool_put().
> 
> the logic is that the system is low on memory so it's not possible
> to allocate both a rx descriptor and an skb, so we might as well
> drop the skb and return the descriptor to the rx pool to avoid
> draining it (and preventing any future packet reception).

Hi Marios,

Can we just run the mvpp2_rx_refill() when the skb has been already
successfully sent to the networking stack? Doing so, if the skb allocation
fails, we will refill the in-flight rx descriptor (the one consumed by the
skb) to the descriptor ring, reducing the pressure on it.
What do you think?

Regards,
Lorenzo

> 
> the skb freeing is unfortunate, as build_skb() takes ownership
> of the 'data' buffer:
>  - build_skb() calls  __finalize_skb_around() which sets skb->head
>  and skb->data to point to 'data'
>  - dev_free_skb_any() may call skb_free_frag() on skb->head
> 
> thus, the final mvpp2_bm_pool_put() rearms a descriptor that was
> just freed.
> 
> call mvpp2_rx_refill() first, so there's no skb to free.
> 
> incidentally, doing rx refill prior to skb allocation is what is
> done in marvell's mvneta driver for armada 370 (mvneta_rx_hwbm() in
> mvneta.c)
> 
> Fixes: d6526926de739 ("net: mvpp2: fix memory leak in mvpp2_rx")
> Signed-off-by: Marios Makassikis <mmakassikis@...ebox.fr>
> ---
>  drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> index 416a926a8281..e13055ec4483 100644
> --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> @@ -4003,6 +4003,12 @@ static int mvpp2_rx(struct mvpp2_port *port, struct napi_struct *napi,
>  			metasize = xdp.data - xdp.data_meta;
>  		}
>  
> +		err = mvpp2_rx_refill(port, bm_pool, pp, pool);
> +		if (err) {
> +			netdev_err(port->dev, "failed to refill BM pools\n");
> +			goto err_drop_frame;
> +		}
> +
>  		if (frag_size)
>  			skb = build_skb(data, frag_size);
>  		else
> @@ -4021,13 +4027,6 @@ static int mvpp2_rx(struct mvpp2_port *port, struct napi_struct *napi,
>  					 skb_hwtstamps(skb));
>  		}
>  
> -		err = mvpp2_rx_refill(port, bm_pool, pp, pool);
> -		if (err) {
> -			netdev_err(port->dev, "failed to refill BM pools\n");
> -			dev_kfree_skb_any(skb);
> -			goto err_drop_frame;
> -		}
> -
>  		if (pp)
>  			skb_mark_for_recycle(skb);
>  		else
> -- 
> 2.49.0
> 
> 

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)