Re: [PATCH net v3 5/5] eth: fbnic: bound the XDP Tx completion head to the ring

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 (1) · Medium: 0 · 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 fixes only TWQ1.

Pre-existing issues:
- [High] fbnic_clean_twq1() can release the same XDP_TX pages to the
  page_pool twice, by a path the new head-window check does not cover.

--

eth: fbnic: bound the XDP Tx completion head to the ring

This patch adds a check to fbnic_clean_twq1() that returns early when the
TWQ1 completion head reported by the device is outside the ring or outside
the posted window. The goal is to stop the cleanup loop from running
forever, and to stop XDP Tx pages being released to the page pool twice,
when the device reports a bogus head.

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
> index 7a9bd91bcc41c..3a148eb4e560f 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
> @@ -661,6 +661,12 @@ static void fbnic_clean_twq1(struct fbnic_napi_vector *nv, bool pp_allow_direct,
>  	u64 total_bytes = 0, total_packets = 0;
>  	unsigned int head = ring->head;
>  
> +	/* bound the device head to the ring; see fbnic_clean_twq0() */
> +	if (unlikely(hw_head > ring->size_mask ||
> +		     ((hw_head - head) & ring->size_mask) >
> +		     fbnic_desc_used(ring)))
> +		return;

[Severity: High]
The new comment names fbnic_clean_twq0() as the model for this check. The
commit message says:

  fbnic_clean_twq0() masks the head difference to the ring and is finite;
  twq1 did not, which this brings in line.

Does fbnic_clean_twq0() actually do either of these checks? As far as I
can see, it only masks the head:

fbnic_clean_twq0() {
    ...
	clean_desc = (hw_head - head) & ring->size_mask;

	while (clean_desc) {
		struct sk_buff *skb = ring->tx_buf[head];
		unsigned int desc_cnt;

		desc_cnt = FBNIC_XMIT_CB(skb)->desc_count;
		if (desc_cnt > clean_desc)
			break;
    ...
}

fbnic_clean_tcq() reads head0 from the same TCD that head1 comes from:

	head0 = FIELD_GET(FBNIC_TCD_TYPE0_HEAD0_MASK, tcd);

head0 then reaches fbnic_clean_twq0() through fbnic_clean_twq() with no
ring or window check.

Suppose head0 is past the posted tail, or wraps behind ring->head. The loop
cleans every posted packet and head reaches ring->tail, but clean_desc is
still non-zero.

Wouldn't skb = ring->tx_buf[tail] be NULL at that point? The slot was
either never written or was cleared by ring->tx_buf[head] = NULL. If so,
FBNIC_XMIT_CB(skb)->desc_count dereferences NULL before the
desc_cnt > clean_desc break can run.

The timestamp path has the same exposure. It stores the unchecked value in
FBNIC_XMIT_CB(skb)->hw_head, and fbnic_clean_tsq() later passes it back as
head0.

fbnic_bdq_head_valid() already implements this exact check:

static bool fbnic_bdq_head_valid(struct fbnic_ring *ring, unsigned int hw_head)
{
	if (hw_head > ring->size_mask)
		return false;

	return ((hw_head - ring->head) & ring->size_mask) <=
	       fbnic_desc_used(ring);
}

Could this helper be used for both TWQ0 and TWQ1? If so, the comment and
commit message could be updated so they no longer describe TWQ0 as already
bounded.

> +
>  	while (hw_head != head) {
>  		struct page *page;
>  		u64 twd;

[Severity: High]
This is a pre-existing issue, not one introduced by this patch. Can
fbnic_clean_twq1() still release the same XDP_TX pages twice, by a path
the new window check does not cover?

Further down in fbnic_clean_twq1(), the page is released for every AL
descriptor the loop walks. ring->head only advances when total_bytes is
non-zero:

		page_pool_put_page(pp_page_to_nmdesc(page)->pp, page, -1,
				   pp_allow_direct);
next_desc:
		head++;
		head &= ring->size_mask;
	}

	if (!total_bytes)
		return;

	ring->head = head;

The tx_buf[] entries are not cleared either. If every descriptor walked in
one call has length 0, the pages go back to the pool but ring->head does
not move.

The next TWQ1 completion would then walk from the stale head and put the
same pages again. So would the flush in fbnic_nv_flush():

	fbnic_clean_twq1(nv, false, &qt->sub1, true,
			 qt->sub1.tail);

A zero-length AL descriptor appears to be postable. FBNIC_RX_PAD is 0, and
fbnic_pkt_prepare() accepts an RCD with len == 0 and builds a zero-length
XDP frame from it.

An XDP program that returns XDP_TX without adjusting the head or tail sends
that frame to fbnic_pkt_tx(). fbnic_pkt_tx() stores the page in tx_buf[]
and writes this length into the descriptor:

	size = pkt->buff.data_end - pkt->buff.data;

Zero-size frags from fbnic_add_rx_frag() can also be posted as len-0
descriptors when a non-zero frag follows them.

A device head1 just past such descriptors would pass the new check. Should
ring->head be advanced before the total_bytes early return?

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