Re: [PATCH v3 net 2/7] net: enetc: ensure enetc_xdp_xmit() calls enetc_update_tx_ring_tail()

[netdev-bot+sashiko]

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

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

- [High] drivers/net/ethernet/freescale/enetc/enetc.c, enetc_xdp_xmit():
  the new doorbell condition `if (unlikely(k && ((flags &…
- [Low] The commit message of "net: enetc: ensure enetc_xdp_xmit() calls
  enetc_update_tx_ring_tail()" carries a Fixes: 9d2b68cc108d tag (making…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the function
  it modifies contains an unsynchronised shared-TX-ring race.

--

net: enetc: ensure enetc_xdp_xmit() calls enetc_update_tx_ring_tail()

This changes the condition under which enetc_xdp_xmit() rings the TX
doorbell, since the old "k != xdp_tx_frm_cnt" test could never be true,
and removes the now redundant xdp_tx_frm_cnt variable in favour of the
loop index k.

A couple of questions below.

> The correct condition on which we should ring the doorbell is when at
> least one frame was sent, and either the caller required us to flush, or
> we couldn't enqueue the entire passed array.

[Severity: Low]
The patch carries a Fixes: tag but does not describe any user-visible
symptom of the old code.  Was one observed?

Looking at bq_enqueue() in kernel/bpf/devmap.c, the current frame is always
stored right after the bulk transmit:

	if (unlikely(bq->count == DEV_MAP_BULK_SIZE))
		bq_xmit_all(bq, 0);
	...
	bq->q[bq->count++] = xdpf;

so bq->count >= 1 whenever __dev_flush() runs, and bq_xmit_all() does not
take its "if (unlikely(!cnt)) return;" early exit.  The pre-patch code then
rang the doorbell unconditionally on that XDP_XMIT_FLUSH call in the same
NAPI poll, which would make the dead "k != xdp_tx_frm_cnt" condition a
latency issue rather than a lost-frame issue.

Could the commit message say what the practical effect was, so that stable
backporters can judge the risk?

> diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
> index e7ecdf8ef67c8..c9c23e994f6d1 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc.c

[ ... ]

> @@ -1845,19 +1844,17 @@ int enetc_xdp_xmit(struct net_device *ndev, int num_frames,
>  						    &xdp_redirect_arr[i]);
>  			break;
>  		}
> -
> -		xdp_tx_frm_cnt++;
>  	}
>  
> -	if (unlikely((flags & XDP_XMIT_FLUSH) || k != xdp_tx_frm_cnt))
> +	if (unlikely(k && ((flags & XDP_XMIT_FLUSH) || k < num_frames)))
>  		enetc_update_tx_ring_tail(tx_ring);

[Severity: High]
Can the new "k &&" term drop a doorbell that the caller explicitly asked
for with XDP_XMIT_FLUSH?

enetc_xdp_tx() only writes the BDs and advances tx_ring->next_to_use.  The
hardware producer index is written only here:

static void enetc_update_tx_ring_tail(struct enetc_bdr *tx_ring)
{
	/* includes wmb() */
	enetc_wr_reg_hot(tx_ring->tpir, tx_ring->next_to_use);
}

Now consider the devmap bulk path in kernel/bpf/devmap.c:

bq_enqueue()
	if (unlikely(bq->count == DEV_MAP_BULK_SIZE))
		bq_xmit_all(bq, 0);		/* flags == 0, no doorbell */

__dev_flush()
	bq_xmit_all(bq, XDP_XMIT_FLUSH);	/* the only flush */
	bq->dev_rx = NULL;
	__list_del_clearprev(&bq->flush_node);

The flags == 0 call enqueues all its frames (k == num_frames) and
legitimately defers the doorbell.  If the following XDP_XMIT_FLUSH call
breaks at the very first frame, k == 0 and enetc_update_tx_ring_tail() is
now skipped, so the BDs written by the earlier call are never announced to
hardware.  The bq and its flush-list entry are cleared right afterwards, so
there is no retry.

k == 0 on that flush call looks reachable through any of the loop's early
breaks:

	if (unlikely((shinfo->nr_frags + 1) > ENETC_MAX_SKB_FRAGS))
		break;
	...
	if (unlikely(xdp_tx_bd_cnt < 0))		/* DMA map error */
		break;
	...
	if (unlikely(!enetc_xdp_tx(tx_ring, xdp_redirect_arr,
				   xdp_tx_bd_cnt))) {		/* ring full */

The ring-full variant looks self-perpetuating: the un-announced BDs keep
enetc_bd_unused() low, every later call fails at frame 0, and the doorbell
is skipped again.  enetc_clean_tx_ring() cannot release those xdp_frames or
their DMA mappings because hardware never completes descriptors it was not
told about, and the dedicated per-CPU XDP TX ring is outside
real_num_tx_queues so the TX watchdog does not cover it.  The XDP_TX path
has the same shape:

enetc_clean_rx_ring_xdp()
	if (xdp_tx_frm_cnt)
		enetc_update_tx_ring_tail(tx_ring);

so it does not re-synchronise TPIR either.

Since writing TPIR with an unchanged next_to_use is idempotent, is there
anything gained by the "k &&" term?  Would keeping the flush branch
unconditional be preferable, e.g.

	if (unlikely((flags & XDP_XMIT_FLUSH) || (k && k < num_frames)))

The condition is unchanged at the end of the series (381447d90b1f).

>  
> -	tx_ring->stats.xdp_tx += xdp_tx_frm_cnt;
> -	tx_ring->stats.xdp_tx_drops += num_frames - xdp_tx_frm_cnt;
> +	tx_ring->stats.xdp_tx += k;
> +	tx_ring->stats.xdp_tx_drops += num_frames - k;
>  
>  	enetc_unlock_mdio();

[Severity: High]
This isn't a bug introduced by this patch, but while touching
enetc_xdp_xmit() it may be worth looking at what serialises this ring
against the stack TX path.

enetc_xdp_xmit() picks the ring per-CPU and holds no TX queue lock:

	tx_ring = priv->xdp_tx_ring[smp_processor_id()];

The only lock taken is enetc_lock_mdio(), which in enetc_hw.h is

	if (static_branch_unlikely(&enetc_has_err050089))
		read_lock(&enetc_mdio_lock);

i.e. a reader lock for an unrelated MDIO erratum, so it gives no mutual
exclusion between two producers.

The XDP rings are a slice of the same array the stack uses:

enetc_alloc_msix()
	first_xdp_tx_ring = priv->num_tx_rings - num_possible_cpus();
	priv->xdp_tx_ring = &priv->tx_ring[first_xdp_tx_ring];

and they are only hidden from the stack when a program is attached:

enetc_num_stack_tx_queues()
	if (priv->xdp_prog)
		return num_tx_rings - num_possible_cpus();

	return num_tx_rings;

enetc_pf_netdev_setup() advertises NETDEV_XDP_ACT_NDO_XMIT unconditionally,
and devmap only tests that flag, so ndo_xdp_xmit() can be called into this
device with priv->xdp_prog == NULL.  In that case every ring, including the
xdp_tx_ring range, is also reachable from

enetc_start_xmit()
	tx_ring = priv->tx_ring[skb->queue_mapping];

under only that queue's __netif_tx_lock.  Can the two producers then race on
the same next_to_use and tx_swbd slot, overwriting bookkeeping (leaking the
previous skb and its DMA mapping) and leaving enetc_clean_tx_ring() freeing
entries that do not match what hardware consumed?

The overlap looks unavoidable when num_tx_rings == num_possible_cpus():
first_xdp_tx_ring becomes 0, and enetc_setup_xdp_prog()'s check

	if (priv->min_num_stack_tx_queues + num_xdp_tx_queues >
	    priv->num_tx_rings) {

prevents attaching a program at all, yet ndo_xdp_xmit() still targets rings
0..nr_cpus-1, which are pure stack queues.

i40e and ice return -ENXIO from ndo_xdp_xmit when XDP TX rings are not
reserved.  Would a similar check fit here?

>  
> -	return xdp_tx_frm_cnt;
> +	return k;
>  }
>  EXPORT_SYMBOL_GPL(enetc_xdp_xmit);
>

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