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).

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@freebox.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

> 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@freebox.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
> 
>