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 <[email protected]> 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/[email protected]/ > 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/[email protected]?part=2
