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@xxxxxxxxxxxxxxx
> Signed-off-by: Yehyeong Lee <yhlee@xxxxxxxxxxxxxxxxxx>
> ---
>   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.