Re: [PATCH net-next v8 1/5] net: stmmac: take ownership of saved RX state at poll entry

[netdev-bot+sashiko@kernel.org]

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 authorship and Signed-off-by chain disagree.

Pre-existing issues:
- [High] At this commit, stmmac_rx() still uses `skb == NULL` to decide
  that a descriptor starts a new frame.
- [High] This problem wasn't introduced by this patch, but the commit
  message relies on it being absent.

--

net: stmmac: take ownership of saved RX state at poll entry

This commit makes stmmac_rx() and stmmac_rx_zc() take the saved
partial-frame state at poll entry. A new in_progress flag tracks whether
a frame is still incomplete, and the state is saved again only in that
case. It also frees the saved skb in stmmac_reset_rx_queue() and in
__free_dma_rx_desc_resources().

> Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
> Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>

[Severity: Low]
This isn't a bug, but the author field and the Signed-off-by chain don't
match. The commit author is Lorenzo Bianconi. The first Signed-off-by is
James Hilliard, and there is no Co-developed-by tag.

If James wrote the patch, should it carry a "From: James Hilliard
<james.hilliard1@gmail.com>" line so git credits him as the author? If
Lorenzo is the author, does James's sign-off need a matching
Co-developed-by tag?

The other patches in the series carry only Lorenzo's sign-off. That
suggests this patch was written by James and picked into the series.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 53782ab55e125..8fd2acb25fb60 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c

[ ... ]

> @@ -5958,9 +5970,7 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
>  						      STMMAC_XDP_REDIRECT)) {
>  					xdp_status |= xdp_res;
>  					buf->page = NULL;
> -					skb = NULL;
> -					count++;
> -					continue;
> +					goto next;
>  				}
>  			}
>  		}

[Severity: High]
This is a pre-existing issue, and this patch doesn't change how it
behaves. What happens here when the XDP verdict is taken on a descriptor
that still has rx_not_ls set?

stmmac_rx() uses skb == NULL to mean a new frame is starting. The goto
next now also clears in_progress, and error stays 0. The remaining
descriptors of the same hardware frame would then each be handled as the
first descriptor of a new frame.

The XDP_CONSUMED branch just above has the same shape:

	skb = NULL;

	if (unlikely((status & rx_not_ls)))
		goto read_again;

So does the napi_build_skb() failure path:

	if (!skb) {
		page_pool_recycle_direct(rx_q->page_pool,
					 buf->page);
		buf->page = NULL;
		rx_dropped++;
		count++;
		goto drain_data;
	}

In each of these three cases, could the middle and tail descriptors be
passed to XDP and built into an skb with a stale or zero len? Could
buf1_len then underflow and produce an skb length near 4G?

An on-link peer can reach this by sending a frame that spans several
descriptors while an XDP program returns a non-PASS verdict. A
napi_build_skb() failure on such a frame reaches it too.

Later patches in this series seem to fix it. "net: stmmac: drop
multi-buffer frames in XDP mode" sets error on non-last descriptors when
XDP is attached. "net: stmmac: rework stmmac_rx to support XDP rx
multi-buff" runs XDP and stmmac_build_skb() only on the last descriptor
and sends build failures to error_free_frag.

[ ... ]

> @@ -8399,6 +8408,9 @@ static void stmmac_reset_rx_queue(struct stmmac_priv *priv, u32 queue)
>  {
>  	struct stmmac_rx_queue *rx_q = &priv->dma_conf.rx_queue[queue];
>  
> +	dev_kfree_skb_any(rx_q->state.skb);
> +	rx_q->state.skb = NULL;
> +	rx_q->state_saved = false;
>  	rx_q->cur_rx = 0;
>  	rx_q->dirty_rx = 0;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but the commit message says
that, without this change, the first poll "resumes that stale skb with
descriptors from the freshly initialized ring". Is the ring actually
re-initialized on the stmmac_resume() path?

stmmac_resume() does:

	stmmac_reset_queues_param(priv);

	stmmac_free_tx_skbufs(priv);
	stmmac_clear_descriptors(priv, &priv->dma_conf);

	ret = stmmac_hw_setup(ndev);

None of these calls refills buf_pool or rewrites the descriptor buffer
addresses. stmmac_clear_rx_descriptors() calls stmmac_init_rx_desc() on
every entry. On dwmac4 that only sets the owner bits:

static void dwmac4_rd_init_rx_desc(struct dma_desc *p, int disable_rx_ic,
				   u8 descriptor_mode, int end, int bfsize)
{
	dwmac4_set_rx_owner(p, disable_rx_ic);
}

Suppose stmmac_rx_refill() hit a page_pool allocation failure before
suspend:

		if (!buf->page) {
			buf->page = page_pool_alloc_pages(rx_q->page_pool, gfp);
			if (!buf->page)
				break;
		}

Then the entries between dirty_rx and cur_rx still have buf->page ==
NULL. On resume, cur_rx and dirty_rx are reset to 0 and every descriptor
goes back to the DMA. When one of those entries completes, can
stmmac_rx() call page_address(buf->page) and napi_build_skb() on a NULL
page?

There is a similar problem on dwmac4/xgmac. Descriptors that the hardware
writes back between napi_disable() in stmmac_suspend() and
stmmac_stop_all_dma() have RDES0/RDES1 overwritten with status words.
If those descriptors are given back to the DMA, would it write to a
bogus buffer address?

The resume sequence is the same at the end of the series.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261018-stmmac-rx-mb-v8-0-5f58c0b25849%40oss.qualcomm.com