Re: [PATCH net v3 3/5] eth: fbnic: return a pending Rx packet before reusing its buffer

Re: [PATCH net v3 3/5] eth: fbnic: return a pending Rx packet before reusing its buffer

[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 patch adds a second place where an Rx packet gets thrown
  away, and that path is not counted in any statistic.

--

eth: fbnic: return a pending Rx packet before reusing its buffer

This patch changes fbnic_pkt_prepare() so it no longer only warns when a
new header completion arrives while a packet is still pending. It now
returns the pending header page and any attached fragments to the page
pool, then clears data_hard_start. To make this possible,
fbnic_put_pkt_buff() is moved above fbnic_pkt_prepare().

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
> index 7dda59bba14ee..47ded7a9e0a81 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c

[ ... ]

> @@ -1025,8 +1052,12 @@ static void fbnic_pkt_prepare(struct fbnic_napi_vector *nv, u64 rcd,
>  	unsigned char *hdr_start;
>  	struct page *page;
>  
> -	/* data_hard_start should always be NULL when this is called */
> -	WARN_ON_ONCE(pkt->buff.data_hard_start);
> +	/* a new header while one is pending: release the old one */
> +	if (unlikely(pkt->buff.data_hard_start)) {
> +		WARN_ON_ONCE(1);
> +		fbnic_put_pkt_buff(qt, pkt, 1);
> +		pkt->buff.data_hard_start = NULL;
> +	}

[Severity: Low]

Should this new drop path also be counted in the Rx drop statistics?

Every other place that discards a packet in the META handling of
fbnic_clean_rcq() counts it before calling fbnic_put_pkt_buff(). That
includes a META completion that arrives with no header:

drivers/net/ethernet/meta/fbnic/fbnic_txrx.c:fbnic_clean_rcq() {
    ...
			} else {
				if (!skb)
					alloc_failed++;

				if (skb == ERR_PTR(-FBNIC_XDP_LEN_ERR))
					length_errors++;
				else
					dropped++;

				fbnic_put_pkt_buff(qt, pkt, 1);
				goto next_dont_count;
			}
    ...
}

Suppose the device sends HDR_AL(A), [PAY_AL ...], HDR_AL(B), ..., META.
Packet A is then freed here and nothing increments dropped, length_errors
or alloc_failed. So rcq->stats.dropped, rx_dropped and the per-queue stats
do not show it. Only the first such event per boot leaves a trace, through
WARN_ON_ONCE().

Before this patch the packet was also not counted, because it was leaked.
Now that it is dropped on purpose, should fbnic_pkt_prepare() update
rcq->stats (or report back to fbnic_clean_rcq() so it can bump dropped)?
That would match how the other drop paths are counted.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1791775409.git.yhlee%40isslab.korea.ac.kr

Re: [PATCH net v3 3/5] eth: fbnic: return a pending Rx packet before reusing its buffer

[Mohsin Bashir]

On 10/11/26 9:15 PM, Yehyeong Lee wrote:
> fbnic_pkt_prepare() starts a new packet on each header completion and so
> far only warned when pkt->buff was still populated from a previous header
> that no META completion had finished. The stale buffer, and any fragments
> attached to it, were then overwritten and leaked.
> 
> Return the pending buffer to the page pool before starting over and clear
> data_hard_start, so the META drop path does not release it a second time.
> This leaks memory rather than crashing, so there is no oops to show.
> 
> Fixes: a29b8eb6e533 ("eth: fbnic: Add basic Rx handling")
> Cc: stable@vger.kernel.org
> Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
> ---
>   drivers/net/ethernet/meta/fbnic/fbnic_txrx.c | 62 +++++++++++---------
>   1 file changed, 33 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
> index 7dda59bba14ee..47ded7a9e0a81 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
> @@ -1014,6 +1014,33 @@ static unsigned int fbnic_hdr_pg_end(unsigned int pg_off, unsigned int len)
>   	return ALIGN(pg_off, 128) - FBNIC_RX_HROOM;
>   }
>   
> +static void fbnic_put_pkt_buff(struct fbnic_q_triad *qt,
> +			       struct fbnic_pkt_buff *pkt, int budget)
> +{
> +	struct page *page;
> +
> +	if (!pkt->buff.data_hard_start)
> +		return;
> +
> +	if (xdp_buff_has_frags(&pkt->buff)) {
> +		struct skb_shared_info *shinfo;
> +		netmem_ref netmem;
> +		int nr_frags;
> +
> +		shinfo = xdp_get_shared_info_from_buff(&pkt->buff);
> +		nr_frags = shinfo->nr_frags;
> +
> +		while (nr_frags--) {
> +			netmem = skb_frag_netmem(&shinfo->frags[nr_frags]);
> +			page_pool_put_full_netmem(qt->sub1.page_pool, netmem,
> +						  !!budget);
> +		}
> +	}
> +
> +	page = virt_to_page(pkt->buff.data_hard_start);
> +	page_pool_put_full_page(qt->sub0.page_pool, page, !!budget);
> +}
> +
>   static void fbnic_pkt_prepare(struct fbnic_napi_vector *nv, u64 rcd,
>   			      struct fbnic_pkt_buff *pkt,
>   			      struct fbnic_q_triad *qt)
> @@ -1025,8 +1052,12 @@ static void fbnic_pkt_prepare(struct fbnic_napi_vector *nv, u64 rcd,
>   	unsigned char *hdr_start;
>   	struct page *page;
>   
> -	/* data_hard_start should always be NULL when this is called */
> -	WARN_ON_ONCE(pkt->buff.data_hard_start);
> +	/* a new header while one is pending: release the old one */
> +	if (unlikely(pkt->buff.data_hard_start)) {
> +		WARN_ON_ONCE(1);
> +		fbnic_put_pkt_buff(qt, pkt, 1);
> +		pkt->buff.data_hard_start = NULL;
> +	}
>   
>   	/* A buffer ID we never posted cannot name a buffer we own, so drop
>   	 * the packet instead of running off the end of rx_buf[] or picking
> @@ -1146,33 +1177,6 @@ static void fbnic_add_rx_frag(struct fbnic_napi_vector *nv, u64 rcd,
>   	}
>   }
>   
> -static void fbnic_put_pkt_buff(struct fbnic_q_triad *qt,
> -			       struct fbnic_pkt_buff *pkt, int budget)
> -{
> -	struct page *page;
> -
> -	if (!pkt->buff.data_hard_start)
> -		return;
> -
> -	if (xdp_buff_has_frags(&pkt->buff)) {
> -		struct skb_shared_info *shinfo;
> -		netmem_ref netmem;
> -		int nr_frags;
> -
> -		shinfo = xdp_get_shared_info_from_buff(&pkt->buff);
> -		nr_frags = shinfo->nr_frags;
> -
> -		while (nr_frags--) {
> -			netmem = skb_frag_netmem(&shinfo->frags[nr_frags]);
> -			page_pool_put_full_netmem(qt->sub1.page_pool, netmem,
> -						  !!budget);
> -		}
> -	}
> -
> -	page = virt_to_page(pkt->buff.data_hard_start);
> -	page_pool_put_full_page(qt->sub0.page_pool, page, !!budget);
> -}
> -
>   static struct sk_buff *fbnic_build_skb(struct fbnic_napi_vector *nv,
>   				       struct fbnic_pkt_buff *pkt)
>   {

Under normal operation, the completion sequence is HDR_AL, followed by 
any PAY_AL completions, optional META, and primary META. This patch 
assumes that a new HDR_AL arrives before the previous packet’s primary 
META, violating that sequence.

[PATCH net v3 3/5] eth: fbnic: return a pending Rx packet before reusing its buffer

[Yehyeong Lee]

fbnic_pkt_prepare() starts a new packet on each header completion and so
far only warned when pkt->buff was still populated from a previous header
that no META completion had finished. The stale buffer, and any fragments
attached to it, were then overwritten and leaked.

Return the pending buffer to the page pool before starting over and clear
data_hard_start, so the META drop path does not release it a second time.
This leaks memory rather than crashing, so there is no oops to show.

Fixes: a29b8eb6e533 ("eth: fbnic: Add basic Rx handling")
Cc: stable@vger.kernel.org
Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
---
 drivers/net/ethernet/meta/fbnic/fbnic_txrx.c | 62 +++++++++++---------
 1 file changed, 33 insertions(+), 29 deletions(-)

diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
index 7dda59bba14ee..47ded7a9e0a81 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
@@ -1014,6 +1014,33 @@ static unsigned int fbnic_hdr_pg_end(unsigned int pg_off, unsigned int len)
 	return ALIGN(pg_off, 128) - FBNIC_RX_HROOM;
 }
 
+static void fbnic_put_pkt_buff(struct fbnic_q_triad *qt,
+			       struct fbnic_pkt_buff *pkt, int budget)
+{
+	struct page *page;
+
+	if (!pkt->buff.data_hard_start)
+		return;
+
+	if (xdp_buff_has_frags(&pkt->buff)) {
+		struct skb_shared_info *shinfo;
+		netmem_ref netmem;
+		int nr_frags;
+
+		shinfo = xdp_get_shared_info_from_buff(&pkt->buff);
+		nr_frags = shinfo->nr_frags;
+
+		while (nr_frags--) {
+			netmem = skb_frag_netmem(&shinfo->frags[nr_frags]);
+			page_pool_put_full_netmem(qt->sub1.page_pool, netmem,
+						  !!budget);
+		}
+	}
+
+	page = virt_to_page(pkt->buff.data_hard_start);
+	page_pool_put_full_page(qt->sub0.page_pool, page, !!budget);
+}
+
 static void fbnic_pkt_prepare(struct fbnic_napi_vector *nv, u64 rcd,
 			      struct fbnic_pkt_buff *pkt,
 			      struct fbnic_q_triad *qt)
@@ -1025,8 +1052,12 @@ static void fbnic_pkt_prepare(struct fbnic_napi_vector *nv, u64 rcd,
 	unsigned char *hdr_start;
 	struct page *page;
 
-	/* data_hard_start should always be NULL when this is called */
-	WARN_ON_ONCE(pkt->buff.data_hard_start);
+	/* a new header while one is pending: release the old one */
+	if (unlikely(pkt->buff.data_hard_start)) {
+		WARN_ON_ONCE(1);
+		fbnic_put_pkt_buff(qt, pkt, 1);
+		pkt->buff.data_hard_start = NULL;
+	}
 
 	/* A buffer ID we never posted cannot name a buffer we own, so drop
 	 * the packet instead of running off the end of rx_buf[] or picking
@@ -1146,33 +1177,6 @@ static void fbnic_add_rx_frag(struct fbnic_napi_vector *nv, u64 rcd,
 	}
 }
 
-static void fbnic_put_pkt_buff(struct fbnic_q_triad *qt,
-			       struct fbnic_pkt_buff *pkt, int budget)
-{
-	struct page *page;
-
-	if (!pkt->buff.data_hard_start)
-		return;
-
-	if (xdp_buff_has_frags(&pkt->buff)) {
-		struct skb_shared_info *shinfo;
-		netmem_ref netmem;
-		int nr_frags;
-
-		shinfo = xdp_get_shared_info_from_buff(&pkt->buff);
-		nr_frags = shinfo->nr_frags;
-
-		while (nr_frags--) {
-			netmem = skb_frag_netmem(&shinfo->frags[nr_frags]);
-			page_pool_put_full_netmem(qt->sub1.page_pool, netmem,
-						  !!budget);
-		}
-	}
-
-	page = virt_to_page(pkt->buff.data_hard_start);
-	page_pool_put_full_page(qt->sub0.page_pool, page, !!budget);
-}
-
 static struct sk_buff *fbnic_build_skb(struct fbnic_napi_vector *nv,
 				       struct fbnic_pkt_buff *pkt)
 {
-- 
2.43.0