Re: [PATCH net v4 1/2] igb: do not let XDP frames span more than one Rx buffer

[sashiko-bot@kernel.org]

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 <tkusters@aweta.nl>

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/20261028083344.3047350-3-p@1g4.org/

> 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/20261102-igb-rx-ts-fix-v4-0-8785124c5d23@aweta.nl?part=1