Re: [PATCH net v2 2/2] eth: fbnic: return the page reference taken for a dropped Rx fragment

Re: [PATCH net v2 2/2] eth: fbnic: return the page reference taken for a dropped Rx fragment

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 (1) · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] A device-supplied buffer ID that is in range but names a slot
  holding no buffer still causes a NULL netmem dereference.

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but a second
  header completion leaks pages.

--

eth: fbnic: return the page reference taken for a dropped Rx fragment

When xdp_buff_add_frag() fails in fbnic_add_rx_frag(), this patch restores
the sub1 rx_buf pagecnt_bias. The page reference that
fbnic_page_pool_get_data() took for a fragment past MAX_SKB_FRAGS is then
returned to the page pool by fbnic_page_pool_drain().

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
> index b4ca9f7e8bb2e..ba663fe7598f1 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
> @@ -1076,6 +1076,8 @@ static void fbnic_add_rx_frag(struct fbnic_napi_vector *nv, u64 rcd,
>  
>  	added = xdp_buff_add_frag(&pkt->buff, netmem, pg_off, len, truesize);
>  	if (unlikely(!added)) {
> +		/* Give back the reference fbnic_page_pool_get_data() took */
> +		qt->sub1.rx_buf[pg_idx].pagecnt_bias++;
>  		pkt->add_frag_failed = true;
>  		netdev_err_once(nv->napi.dev,
>  				"Failed to add fragment to xdp_buff\n");

[Severity: High]
The new line runs only after netmem has already been used. That raises a
question about the buffer ID check earlier in this function.

The previous patch in this series, "eth: fbnic: validate the Rx completion
buffer index", added only an upper bound check in fbnic_add_rx_frag():

	if (unlikely(pg_idx > qt->sub1.size_mask)) {
		pkt->add_frag_failed = true;
		return;
	}

	netmem = fbnic_page_pool_get_data(qt, pg_idx);

fbnic_pkt_prepare() has the same kind of check on hdr_pg_idx against
qt->sub0.size_mask. Its comment says:

	/* A buffer ID the device never got from us cannot name a buffer we
	 * own, so drop the packet instead of running off the end of rx_buf[].
	 */

Can an in-range ID still name a slot that holds no buffer?

rx_buf[] comes from kvzalloc(), and fbnic_fill_bdq() only posts
fbnic_desc_unused() slots:

	return (ring->head - ring->tail - 1) & ring->size_mask;

So the slot at ring->tail is never posted. fbnic_page_pool_drain() also
clears every slot it drains:

	rx_buf->netmem = 0;

If a PAY_AL completion names sub1.tail, fbnic_page_pool_get_data() returns
0. Would the dereference of NULL then happen in NAPI context? It could
happen in page_pool_dma_sync_netmem_for_cpu(), or along this path:

xdp_buff_add_frag()->netmem_is_pfmemalloc()->page_is_pfmemalloc(NULL)

The HDR_AL case looks similar. If the completion names sub0.tail,
fbnic_page_pool_get_head() returns netmem_to_page(0), which is NULL. In
fbnic_pkt_prepare(), the dma_sync_single_range_for_cpu() call then passes
page_pool_get_dma_addr(page), which dereferences it.

fbnic_clean_bdq() also checks hw_head only against size_mask. Suppose
fbnic_fill_bdq() left extra slots unposted after a page_pool allocation
failure. Could an in-range hw_head past tail make the drain loop call
page_pool_unref_netmem(0, ...)? That would be the same null-ptr-deref that
the earlier commit message says it fixes.

An in-range ID that names a slot which was drained and refilled would
instead attach a recycled page_pool page. The device never wrote that page
for this packet.

Should the check also require rx_buf[idx].netmem to be non-zero, or idx to
fall inside the posted window between ring->head and ring->tail?

[Severity: Medium]
This is a pre-existing issue and was not introduced by this patch. It is a
separate path from the xdp_buff_add_frag() failure fixed here. Can a second
header completion leak the pages of the packet that is still pending?

fbnic_clean_rcq() calls fbnic_pkt_prepare() for every
FBNIC_RCD_TYPE_HDR_AL completion. Only the FBNIC_RCD_TYPE_META case sets
pkt->buff.data_hard_start back to NULL. fbnic_pkt_prepare() warns about a
pending packet but carries on:

	/* data_hard_start should always be NULL when this is called */
	WARN_ON_ONCE(pkt->buff.data_hard_start);
	...
	page = fbnic_page_pool_get_head(qt, hdr_pg_idx);
	...
	xdp_init_buff(&pkt->buff, frame_sz, &qt->xdp_rxq);
	...
	xdp_prepare_buff(&pkt->buff, hdr_start, headroom,
			 len - FBNIC_RX_PAD, true);
	...
	pkt->add_frag_failed = false;

Say the device sends HDR_AL, then some PAY_AL, then a second HDR_AL with no
META in between. The second HDR_AL has these effects:

- xdp_init_buff() clears XDP_FLAGS_HAS_FRAGS.
- xdp_prepare_buff() replaces data_hard_start. This also moves the shinfo
  that xdp_get_shared_info_from_buff() finds.
- add_frag_failed is reset.

At the next META, fbnic_put_pkt_buff() only releases what the current
pkt->buff describes. The earlier head page and all sub1 fragments already
attached to it can no longer be reached. That includes fragments of a
packet that already hit the MAX_SKB_FRAGS failure handled in this patch.

Each of those references was already subtracted from pagecnt_bias. Does
this leave fbnic_page_pool_drain() one short for each lost reference? If
so, the pages never return to the pool and are still counted as inflight
when the pool is destroyed.

The warn-and-continue logic seems to date back to a29b8eb6e533 ("eth:
fbnic: Add basic Rx handling"). The shinfo frag tracking came in with
61f9a066c309 ("eth: fbnic: Use shinfo to track frags state on Rx").

One option would be to call fbnic_put_pkt_buff() on a stale pending packet
before the new one is prepared. That would be a separate change from this
one.

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

[PATCH net v2 2/2] eth: fbnic: return the page reference taken for a dropped Rx fragment

[Yehyeong Lee]

fbnic_add_rx_frag() takes a page reference through
fbnic_page_pool_get_data(), which decrements the buffer's pagecnt_bias,
before handing the page to xdp_buff_add_frag().  xdp_buff_add_frag()
fails when the buffer already holds MAX_SKB_FRAGS fragments, and on that
path the reference is lost: add_frag_failed is set and the frame is
dropped, but the bias is not restored and fbnic_put_pkt_buff() only
returns the fragments that were actually attached.

The page is then never returned to the page pool.  A device that keeps
sending payload completions for one packet past MAX_SKB_FRAGS leaks a
page for every completion beyond the limit.

Restore pagecnt_bias on the failure path, the exact inverse of the get,
so fbnic_page_pool_drain() reclaims the page.

Fixes: 61f9a066c309 ("eth: fbnic: Use shinfo to track frags state on Rx")
Cc: stable@vger.kernel.org
Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
---
 drivers/net/ethernet/meta/fbnic/fbnic_txrx.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
index b4ca9f7e8bb2e..ba663fe7598f1 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
@@ -1076,6 +1076,8 @@ static void fbnic_add_rx_frag(struct fbnic_napi_vector *nv, u64 rcd,
 
 	added = xdp_buff_add_frag(&pkt->buff, netmem, pg_off, len, truesize);
 	if (unlikely(!added)) {
+		/* Give back the reference fbnic_page_pool_get_data() took */
+		qt->sub1.rx_buf[pg_idx].pagecnt_bias++;
 		pkt->add_frag_failed = true;
 		netdev_err_once(nv->napi.dev,
 				"Failed to add fragment to xdp_buff\n");
-- 
2.43.0