Re: [PATCH v4 net 6/8] net: enetc: pad short XDP frames in software

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · Medium: 1 (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] The new XDP pad BD in enetc_xdp_tx() is only partly initialised.
- [Medium] In enetc_xdp_tx()
  (drivers/net/ethernet/freescale/enetc/enetc.c), the new early `return…

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but the XDP_TX
  path in enetc_clean_rx_ring_xdp() ignores changes the BPF program…

--

net: enetc: pad short XDP frames in software

When an XDP frame is shorter than ETH_ZLEN, enetc_xdp_tx() now appends a
BD that points to the shared tx_pad zero buffer. The F flag, is_eof and
xdp_frame move onto that pad BD. Frames whose head buffer is shorter than
ENETC_MIN_BUFF_SIZE are dropped, because both callers treat a false return
as a drop.

> diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
> index 89b47accbac84..d456b9b488acf 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc.c
> @@ -1750,8 +1750,11 @@ static bool enetc_xdp_tx(struct enetc_bdr *tx_ring,
>  {
>  	struct enetc_tx_swbd *tmp_tx_swbd = xdp_tx_arr;
>  	int i, k, frm_len = tmp_tx_swbd->len;
> +	struct enetc_tx_swbd *last_swbd;
> +	int pad_len;
>  
> -	if (unlikely(enetc_bd_unused(tx_ring) < ENETC_TXBDS_NEEDED(num_tx_swbd)))
> +	/* Hardware does not support a first buffer size of less than 16 bytes */
> +	if (unlikely(xdp_tx_arr[0].len < ENETC_MIN_BUFF_SIZE))
>  		return false;

[Severity: Medium]
Before this patch, enetc_xdp_tx() returned false only when the TX ring
was full. With this check it can also return false because of what is in
the frame.

enetc_xdp_xmit() still treats every false return as a full ring and stops
processing the bulk:

drivers/net/ethernet/freescale/enetc/enetc.c:enetc_xdp_xmit() {
    ...
		if (unlikely(!enetc_xdp_tx(tx_ring, xdp_redirect_arr,
					   xdp_tx_bd_cnt))) {
			for (i = 0; i < xdp_tx_bd_cnt; i++)
				enetc_unmap_tx_buff(tx_ring,
						    &xdp_redirect_arr[i]);
			break;
		}
    ...
}

Then bq_xmit_all() in kernel/bpf/devmap.c frees every frame from the
returned count onward:

	for (i = sent; unlikely(i < to_send); i++)
		xdp_return_frame_rx_napi(bq->q[i]);

Take a redirected frame with a 14 or 15 byte head. One example is a
14-byte AF_PACKET frame sent on a veth peer and redirected through a
devmap. Another is a frame trimmed to ETH_HLEN with bpf_xdp_adjust_tail().

In that case, are the valid frames after it in the same ndo_xdp_xmit()
bulk dropped and counted in xdp_tx_drops too, even though the ring has
room?

The commit message says:

    Both callers already treat a false return code from enetc_xdp_tx() as
    a drop.

It doesn't mention that the rest of the bulk is dropped as well, and the
later patches in the series don't change this.

Could enetc_xdp_xmit() check the head length before the DMA mapping, and
drop only that frame instead of breaking out of the loop?

>  	while (unlikely(!tmp_tx_swbd->is_eof)) {
> @@ -1759,15 +1762,24 @@ static bool enetc_xdp_tx(struct enetc_bdr *tx_ring,
>  		frm_len += tmp_tx_swbd->len;
>  	}
>  
> +	/* Pad frames below ETH_ZLEN so the mEMAC does not pad them. */
> +	pad_len = max_t(int, ETH_ZLEN - frm_len, 0);

[Severity: Medium]
This is a pre-existing issue, not one introduced by this patch. On the
XDP_TX path, frm_len is built from lengths that leave out any changes the
BPF program made. enetc_clean_rx_ring_xdp() fills xdp_tx_arr through
enetc_rx_swbd_to_xdp_tx_swbd():

	tx_swbd->dma = rx_swbd->dma;
	...
	tx_swbd->page_offset = rx_swbd->page_offset;
	tx_swbd->len = rx_swbd->len;

rx_swbd->len is set in enetc_map_rx_buff_to_xdp() and
enetc_add_rx_buff_to_xdp() before bpf_prog_run_xdp() runs. The driver
never reads back xdp_buff.data/data_end or any frag sizes the program
changed.

If a program calls bpf_xdp_adjust_head() or bpf_xdp_adjust_tail() and
returns XDP_TX, does it still send the original byte range and length?

The commit message mentions this ("once the driver honours
bpf_xdp_adjust_tail() and bpf_xdp_adjust_head() for XDP_TX"), and no patch
in the series fixes it. The padding added here uses the same stale
lengths, which do match what is actually sent.

> +
> +	if (unlikely(enetc_bd_unused(tx_ring) <
> +		     ENETC_TXBDS_NEEDED(num_tx_swbd + !!pad_len)))
> +		return false;
> +
>  	i = tx_ring->next_to_use;

[ ... ]

> @@ -1776,6 +1788,22 @@ static bool enetc_xdp_tx(struct enetc_bdr *tx_ring,
>  		enetc_bdr_idx_inc(tx_ring, &i);
>  	}
>  
> +	/* The pad BD becomes the last one, so it takes over the xdp_frame
> +	 * (if any) from the last data BD. The pad BD has no DMA mapping of
> +	 * its own, and it is not an XDP_TX buffer to be recycled.
> +	 */
> +	if (unlikely(pad_len)) {
> +		struct enetc_tx_swbd *pad_swbd = &tx_ring->tx_swbd[i];
> +
> +		enetc_map_tx_pad(tx_ring, i, pad_len);
> +		pad_swbd->is_xdp_redirect = last_swbd->is_xdp_redirect;
> +		pad_swbd->xdp_frame = last_swbd->xdp_frame;

[Severity: High]
Is pad_swbd fully initialized here? enetc_map_tx_pad() sets only these
swbd fields:

	tx_swbd->len = pad_len;
	tx_swbd->is_eof = 1;
	tx_swbd->do_twostep_tstamp = false;
	tx_swbd->check_wb = false;

This block then adds is_xdp_redirect and xdp_frame. The other fields
(is_xdp_tx, page, page_offset, dma, is_dma_page, dir and qbv_en) keep
whatever the slot held before. The data BDs are fine, because
enetc_xdp_map_tx_buff() memcpy()s the whole swbd.

enetc_clean_tx_ring() normally memsets each slot it cleans, but slots can
be left stale. enetc_stop() doesn't clean BDs that have already completed.
After that, enetc_free_tx_frame() only does:

	if (tx_swbd->dma)
		enetc_unmap_tx_buff(tx_ring, tx_swbd);

That clears dma but leaves is_xdp_tx and page set.

Now suppose enetc_reconfigure_xdp_cb() fails, for example when
netif_set_real_num_tx_queues() returns -ENOMEM on XDP detach.
enetc_reconfigure() then restarts the same tx_swbd arrays without
scrubbing them:

out_restart:
	enetc_setup_bdrs(priv, extended);
	enetc_start(priv->ndev);

enetc_setup_txbdr() reloads next_to_clean and next_to_use from the
hardware, so these uncleaned slots end up in the free part of the ring.

Later, a short XDP frame may put its pad BD in one of those slots. At
completion, enetc_clean_tx_ring() would then run:

		if (tx_swbd->is_xdp_tx)
			enetc_recycle_xdp_tx_buff(tx_ring, tx_swbd);

on the pad BD. Couldn't this put the stale page back into the RX ring
with dma == 0, or unmap and free a page that was already freed? Either
way, xdp_tx_in_flight would be decremented for a BD that never
incremented it.

The later patch "fix leak of pending XDP_TX frames when going down"
seems to make this worse. It frees the page but still leaves is_xdp_tx
and page set.

Would it help to clear the pad swbd fully, for example with a memset
before enetc_map_tx_pad(), or by clearing is_xdp_tx, page, is_dma_page
and dma explicitly?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261015105536.1692348-1-vladimir.oltean%40nxp.com