[PATCH net] eth: fbnic: validate the Rx completion buffer index

[PATCH net] eth: fbnic: validate the Rx completion buffer index

[Yehyeong Lee]

The buffer ID in an address/length Rx completion descriptor is a 16 bit
field, FBNIC_RCD_AL_BUFF_PAGE_MASK being DESC_GENMASK(15, 0) once the
fragment bits are removed, so the device can name any index from 0 to
65535.  fbnic_pkt_prepare() and fbnic_add_rx_frag() feed that value
straight into fbnic_page_pool_get_head() and fbnic_page_pool_get_data(),
which use it to subscript qt->sub0.rx_buf[] and qt->sub1.rx_buf[].

Those arrays only cover the buffers the driver posted.  They are
allocated with size_mask + 1 entries, and size_mask comes from
hpq_size / FBNIC_BD_FRAG_COUNT, so at the default FBNIC_HPQ_SIZE_DEFAULT
of 256 each array holds 256 entries of 16 bytes, that is 4096 bytes.  A
buffer ID of 65535 reaches 1048560 bytes past the start of the
allocation.

The fill side already masks every index it computes against
bdq->size_mask, and so does all of the ring head and tail arithmetic.
The completion side does not.  A descriptor naming a buffer that was
never posted makes fbnic_page_pool_get_head() decrement pagecnt_bias
through a pointer outside the array and hand the out-of-bounds
netmem_ref back as a struct page; page_pool_get_dma_addr() and
page_address() then dereference it and the skb is built on whatever it
points at.

Injecting an out-of-range header completion reports under KASAN:

  BUG: KASAN: slab-out-of-bounds in fbnic_page_pool_get_head
  Read of size 8 at addr ffff88800c719008
  The buggy address is located 8 bytes to the right of
   allocated 4096-byte region [ffff88800c718000, ffff88800c719000)

Check the ID against size_mask before it is used and drop the packet if
it is out of range.  Masking it instead would keep the access in bounds
but would silently attribute the completion to an unrelated buffer that
the driver does own, which is worse than a drop.  Set add_frag_failed so
the frame takes the existing error path.

