Re: [PATCH net v4 2/2] igb: only strip Rx timestamp header on the first buffer of a frame

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] igb: XDP setup and MTU checks in igb_xdp_setup() reject default MTU 1500
--

commit e6fdd6185a160da19639b21242c6743c31579ab8
Author: Tjerk Kusters <tkusters@aweta.nl>

igb: only strip Rx timestamp header on the first buffer of a frame

This commit guards the Rx hardware timestamp header strip in
igb_clean_rx_irq() with !skb so that continuation buffers of multi-buffer
frames do not have payload bytes stripped when their payload starts with 8
zero bytes. It relies on the preceding commit in the series to prevent
multi-buffer frames from reaching the XDP path.

Link: https://lore.kernel.org/all/20261028083344.3047350-2-p@1g4.org/

> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> index 5bf489cd7b066..c4eb9f589fbe7 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -9072,8 +9072,11 @@ static int igb_clean_rx_irq(struct igb_q_vector *q_vector, const int budget)
>  		rx_buffer = igb_get_rx_buffer(rx_ring, size, &rx_buf_pgcnt);
>  		pktbuf = page_address(rx_buffer->page) + rx_buffer->page_offset;
>  
> -		/* pull rx packet timestamp if available and valid */
> -		if (igb_test_staterr(rx_desc, E1000_RXDADV_STAT_TSIP)) {
> +		/* pull rx packet timestamp if available and valid; it is only
> +		 * present on the first buffer of a frame
> +		 */
> +		if (!skb &&
> +		    igb_test_staterr(rx_desc, E1000_RXDADV_STAT_TSIP)) {

[Severity: High]
Does the prerequisite buffer size check in the series break attaching and
detaching XDP programs at the default MTU of 1500 on systems with 4 KiB
pages?

When the interface is opened at the default MTU of 1500
(adapter->max_frame_size = 1526), igb_open() -> igb_configure() ->
igb_configure_rx() -> igb_set_rx_buffer_len() enables build_skb without
setting the 3 KiB buffer flag:

drivers/net/ethernet/intel/igb/igb_main.c:igb_set_rx_buffer_len() {
	...
	set_ring_build_skb_enabled(rx_ring);

#if (PAGE_SIZE < 8192)
	if (adapter->max_frame_size > IGB_MAX_FRAME_BUILD_SKB ||
	    IGB_2K_TOO_SMALL_WITH_PADDING ||
	    rd32(E1000_RCTL) & E1000_RCTL_SBP)
		set_ring_uses_large_buffer(rx_ring);
#endif
}

Because adapter->max_frame_size (1526) is not greater than
IGB_MAX_FRAME_BUILD_SKB (1536 - NET_IP_ALIGN, i.e. 1536 or 1534) and
IGB_2K_TOO_SMALL_WITH_PADDING is false with 4 KiB pages, igb_rx_bufsz()
returns IGB_MAX_FRAME_BUILD_SKB:

drivers/net/ethernet/intel/igb/igb.h:igb_rx_bufsz() {
#if (PAGE_SIZE < 8192)
	if (ring_uses_large_buffer(ring))
		return IGB_RXBUFFER_3072;

	if (ring_uses_build_skb(ring))
		return IGB_MAX_FRAME_BUILD_SKB;
#endif
	return IGB_RXBUFFER_2048;
}

However, igb_xdp_setup() checks whether frame_size + IGB_TS_HDR_LEN fits in
igb_rx_bufsz(ring):

drivers/net/ethernet/intel/igb/igb_main.c:igb_xdp_setup() {
	int i, frame_size = dev->mtu + IGB_ETH_PKT_HDR_PAD;
	...
	for (i = 0; i < adapter->num_rx_queues; i++) {
		struct igb_ring *ring = adapter->rx_ring[i];

		if (frame_size + IGB_TS_HDR_LEN > igb_rx_bufsz(ring)) {
			...
			return -EINVAL;
		}
	}
}

At MTU 1500, frame_size + IGB_TS_HDR_LEN is 1526 + 16 = 1542, which exceeds
IGB_MAX_FRAME_BUILD_SKB (1536 or 1534) and causes igb_xdp_setup() to reject
attaching XDP with -EINVAL. In addition, because igb_xdp_setup() runs this
check even when bpf->prog is NULL, if an XDP program was attached before the
interface was first opened (when ring->flags is 0 and igb_rx_bufsz() returns
2048) or at MTU 3000 before lowering the MTU to 1500, detaching the XDP
program at MTU 1500 also fails with -EINVAL.

Similarly, igb_change_mtu() checks max_frame + IGB_TS_HDR_LEN against
igb_rx_bufsz(ring) using the current ring flags before adapter->max_frame_size
is updated and igb_set_rx_buffer_len() reconfigures the rings:

drivers/net/ethernet/intel/igb/igb_main.c:igb_change_mtu() {
	...
	if (igb_xdp_is_enabled(adapter)) {
		int i;

		for (i = 0; i < adapter->num_rx_queues; i++) {
			struct igb_ring *ring = adapter->rx_ring[i];

			if (max_frame + IGB_TS_HDR_LEN > igb_rx_bufsz(ring)) {
				...
				return -EINVAL;
			}
		}
	}
}

If XDP is attached at MTU <= 1494 (where 1536-byte build_skb buffers are
used), raising the MTU to 1500..3030 is rejected even though MTUs above 1510
would switch the rings to 3072-byte buffers, whereas lowering the MTU from
3000 to 1500 succeeds and leaves XDP active on 1536-byte build_skb buffers
where it can no longer be detached.

Should igb_set_rx_buffer_len() account for IGB_TS_HDR_LEN (or select 3 KiB
buffers when needed for XDP/PTP), and should igb_xdp_setup() skip the frame
size check when detaching (bpf->prog == NULL)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-igb-rx-ts-fix-v4-0-8785124c5d23@aweta.nl?part=2