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

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?

[ ... ]

[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).

[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?

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