Rejecting the header completion leaves pkt->buff uninitialized, and the
payload completions that follow are handled before the metadata
descriptor that looks at add_frag_failed, so fbnic_add_rx_frag() has to
bail out as well or xdp_buff_add_frag() writes through a shared info
pointer derived from a NULL data_hard_start.  Key that on
data_hard_start, the way fbnic_put_pkt_buff() already does, so the
buffer accounting on the normal path is untouched.

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 | 27 ++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
index 10caacffee0f0..d85daab06c99c 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
@@ -985,14 +985,24 @@ static void fbnic_pkt_prepare(struct fbnic_napi_vector *nv, u64 rcd,
 {
 	unsigned int hdr_pg_idx = FIELD_GET(FBNIC_RCD_AL_BUFF_PAGE_MASK, rcd);
 	unsigned int hdr_pg_off = FIELD_GET(FBNIC_RCD_AL_BUFF_OFF_MASK, rcd);
-	struct page *page = fbnic_page_pool_get_head(qt, hdr_pg_idx);
 	unsigned int len = FIELD_GET(FBNIC_RCD_AL_BUFF_LEN_MASK, rcd);
 	unsigned int frame_sz, hdr_pg_start, hdr_pg_end, headroom;
 	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 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[].
+	 */
+	if (unlikely(hdr_pg_idx > qt->sub0.size_mask)) {
+		pkt->add_frag_failed = true;
+		return;
+	}
+
+	page = fbnic_page_pool_get_head(qt, hdr_pg_idx);
+
 	/* Short-cut the end calculation if we know page is fully consumed */
 	hdr_pg_end = FIELD_GET(FBNIC_RCD_AL_PAGE_FIN, rcd) ?
 		     FBNIC_BD_FRAG_SIZE : fbnic_hdr_pg_end(hdr_pg_off, len);
@@ -1026,10 +1036,23 @@ static void fbnic_add_rx_frag(struct fbnic_napi_vector *nv, u64 rcd,
 	unsigned int pg_idx = FIELD_GET(FBNIC_RCD_AL_BUFF_PAGE_MASK, rcd);
 	unsigned int pg_off = FIELD_GET(FBNIC_RCD_AL_BUFF_OFF_MASK, rcd);
 	unsigned int len = FIELD_GET(FBNIC_RCD_AL_BUFF_LEN_MASK, rcd);
-	netmem_ref netmem = fbnic_page_pool_get_data(qt, pg_idx);
 	unsigned int truesize;
+	netmem_ref netmem;
 	bool added;
 
+	/* The header completion was rejected, so buff was never initialized
+	 * and add_frag_failed is already set.  There is nothing to add to.
+	 */
+	if (unlikely(!pkt->buff.data_hard_start))
+		return;
+
+	if (unlikely(pg_idx > qt->sub1.size_mask)) {
+		pkt->add_frag_failed = true;
+		return;
+	}
+
+	netmem = fbnic_page_pool_get_data(qt, pg_idx);
+
 	truesize = FIELD_GET(FBNIC_RCD_AL_PAGE_FIN, rcd) ?
 		   FBNIC_BD_FRAG_SIZE - pg_off : ALIGN(len, 128);
 
-- 
2.43.0

Re: [PATCH net] eth: fbnic: validate the Rx completion buffer index

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - What hardware the change was tested on. For driver fixes please
   mention the device (and if relevant firmware version) used for
   testing, or say that the change was not tested on real hardware.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH net] eth: fbnic: validate the Rx completion buffer index

[Daniel Zahka]

On Fri Oct 9, 2026 at 1:53 PM EDT, Yehyeong Lee wrote:
> The buffer ID in an address/length Rx completion descriptor is a 16 bit
> field, FBNIC_RCD_AL_BUFF_PAGE_MASK being DESC_GENMASK(15, 0) once the
> fragment bits are removed, so the device can name any index from 0 to
> 65535.  fbnic_pkt_prepare() and fbnic_add_rx_frag() feed that value
> straight into fbnic_page_pool_get_head() and fbnic_page_pool_get_data(),
> which use it to subscript qt->sub0.rx_buf[] and qt->sub1.rx_buf[].
>
> Those arrays only cover the buffers the driver posted.  They are
> allocated with size_mask + 1 entries, and size_mask comes from
> hpq_size / FBNIC_BD_FRAG_COUNT, so at the default FBNIC_HPQ_SIZE_DEFAULT
> of 256 each array holds 256 entries of 16 bytes, that is 4096 bytes.  A
> buffer ID of 65535 reaches 1048560 bytes past the start of the
> allocation.
>
> The fill side already masks every index it computes against
> bdq->size_mask, and so does all of the ring head and tail arithmetic.
> The completion side does not.  A descriptor naming a buffer that was
> never posted makes fbnic_page_pool_get_head() decrement pagecnt_bias
> through a pointer outside the array and hand the out-of-bounds
> netmem_ref back as a struct page; page_pool_get_dma_addr() and
> page_address() then dereference it and the skb is built on whatever it
> points at.
>
> Injecting an out-of-range header completion reports under KASAN:

How exactly are you injecting the completion? Just overwriting the id
field in the Rx completion with an arbitrary value from the driver?

The buffer id is chosen by SW when the BD is submitted to the device,
and the device just echoes it back like a cookie when that buffer is
used.

>
>   BUG: KASAN: slab-out-of-bounds in fbnic_page_pool_get_head
>   Read of size 8 at addr ffff88800c719008
>   The buggy address is located 8 bytes to the right of
>    allocated 4096-byte region [ffff88800c718000, ffff88800c719000)
>
> Check the ID against size_mask before it is used and drop the packet if
> it is out of range.  Masking it instead would keep the access in bounds
> but would silently attribute the completion to an unrelated buffer that
> the driver does own, which is worse than a drop.  Set add_frag_failed so
> the frame takes the existing error path.
>
> Rejecting the header completion leaves pkt->buff uninitialized, and the
> payload completions that follow are handled before the metadata
> descriptor that looks at add_frag_failed, so fbnic_add_rx_frag() has to
> bail out as well or xdp_buff_add_frag() writes through a shared info
> pointer derived from a NULL data_hard_start.  Key that on
> data_hard_start, the way fbnic_put_pkt_buff() already does, so the
> buffer accounting on the normal path is untouched.
>
> 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 | 27 ++++++++++++++++++--
>  1 file changed, 25 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
> index 10caacffee0f0..d85daab06c99c 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
> @@ -985,14 +985,24 @@ static void fbnic_pkt_prepare(struct fbnic_napi_vector *nv, u64 rcd,
>  {
>  	unsigned int hdr_pg_idx = FIELD_GET(FBNIC_RCD_AL_BUFF_PAGE_MASK, rcd);
>  	unsigned int hdr_pg_off = FIELD_GET(FBNIC_RCD_AL_BUFF_OFF_MASK, rcd);
> -	struct page *page = fbnic_page_pool_get_head(qt, hdr_pg_idx);
>  	unsigned int len = FIELD_GET(FBNIC_RCD_AL_BUFF_LEN_MASK, rcd);
>  	unsigned int frame_sz, hdr_pg_start, hdr_pg_end, headroom;
>  	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 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[].
> +	 */
> +	if (unlikely(hdr_pg_idx > qt->sub0.size_mask)) {
> +		pkt->add_frag_failed = true;
> +		return;
> +	}
> +
> +	page = fbnic_page_pool_get_head(qt, hdr_pg_idx);
> +
>  	/* Short-cut the end calculation if we know page is fully consumed */
>  	hdr_pg_end = FIELD_GET(FBNIC_RCD_AL_PAGE_FIN, rcd) ?
>  		     FBNIC_BD_FRAG_SIZE : fbnic_hdr_pg_end(hdr_pg_off, len);
> @@ -1026,10 +1036,23 @@ static void fbnic_add_rx_frag(struct fbnic_napi_vector *nv, u64 rcd,
>  	unsigned int pg_idx = FIELD_GET(FBNIC_RCD_AL_BUFF_PAGE_MASK, rcd);
>  	unsigned int pg_off = FIELD_GET(FBNIC_RCD_AL_BUFF_OFF_MASK, rcd);
>  	unsigned int len = FIELD_GET(FBNIC_RCD_AL_BUFF_LEN_MASK, rcd);
> -	netmem_ref netmem = fbnic_page_pool_get_data(qt, pg_idx);
>  	unsigned int truesize;
> +	netmem_ref netmem;
>  	bool added;
>  
> +	/* The header completion was rejected, so buff was never initialized
> +	 * and add_frag_failed is already set.  There is nothing to add to.
> +	 */
> +	if (unlikely(!pkt->buff.data_hard_start))
> +		return;
> +
> +	if (unlikely(pg_idx > qt->sub1.size_mask)) {
> +		pkt->add_frag_failed = true;
> +		return;
> +	}
> +
> +	netmem = fbnic_page_pool_get_data(qt, pg_idx);
> +
>  	truesize = FIELD_GET(FBNIC_RCD_AL_PAGE_FIN, rcd) ?
>  		   FBNIC_BD_FRAG_SIZE - pg_off : ALIGN(len, 128);
>  

Re: [PATCH net] eth: fbnic: validate the Rx completion buffer index

[Yehyeong Lee]

On Fri Oct 9, 2026 at 2:14 PM EDT, Daniel Zahka wrote:
> How exactly are you injecting the completion? Just overwriting the id
> field in the Rx completion with an arbitrary value from the driver?

Yes.  No emulated fbnic, so this isn't from real hardware: on a KASAN
build I ran the completion handlers with FBNIC_RCD_AL_BUFF_PAGE_MASK set
out of range and saw the out-of-bounds access, which the patch removes.
Found by reading the code, not from a report.

> The buffer id is chosen by SW when the BD is submitted to the device,
> and the device just echoes it back like a cookie when that buffer is
> used.

Agreed, a conforming device stays in range: fbnic_bd_prep() puts the
ring slot i, already masked to size_mask in fbnic_fill_bdq(), into
FBNIC_BD_PAGE_ID_MASK, and rx_buf[i] is that slot, so every id the
driver hands out is in range by construction.

The completion path is the one place that takes the id back and uses it
as the rx_buf[] subscript without bounding it.  A device that returns an
id it was never given then gets an out-of-bounds pagecnt_bias decrement
and an out-of-bounds netmem dereference: a device-triggered memory
safety bug, just not one a conforming device reaches.  The patch bounds
the index and drops the frame for one comparison per completion.

Re: [PATCH net] eth: fbnic: validate the Rx completion buffer index

[netdev-bot+sashiko]

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

Critical: 0 · High: 2 (1) · Medium: 2 · 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] The patch does not fix the injection case its commit message
  describes.
- [High] eth: fbnic: uninitialized buffer dereference when payload
  descriptor arrives without header descriptor
- [Medium] The new guards `hdr_pg_idx > qt->sub0.size_mask`
  (fbnic_pkt_prepare(), line 999) and `pg_idx > qt->sub1.size_mask`…
- [Medium] eth: fbnic: memory leak of netmem fragment when
  xdp_buff_add_frag() fails

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but it is a
  memory-corruption bug on the same malformed-completion path the patch…

--

eth: fbnic: validate the Rx completion buffer index

This patch checks the buffer ID from address/length Rx completions
against the BDQ size_mask in fbnic_pkt_prepare() and fbnic_add_rx_frag().
An out-of-range ID sets add_frag_failed so the frame is dropped.
fbnic_add_rx_frag() also returns early when the header completion was
rejected and pkt->buff.data_hard_start is still NULL.

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
> index 10caacffee0f0..d85daab06c99c 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
> @@ -985,14 +985,24 @@ static void fbnic_pkt_prepare(struct fbnic_napi_vector *nv, u64 rcd,
>  {
>  	unsigned int hdr_pg_idx = FIELD_GET(FBNIC_RCD_AL_BUFF_PAGE_MASK, rcd);
>  	unsigned int hdr_pg_off = FIELD_GET(FBNIC_RCD_AL_BUFF_OFF_MASK, rcd);
> -	struct page *page = fbnic_page_pool_get_head(qt, hdr_pg_idx);
>  	unsigned int len = FIELD_GET(FBNIC_RCD_AL_BUFF_LEN_MASK, rcd);
>  	unsigned int frame_sz, hdr_pg_start, hdr_pg_end, headroom;
>  	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 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[].
> +	 */
> +	if (unlikely(hdr_pg_idx > qt->sub0.size_mask)) {
> +		pkt->add_frag_failed = true;
> +		return;
> +	}

[Severity: High]
Does this rejection stop the injected out-of-range completion described
in the commit message? The caller stores the raw ID before it calls
in here, and nothing resets it when the check fails:

drivers/net/ethernet/meta/fbnic/fbnic_txrx.c:fbnic_clean_rcq() {
    ...
		case FBNIC_RCD_TYPE_HDR_AL:
			head0 = FIELD_GET(FBNIC_RCD_AL_BUFF_PAGE_MASK, rcd);
			fbnic_pkt_prepare(nv, rcd, pkt, qt);
    ...
	if (head0 >= 0)
		fbnic_clean_bdq(&qt->sub0, head0, budget);
    ...
}

So a head0 of 65535 still reaches fbnic_clean_bdq() as hw_head:

fbnic_clean_bdq() {
    ...
	do {
		fbnic_page_pool_drain(ring, head, napi_budget);

		head++;
		head &= ring->size_mask;
	} while (head != hw_head);
    ...
}

Because head is masked to [0, size_mask], it can never equal an hw_head
above size_mask. Does this loop ever terminate?

As the loop runs, fbnic_page_pool_drain() hands buffers that are still
posted to the device back to the page_pool, and the device can still DMA
into them.

The ring always keeps at least one slot unposted. fbnic_desc_unused()
returns (head - tail - 1) & size_mask, and that slot has
rx_buf->netmem == 0. When the loop reaches it,
page_pool_unref_netmem(0, ...) dereferences
netmem_to_nmdesc(0)->pp_ref_count.

Wouldn't the KASAN injection test from the commit message still oops in
NAPI softirq, just later in the poll?

head1 has the same problem: FBNIC_RCD_TYPE_PAY_AL stores it before the
new check in fbnic_add_rx_frag() runs, and fbnic_clean_bdq(&qt->sub1,
head1, budget) uses it afterwards.

Could the ID be validated in fbnic_clean_rcq() before head0/head1 are
updated? Alternatively, the helpers could return a bool and the heads
could be updated only on success.

> +
> +	page = fbnic_page_pool_get_head(qt, hdr_pg_idx);

[Severity: Medium]
The new comment says "A buffer ID the device never got from us cannot
name a buffer we own". The commit message says the arrays "only cover
the buffers the driver posted". Are those statements accurate?

rx_buf[] has size_mask + 1 entries, and at least one of them is never
posted. fbnic_desc_unused() always leaves one slot free. fbnic_fill_bdq()
also stops early when page_pool_dev_alloc_netmems() fails. These slots
have rx_buf->netmem == 0, either from kvzalloc() or from the
rx_buf->netmem = 0 in fbnic_page_pool_drain().

A completion that names one of these in-range slots passes the size_mask
check. fbnic_page_pool_get_head() then decrements pagecnt_bias on the
empty slot and returns netmem_to_page(0), which is NULL.

Can that NULL page then be dereferenced by page_pool_get_dma_addr(page)
and page_address(page) later in fbnic_pkt_prepare()?

The payload side in fbnic_add_rx_frag() looks the same: netmem 0 would
be passed to page_pool_dma_sync_netmem_for_cpu() and
xdp_buff_add_frag().

Should the check also confirm that the slot was posted? For example, it
could test !qt->sub0.rx_buf[idx].netmem, or check the index against the
[ring->head, ring->tail) window.

> +
>  	/* Short-cut the end calculation if we know page is fully consumed */
>  	hdr_pg_end = FIELD_GET(FBNIC_RCD_AL_PAGE_FIN, rcd) ?
>  		     FBNIC_BD_FRAG_SIZE : fbnic_hdr_pg_end(hdr_pg_off, len);

[Severity: High]
This isn't a bug introduced by this patch, but it is on the same
malformed-completion path the patch hardens. Is len or hdr_pg_off
checked against the fragment size anywhere?

FBNIC_RCD_AL_BUFF_LEN_MASK is DESC_GENMASK(28, 16), so len can be as
large as 8191. FBNIC_BD_FRAG_SIZE is 4096.

As an example, take an in-range, posted header ID with
hdr_pg_off = 256, len = 8191 and PAGE_FIN clear. On x86_64 with 4K
pages (FBNIC_RX_TROOM = 320, FBNIC_RX_HROOM = 192):

    fbnic_hdr_pg_end(256, 8191) = ALIGN(256 + 8191 + 320 + 192, 128) - 192
                                = 8768
    fbnic_hdr_pg_start(256)     = 0
    frame_sz                    = 8768

Nothing compares frame_sz with the page size. As a result,
dma_sync_single_range_for_cpu() runs past the mapping, and
xdp_get_shared_info_from_buff() places skb_shared_info at
data_hard_start + 8448, well past the end of the 4096-byte page.

A following valid PAY completion passes both new checks and reaches
__xdp_buff_add_frag(). That function writes sinfo->xdp_frags_size,
nr_frags and frags[] through that pointer. On the META path,
xdp_build_skb_from_buff() builds the skb with the same frame_sz and
initializes the shared info outside the page.

Separately, an hdr_pg_off below FBNIC_RX_HROOM (other than the special
case in fbnic_hdr_pg_start()) makes fbnic_hdr_pg_start() underflow.

[ ... ]

> @@ -1026,10 +1036,23 @@ static void fbnic_add_rx_frag(struct fbnic_napi_vector *nv, u64 rcd,
>  	unsigned int pg_idx = FIELD_GET(FBNIC_RCD_AL_BUFF_PAGE_MASK, rcd);
>  	unsigned int pg_off = FIELD_GET(FBNIC_RCD_AL_BUFF_OFF_MASK, rcd);
>  	unsigned int len = FIELD_GET(FBNIC_RCD_AL_BUFF_LEN_MASK, rcd);
> -	netmem_ref netmem = fbnic_page_pool_get_data(qt, pg_idx);
>  	unsigned int truesize;
> +	netmem_ref netmem;
>  	bool added;
>  
> +	/* The header completion was rejected, so buff was never initialized
> +	 * and add_frag_failed is already set.  There is nothing to add to.
> +	 */
> +	if (unlikely(!pkt->buff.data_hard_start))
> +		return;
> +
> +	if (unlikely(pg_idx > qt->sub1.size_mask)) {
> +		pkt->add_frag_failed = true;
> +		return;
> +	}
> +
> +	netmem = fbnic_page_pool_get_data(qt, pg_idx);
> +
>  	truesize = FIELD_GET(FBNIC_RCD_AL_PAGE_FIN, rcd) ?
>  		   FBNIC_BD_FRAG_SIZE - pg_off : ALIGN(len, 128);
>  

[Severity: Medium]
This isn't a bug introduced by this patch. Since this hunk moves the
fbnic_page_pool_get_data() call, though, what happens to the reference
it takes when xdp_buff_add_frag() fails further down in this function?

fbnic_page_pool_get_data() decrements rx_buf->pagecnt_bias, which
transfers one page_pool reference to the fragment. xdp_buff_add_frag()
can then return false, for example because the frame already carries
MAX_SKB_FRAGS fragments. In that case the code sets add_frag_failed
and logs the error, but netmem was never attached to pkt->buff.

The drop path in fbnic_put_pkt_buff() only walks the fragments that
are recorded in the shared info, so it never releases this one. Later,
fbnic_page_pool_drain() only drops the remaining pagecnt_bias for the
slot. Doesn't that leave one reference on the page that nobody will
ever drop, so the page is never returned to qt->sub1.page_pool?

Should the failure path hand the reference back? It could restore
rx_buf->pagecnt_bias for that slot, or release netmem to
qt->sub1.page_pool the way fbnic_put_pkt_buff() does for attached
fragments. That probably belongs in a separate fix.

[Severity: High]
The new comment says add_frag_failed is already set whenever
data_hard_start is NULL here. Is a rejected header completion the only
way to reach this point with a NULL data_hard_start?

Suppose the device sends a PAY_AL completion without a preceding HDR_AL
completion. Then fbnic_pkt_prepare() never ran for this frame. As far
as I can see, add_frag_failed is only written by fbnic_pkt_prepare()
and fbnic_add_rx_frag(). It therefore still holds whatever the previous
frame left behind, which is false after any frame that completed
normally.

This early return then leaves it false. When the META completion
arrives, fbnic_clean_rcq() sees !pkt->add_frag_failed and calls
xdp_get_buff_len() and fbnic_run_xdp() on an xdp_buff that was never
initialized for this frame. data_hard_start is NULL and the other
fields are left over from the previous frame. Wouldn't
xdp_build_skb_from_buff() then hand that NULL buffer to
__build_skb_around()?

Before this patch, the same sequence crashed in xdp_buff_add_frag(),
so the crash isn't new. However, the comment now documents an
invariant that the code doesn't enforce. The point of the patch is to
survive a misbehaving device, so could this early return also set
pkt->add_frag_failed = true?

A META completion with no HDR_AL before it would reach fbnic_run_xdp()
the same way, without passing through fbnic_add_rx_frag() at all.
Would it be more robust to also check pkt->buff.data_hard_start in the
FBNIC_RCD_TYPE_META case of fbnic_clean_rcq()?

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