Re: [PATCH v5 net 2/4] net: enetc: do not rely on mEMAC hardware frame padding

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] net: enetc: unreleased one-step timestamp lock in enetc_start_xmit()
--

commit f785a6d3699d340412051eda6871e1a582901866
Author: vladimir.oltean@nxp.com <vladimir.oltean@nxp.com>

net: enetc: do not rely on mEMAC hardware frame padding

This commit pads transmitted frames to ETH_ZLEN in software using
eth_skb_pad() for non-GSO packets and a shared zero-filled DMA padding
buffer descriptor for short GSO segments.

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

[ ... ]

> @@ -1053,6 +1088,9 @@ static netdev_tx_t enetc_start_xmit(struct sk_buff *skb,
>  			enetc_unlock_mdio();
>  		}
>  	} else {
> +		if (eth_skb_pad(skb))
> +			return NETDEV_TX_OK;
> +

[Severity: High]
Can this leave ENETC_TX_ONESTEP_TSTAMP_IN_PROGRESS permanently locked when
eth_skb_pad() fails on a short one-step PTP Sync packet?

At the beginning of enetc_start_xmit() in
drivers/net/ethernet/freescale/enetc/enetc.c, a packet with
ENETC_F_TX_ONESTEP_SYNC_TSTAMP acquires the bit lock:

	if (enetc_cb->flag & ENETC_F_TX_ONESTEP_SYNC_TSTAMP) {
		if (test_and_set_bit_lock(ENETC_TX_ONESTEP_TSTAMP_IN_PROGRESS,
					  &priv->flags)) {
			skb_queue_tail(&priv->tx_skbs, skb);
			return NETDEV_TX_OK;
		}
	}

Normally, this bit is only cleared by enetc_tx_onestep_tstamp() after TX
completion schedules priv->tx_onestep_tstamp from enetc_clean_tx_ring():

	clear_bit_unlock(ENETC_TX_ONESTEP_TSTAMP_IN_PROGRESS, &priv->flags);
	skb = skb_dequeue(&priv->tx_skbs);
	if (skb)
		enetc_start_xmit(skb, priv->ndev);

An IEEE 802.3 L2 PTPv2 Sync frame is 58 bytes (shorter than ETH_ZLEN) and
enters the non-GSO branch. If eth_skb_pad() fails with -ENOMEM, it frees
the skb and enetc_start_xmit() returns NETDEV_TX_OK without scheduling
priv->tx_onestep_tstamp or clearing ENETC_TX_ONESTEP_TSTAMP_IN_PROGRESS.

Because the skb was never queued to the TX ring, enetc_clean_tx_ring()
will never run for it, leaving ENETC_TX_ONESTEP_TSTAMP_IN_PROGRESS set and
causing all subsequent one-step Sync skbs to accumulate in priv->tx_skbs
indefinitely. Should eth_skb_pad() be called before acquiring the
ENETC_TX_ONESTEP_TSTAMP_IN_PROGRESS lock, or should the failure path
release the lock and kick the work item?

>  		if (unlikely(skb_shinfo(skb)->nr_frags > priv->max_frags))
>  			if (unlikely(skb_linearize(skb)))
>  				goto drop_packet_err;

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021145314.1435049-1-vladimir.oltean@nxp.com?part=2