Re: [PATCH v4 net 2/8] net: enetc: serialize access to the same TX ring from NAPI XDP_TX and ndo_xdp_xmit()

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 3 (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 new xdp_lock does not serialize enetc_xdp_xmit()
  (ndo_xdp_xmit) against the regular stack transmit path,…
- [High] Lock-order inversion between tx_ring->xdp_lock and the ERR050089
  MDIO rwlock (enetc_mdio_lock), introduced in enetc_clean_rx_ring_xdp().
- [High] net/enetc: concurrent TX ring corruption from stack and
  XDP_REDIRECT

Pre-existing issues:
- [High] enetc_stop() does not synchronize with ndo_xdp_xmit() calls
  already in progress.

--

net: enetc: serialize access to the same TX ring from NAPI XDP_TX and ndo_xdp_xmit()

This patch adds a spinlock (xdp_lock) to struct enetc_bdr. The lock
serializes the NAPI XDP_TX path and ndo_xdp_xmit() when both target the
same XDP TX ring. NAPI takes the lock for the whole poll cycle, drops it
around napi_gro_receive() and xdp_do_redirect(), and takes it again
around enetc_update_tx_ring_tail().

> diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
> index 26abc5fafd188..418be387af18b 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc.c
> @@ -1817,6 +1817,7 @@ int enetc_xdp_xmit(struct net_device *ndev, int num_frames,
>  	int err = 0;
>  
>  	tx_ring = priv->xdp_tx_ring[smp_processor_id()];
> +	spin_lock(&tx_ring->xdp_lock);

[Severity: High]
When no XDP program is attached to this interface, is the ring picked
here also used by the regular stack transmit path?

With priv->xdp_prog NULL, enetc_num_stack_tx_queues() exposes every TX
ring to the stack:

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

	return num_tx_rings;

This makes the rings that priv->xdp_tx_ring[] points to ordinary netdev
queues as well. enetc_start_xmit() selects them through
priv->tx_ring[skb->queue_mapping]. It then updates next_to_use,
tx_swbd[] and the TBPIR doorbell while holding only the netdev_queue
xmit lock.

A redirect from another interface does not need a program on this
port. Devmap only checks the advertised NETDEV_XDP_ACT_NDO_XMIT bit
before calling ndo_xdp_xmit(). enetc_xdp_xmit() does not look at
priv->xdp_prog before using priv->xdp_tx_ring[smp_processor_id()].
That path now takes xdp_lock, but enetc_start_xmit() never does, so
the two producers are still not serialized against each other.

Can they then claim the same next_to_use slots and overwrite each
other's tx_swbd[] entries? tx_swbd[] holds a union of skb and
xdp_frame, and the is_xdp_redirect bit selects between them. Completion
could therefore treat an skb as an xdp_frame or the reverse. It could
also free a buffer twice or leak it, and the hardware could be handed a
BD that points at memory that is already unmapped. Any traffic that
another interface redirects into this port is enough to trigger it.

This race predates the patch, since ndo_xdp_xmit() had no locking at
all before. However, the commit message and the new comment on
struct enetc_bdr::xdp_lock present the lock as covering the TX BDR
producer state. It does not do that while the stack shares the ring.

Would it work to take the netdev_queue xmit lock in both XDP
producers instead? That would be __netif_tx_lock() on
netdev_get_tx_queue(ndev, tx_ring->index), as fec, stmmac and igb do.
That would serialize all three writers and make xdp_lock unnecessary.

Alternatively, should enetc_xdp_xmit() refuse frames, or NDO_XMIT be
cleared with xdp_features_clear_redirect_target(), while no program
has reserved the XDP rings?

[Severity: High]
If no XDP program is attached to this port, can enetc_start_xmit() be
writing to this same ring at the same time?

With priv->xdp_prog NULL, enetc_num_stack_tx_queues() gives every ring to
the stack:

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

	return num_tx_rings;

So the rings that priv->xdp_tx_ring[] points to are also regular stack
queues. There is no ndo_select_queue, and enetc_start_xmit() uses them
while holding only the netdev_queue xmit lock:

	tx_ring = priv->tx_ring[skb->queue_mapping];

enetc_pf_netdev_setup() always advertises NETDEV_XDP_ACT_NDO_XMIT, and
devmap only checks that feature bit. A redirect from another interface
into this port therefore reaches enetc_xdp_xmit() even when no program
is loaded here. enetc_xdp_xmit() does not check priv->xdp_prog. It takes
only xdp_lock, and the stack path never takes that lock.

Could both producers then claim the same next_to_use slots and
tx_swbd[] entries? tx_swbd[] holds a union of skb and xdp_frame,
selected by is_xdp_redirect and the other bitfields. Completion could
then free an skb as an xdp_frame or the reverse, free a buffer twice,
leak one, or give the hardware a DMA address that is already unmapped.

This window also existed before the patch, because ndo_xdp_xmit() had no
locking at all. However, the commit message says:

  Specifically, the spin lock only synchronizes the stats counters
  relevant to XDP (xdp_tx/xdp_tx_drops) and the TX BDR producer state
  (next_to_use, tx_swbd[] and hardware counterparts). The rest should be
  not necessary.

The new comment on struct enetc_bdr::xdp_lock also names only NAPI and
ndo_xdp_xmit() as writers.

Would it work to take the netdev_queue xmit lock in both XDP producers
instead? That would be __netif_tx_lock() on
netdev_get_tx_queue(ndev, tx_ring->index), as fec, stmmac and igb do. It
would cover all three writers and replace xdp_lock.

Alternatively, could ndo_xdp_xmit be refused, or NDO_XMIT cleared with
xdp_features_clear_redirect_target(), while no program reserves the XDP
rings?

>  
>  	if (unlikely(test_bit(ENETC_TX_DOWN, &priv->flags) ||
>  		     !netif_carrier_ok(ndev))) {

[Severity: High]
This is a pre-existing issue, not one introduced by this patch. Does
enetc_stop() synchronize with an enetc_xdp_xmit() call that has already
passed this check?

enetc_stop() does the following without taking xdp_lock or waiting for a
grace period:

	set_bit(ENETC_TX_DOWN, &priv->flags);

	netif_tx_stop_all_queues(ndev);

	enetc_disable_rx_bdrs(priv);

	enetc_wait_bdrs(priv);

	enetc_disable_tx_bdrs(priv);

The napi_disable() calls that follow cover only the enetc vectors. They
do not cover the redirecting device's NAPI, which runs enetc_xdp_xmit()
from the devmap flush.

Suppose the ENETC_TX_DOWN test runs just before set_bit(). Then
enetc_xdp_xmit() keeps writing TX BDs, tx_swbd[] and next_to_use, and
rings TBPIR, while enetc_stop() drains and disables the ring.

After that, enetc_close() calls enetc_free_rxtx_rings() and
enetc_assign_tx_resources(priv, NULL). enetc_reconfigure() frees the
resources in a similar way.

Can the redirecting CPU then write into the freed tx_swbd array and BD
ring?

The core does not seem to close this window either. __dev_close_many()
calls synchronize_net() before ndo_stop, not after, and
enetc_reconfigure() keeps the device up the whole time. The race is
easier to hit on PREEMPT_RT, where softirq processing can be preempted.

The NAPI XDP_TX path has a smaller window of the same kind. It tests
ENETC_TX_DOWN per frame but rings the doorbell at the end of the poll.
napi_synchronize() runs only after enetc_wait_bdrs() and
enetc_disable_tx_bdrs().

On ENETC v1, enetc_setup_txbdr() copies next_to_use and next_to_clean
back from TBPIR/TBCIR. A late doorbell could therefore carry a stale
producer index into the newly allocated ring. Whether the hardware
latches a TBPIR write to a disabled ring has not been verified.

Now that xdp_lock exists, would taking it around set_bit(ENETC_TX_DOWN)
in enetc_stop(), or calling synchronize_net() after the set_bit(), close
this window?

[ ... ]

> @@ -2120,6 +2131,8 @@ static int enetc_clean_rx_ring_xdp(struct enetc_bdr *rx_ring,
>  	}
>  
>  out:
> +	spin_unlock(&tx_ring->xdp_lock);
> +
>  	rx_ring->next_to_clean = i;
>  
>  	rx_ring->stats.packets += rx_frm_cnt;

[ ... ]

> @@ -2131,8 +2144,11 @@ static int enetc_clean_rx_ring_xdp(struct enetc_bdr *rx_ring,
>  		enetc_lock_mdio();
>  	}
>  
> -	if (xdp_tx_frm_cnt)
> +	if (xdp_tx_frm_cnt) {
> +		spin_lock(&tx_ring->xdp_lock);
>  		enetc_update_tx_ring_tail(tx_ring);
> +		spin_unlock(&tx_ring->xdp_lock);
> +	}

[Severity: High]
Does this reverse the lock order between xdp_lock and enetc_mdio_lock?

In the poll loop and in enetc_xdp_xmit(), xdp_lock is taken first and
enetc_lock_mdio() second:

	spin_lock(&tx_ring->xdp_lock);
	...
	enetc_lock_mdio();

At out:, xdp_lock is dropped while the MDIO read lock is still held, and
the xdp_do_flush() block takes the MDIO read lock again. So this
spin_lock() runs with the MDIO read lock held, which is the opposite
order.

On PREEMPT_RT both are sleeping locks, and this cycle seems possible:

CPU A (enetc_clean_rx_ring_xdp)
  holds enetc_mdio_lock (read)
  spin_lock(&tx_ring->xdp_lock)          <- waits for CPU B

CPU B (enetc_xdp_xmit, same XDP TX ring)
  holds tx_ring->xdp_lock
  enetc_lock_mdio() -> read_lock()       <- waits for CPU C

CPU C (_enetc_rd_mdio_reg_wa)
  write_lock_irqsave(&enetc_mdio_lock)   <- waits for CPU A's reader

rwbase_write_lock() takes the rtmutex and removes READER_BIAS. After
that, __rwbase_read_lock() always goes to
rwbase_rtmutex_slowlock_locked(), so CPU B blocks on the rtmutex owned
by CPU C while CPU C waits for CPU A to drop its read lock.

Can this deadlock on LS1028A with PREEMPT_RT? There, enetc_has_err050089
is enabled and periodic PHY polling is enough to provide the MDIO
writer. On non-RT kernels, qrwlock readers in softirq context are not
blocked by a writer that is only waiting, so this looks RT-specific.

Would ringing the doorbell before dropping xdp_lock at out: keep the
order consistent? Another option would be to call enetc_unlock_mdio()
before taking xdp_lock here and take the MDIO lock again afterwards.

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