Re: [PATCH v5 net 3/4] net: enetc: ensure head buffer has at least 16 octets

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] At this commit, the 16-octet minimum for the head BUFF_LEN and
  for FRM_LEN is enforced only in the non-GSO skb branch of…
- [Low] The commit message says the pskb_may_pull() handling "is case 5
  below".

Pre-existing issues:
- [Medium] In enetc_start_xmit(), when an skb has
  ENETC_F_TX_ONESTEP_SYNC_TSTAMP set, the caller takes the…

--

net: enetc: ensure head buffer has at least 16 octets

On the non-GSO path, enetc_start_xmit() now uses pskb_may_pull() to move
bytes from the paged area into the head when the head buffer is shorter
than 16 octets. The ENETC does not accept a first TX BD with BUFF_LEN
below 16. Such a descriptor stalls the TX ring consumer index and leads
to an IRQ storm.

Closes: https://lore.kernel.org/netdev/b3d9136c-2803-4203-b1ea-1f9e62de80a1@gmail.com/

> Because the driver now pads linear skbs to ETH_ZLEN for other reasons
> too, we just need to ensure the head buffer has at least 16 bytes.
> This can be done using pskb_may_pull(), which moves that number of bytes
> from the paged area into the head. This is case 5 below.
>
>   skb_headlen (H) | skb->len (L) | linear? | handled by
>   ----------------+--------------+---------+---------------------------------------
>    H == L         | L < 60       |  yes    | eth_skb_pad() (pads head to 60)
>    H == L         | L >= 60      |  yes    | already valid, no-op
>    H < 16         | L < 60       |  no     | eth_skb_pad() (linearizes + pads to 60)
>    H < 16         | L >= 60      |  no     | pskb_may_pull() (pulls frags into head)
>    H >= 16        | L >= 60      |  no     | already valid, no-op

[Severity: Low]
This isn't a bug, but should "case 5" be "case 4"? The pskb_may_pull()
case (H < 16, L >= 60) is the fourth row. The fifth row (H >= 16,
L >= 60) is the no-op case.

The table also has no row for the non-linear H >= 16, L < 60 case.
eth_skb_pad() handles that one too, because
skb_put_padto()->__skb_pad() linearizes a non-linear skb before padding.

Could that row be added so the case analysis is complete?

> diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
> index f0f9ba94b4df6..f5a47d53d2bea 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc.c
> @@ -1091,6 +1091,12 @@ static netdev_tx_t enetc_start_xmit(struct sk_buff *skb,
>  		if (eth_skb_pad(skb))
>  			return NETDEV_TX_OK;
>  
> +		if (unlikely(skb_headlen(skb) < ENETC_MIN_BUFF_SIZE &&
> +			     !pskb_may_pull(skb, ENETC_MIN_BUFF_SIZE))) {

[Severity: Low]
This covers the non-GSO skb path. Can the XDP transmit path still build
a first BD with BUFF_LEN < 16?

enetc_xdp_xmit()
  enetc_xdp_frame_to_xdp_tx_swbd()   <- head len is xdp_frame->len
  enetc_xdp_tx()
    enetc_xdp_map_tx_buff()

drivers/net/ethernet/freescale/enetc/enetc.c:enetc_xdp_map_tx_buff() {
    ...
	txbd->buf_len = cpu_to_le16(tx_swbd->len);
	txbd->frm_len = cpu_to_le16(frm_len);
    ...
}

Neither length is checked against ENETC_MIN_BUFF_SIZE at this commit.
Two kinds of frame could still produce the invalid BD:

  - a redirected frame that bpf_xdp_adjust_tail() shrank to ETH_HLEN
  - a multi-buffer xdp_frame with a 14-15 byte head

Either one looks like it could cause the TBaCIR stall and IRQ storm
described in the commit message.

The later patch in this series, "net: enetc: pad short XDP frames in
software", appears to close this. It makes enetc_xdp_tx() drop frames
whose first buffer is shorter than ENETC_MIN_BUFF_SIZE, and it adds a
pad BD so that FRM_LEN reaches ETH_ZLEN. enetc_xdp_xmit() and the XDP_TX
path both go through enetc_xdp_tx(), so the gap only exists at this
intermediate commit.

> +			dev_kfree_skb_any(skb);
> +			return NETDEV_TX_OK;
> +		}

[Severity: Medium]
This is a pre-existing issue that was not introduced by this patch, but
the new drop path inherits it. If the skb has
ENETC_F_TX_ONESTEP_SYNC_TSTAMP set, enetc_start_xmit() has already taken
the bit at the top of the function:

	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;
		}
	}

If pskb_may_pull() fails here, the skb is freed. The bit is not cleared
and priv->tx_onestep_tstamp is not scheduled.

Only enetc_tx_onestep_tstamp() releases the bit. That work is scheduled
only from enetc_clean_tx_ring(), when a one-step skb completes in the TX
ring, and a dropped skb never gets there.

After such a drop, would every later one-step Sync skb stay in
priv->tx_skbs forever, with the queue growing without bound?

The existing drop_packet_err label already leaks the bit the same way.
It is reached on skb_linearize() failure, or when DMA mapping fails and
count is zero. A common drop path that releases the bit (or schedules
the work) for one-step skbs would fix both cases, separately from this
patch.

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

[ ... ]

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