Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] igb: XDP attach and detach fail at default 1500 MTU in igb_xdp_setup() --
commit 61b7cd576e8dfdc92f82c2f3538eaf2928658b79 Author: Tjerk Kusters <[email protected]> igb: do not let XDP frames span more than one Rx buffer This commit limits the maximum receive packet length in RLPML and VMOLR to adapter->max_frame_size while XDP is enabled so received frames cannot span multiple Rx buffers. It also accounts for the 16-byte inline Rx timestamp header (IGB_TS_HDR_LEN) when checking if the frame size fits in igb_rx_bufsz() during XDP setup and MTU changes. 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 ce91dda00ec0e..5bf489cd7b066 100644 > --- a/drivers/net/ethernet/intel/igb/igb_main.c > +++ b/drivers/net/ethernet/intel/igb/igb_main.c > @@ -2894,11 +2894,13 @@ static int igb_xdp_setup(struct net_device *dev, > struct netdev_bpf *bpf) > bool running = netif_running(dev); > bool need_reset; > > - /* verify igb ring attributes are sufficient for XDP */ > + /* verify igb ring attributes are sufficient for XDP, leaving room > + * for the inline Rx timestamp header > + */ > for (i = 0; i < adapter->num_rx_queues; i++) { > struct igb_ring *ring = adapter->rx_ring[i]; > > - if (frame_size > igb_rx_bufsz(ring)) { > + if (frame_size + IGB_TS_HDR_LEN > igb_rx_bufsz(ring)) { > NL_SET_ERR_MSG_MOD(bpf->extack, > "The RX buffer size is too small for > the frame size"); > netdev_warn(dev, "XDP RX buffer size %d is too small > for the frame size %d\n", > [ ... ] > @@ -6713,7 +6725,7 @@ static int igb_change_mtu(struct net_device *netdev, > int new_mtu) > for (i = 0; i < adapter->num_rx_queues; i++) { > struct igb_ring *ring = adapter->rx_ring[i]; > > - if (max_frame > igb_rx_bufsz(ring)) { > + if (max_frame + IGB_TS_HDR_LEN > igb_rx_bufsz(ring)) { > netdev_warn(adapter->netdev, > "Requested MTU size is not > supported with XDP. Max frame size is %d\n", > max_frame); [Severity: High] Does adding IGB_TS_HDR_LEN to the igb_rx_bufsz(ring) check in igb_xdp_setup() and igb_change_mtu() break XDP at the default MTU of 1500 on systems with PAGE_SIZE < 8192? In drivers/net/ethernet/intel/igb/igb.h, the constants are: IGB_ETH_PKT_HDR_PAD = ETH_HLEN + ETH_FCS_LEN + (VLAN_HLEN * 2) = 26 IGB_TS_HDR_LEN = 16 IGB_MAX_FRAME_BUILD_SKB = IGB_RXBUFFER_1536 - NET_IP_ALIGN (1536 or 1534) At the default MTU of 1500, frame_size (and adapter->max_frame_size) is 1500 + 26 = 1526. When the interface is opened via igb_open() -> igb_configure() -> igb_configure_rx() -> 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 1526 <= IGB_MAX_FRAME_BUILD_SKB, ring_uses_large_buffer(ring) is not set while ring_uses_build_skb(ring) is set, so igb_rx_bufsz(ring) returns IGB_MAX_FRAME_BUILD_SKB (1536 or 1534): static inline unsigned int igb_rx_bufsz(struct igb_ring *ring) { #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; } When igb_xdp_setup() or igb_change_mtu() then checks: if (frame_size + IGB_TS_HDR_LEN > igb_rx_bufsz(ring)) 1526 + 16 = 1542 > 1536, causing XDP attach at MTU 1500 to fail with -EINVAL on a running interface. Furthermore, because igb_xdp_setup() runs this check even when bpf->prog is NULL, if an XDP program was attached while the interface was down (when ring->flags is 0 and igb_rx_bufsz(ring) returns 2048) and the interface is then brought up at MTU 1500, subsequent attempts to detach or replace the XDP program at MTU 1500 will also fail with -EINVAL. Also, in 2 KiB build_skb mode, igb_skb_pad() computes the buffer offset as SKB_WITH_OVERHEAD(IGB_RXBUFFER_2048) - IGB_MAX_FRAME_BUILD_SKB, leaving only IGB_MAX_FRAME_BUILD_SKB (1536) bytes before skb_shared_info. If a 1526-byte frame arrives with a 16-byte inline timestamp, wouldn't the 1542 bytes written by the DMA engine overflow into skb_shared_info unless 3 KiB buffers are selected or IGB_MAX_FRAME_BUILD_SKB accounts for IGB_TS_HDR_LEN? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
