Re: [PATCH net v6 3/3] igb: do not split AF_XDP zero-copy frames over several Rx buffers

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential
issue(s) to consider.

Critical: 0 · High: 1 (1) · Medium: 1 (1) · Low: 3

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The new XSK queue check sits in igb_sriov_reinit(), and that
  function is shared by the SR-IOV enable and disable paths.
- [Medium] The new -EBUSY return from igb_sriov_reinit() gives userspace
  a deterministic way into igb_enable_sriov()'s err_out path while the…
- [Low] This patch raises the XSK ring hardware buffer in
  igb_setup_srrctl() to 2 KiB or 3 KiB, while the XSK frame can be as…
- [Low] The new -EBUSY refusal in igb_sriov_reinit() runs after
  igb_enable_sriov() has already called igb_vf_configure() for each VF.
- [Low] The new restore `if (dmac) adapter->flags |= IGB_FLAG_DMAC;` in
  igb_enable_sriov() err_out is a non-atomic read-modify-write of…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but it sits in
  igb_xsk_pool(), which the patch edits.
- [Medium] This problem wasn't introduced by this patch.

--

igb: do not split AF_XDP zero-copy frames over several Rx buffers

AF_XDP zero-copy Rx rings now get the same 2 KiB or 3 KiB SRRCTL buffer
size as the other rings, so frames are no longer split over two
buffers. The patch adds checks that refuse an XSK pool, an MTU change,
or VF enablement when a bound pool's frame size or queue would no
longer fit. When the frame does not fit at ring setup, igb_xsk_pool()
falls back to copy mode.

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 e787095d3cdbe..c9351a8c4f5e5 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -3777,9 +3777,29 @@ static int igb_sriov_reinit(struct pci_dev *dev)
>  	struct net_device *netdev = pci_get_drvdata(dev);
>  	struct igb_adapter *adapter = netdev_priv(netdev);
>  	struct pci_dev *pdev = adapter->pdev;
> +	unsigned int i, rxq, txq, end;
>  
>  	rtnl_lock();
>  
> +	/* With VFs the PF gets fewer Rx queues and a single Tx queue, see
> +	 * igb_set_interrupt_capability(). Refuse that while an XSK pool is
> +	 * bound to a queue the PF would lose; the core refuses ethtool -L in
> +	 * the same case.
> +	 */
> +	rxq = min_t(unsigned int, igb_get_max_rss_queues(adapter),
> +		    num_online_cpus());
> +	txq = adapter->vfs_allocated_count ? 1 : rxq;
> +	end = max(netdev->real_num_rx_queues, netdev->real_num_tx_queues);
> +	for (i = min(rxq, txq); i < end; i++) {
> +		if (xsk_get_pool_from_qid(netdev, i)) {
> +			rtnl_unlock();
> +			dev_err(&pdev->dev,
> +				"Cannot change the number of queues, an AF_XDP socket is bound to queue %u\n",
> +				i);
> +			return -EBUSY;
> +		}
> +	}

[Severity: High]
igb_sriov_reinit() is shared by igb_enable_sriov() and
igb_disable_sriov(). Can this check also fire on the disable path? The
commit message only describes refusing the enable case.

igb_disable_sriov() calls igb_sriov_reinit() only after the VFs have
already been torn down:

igb_disable_sriov() {
    ...
		pci_disable_sriov(pdev);
    ...
		kfree(adapter->vf_data);
		adapter->vf_data = NULL;
		adapter->vfs_allocated_count = 0;
    ...
		wr32(E1000_IOVCTL, E1000_IOVCTL_REUSE_VFQ);
    ...
	return reinit ? igb_sriov_reinit(pdev) : 0;
}

One possible path is an i350 with the interface down.
real_num_rx_queues and real_num_tx_queues stay at 8 from
alloc_etherdev_mq(). Enable VFs, then bind a copy-mode XSK to queue 5.
The loop doesn't check pool->dev, so copy-mode pools count too. On a
host with fewer than 8 CPUs, writing 0 to sriov_numvfs then lands queue
5 inside [min(rxq, txq), end) and returns -EBUSY, even though the VFs
are already gone. On 82576, ethtool -L combined 2 on a 1-CPU host, or
offlining a CPU, gives a similar range.

The -EBUSY return skips igb_clear_interrupt_scheme(),
igb_init_queue_configuration() and igb_init_interrupt_scheme(). The PF
keeps a single Tx queue. The rings also keep the reg_idx offset that
igb_cache_ring_register() computed from the old vfs_allocated_count,
while vfs_allocated_count is now 0.

Would the next igb_configure() then program non-VT MRQC that steers to
hardware queue 0, while the only PF Rx ring sits at the old register
index, so PF Rx stops?

Could this check run only on the enable path, ideally at the start of
igb_enable_sriov() before any VF state is set up?

[ ... ]

> @@ -3933,6 +3954,8 @@ static int igb_enable_sriov(struct pci_dev *pdev, int num_vfs, bool reinit)
>  	kfree(adapter->vf_data);
>  	adapter->vf_data = NULL;
>  	adapter->vfs_allocated_count = 0;

[Severity: Medium]
The new -EBUSY return lets userspace reliably reach this err_out
teardown while the PF is still up. The check runs before igb_close(),
so the watchdog and rx_mode keep running. Here rtnl has already been
dropped inside igb_sriov_reinit(), and vfs_lock is not taken.

Some readers walk vf_data without either lock, for example
igb_watchdog_task()->igb_ping_all_vfs():

	for (i = 0 ; i < adapter->vfs_allocated_count; i++) {
		ping = E1000_PF_CONTROL_MSG;
		if (adapter->vf_data[i].flags & IGB_VF_FLAG_CTS)

igb_set_rx_mode()->igb_restore_vf_multicasts() does the same under
addr_list_lock only. rtnl-only VF ops such as igb_ndo_set_vf_mac() also
write vf_data[vf].

vf_data is set to NULL before vfs_allocated_count is cleared. Can a
concurrent reader see a non-zero count and either use the freed
vf_data or dereference NULL plus an offset?

This unsynchronized reader pattern also exists against
igb_disable_sriov(), but this patch adds a user-triggerable way into
it. Moving the XSK check before vfs_allocated_count and vf_data are
published would avoid that.

[Severity: Low]
The commit message says the MAC filter entries reserved for the VFs
stay reserved until the next reset. They come from
igb_vf_configure()->igb_set_vf_mac(), which runs before
igb_sriov_reinit():

	adapter->mac_table[rar_entry].queue = vf;
	adapter->mac_table[rar_entry].state |= IGB_MAC_STATE_IN_USE;
	igb_rar_set_index(adapter, rar_entry);

err_out does not release them. With vfs_allocated_count back at 0, the
PF loses up to num_vfs - 1 unicast filter slots.

If the check ran at the top of igb_enable_sriov() (the PF queue limit
can be derived from num_vfs), would this leak and the disable-path
problem above both go away?

[Severity: Medium]
This isn't a bug introduced by this patch, but when igb_sriov_reinit()
succeeds and pci_enable_sriov() then fails:

	if (!old_vfs) {
		err = pci_enable_sriov(pdev, adapter->vfs_allocated_count);
		if (err)
			goto err_out;
	}

err_out clears vfs_allocated_count without running igb_sriov_reinit()
again. The interrupt scheme, the ring reg_idx offsets and the VT
register programming all stay in VF mode while software believes there
are no VFs.

The new DMAC restore matches the intended no-VF state and does not
make this worse. Should err_out redo the reinit in this case?

> +	if (dmac)
> +		adapter->flags |= IGB_FLAG_DMAC;

[Severity: Low]
This is a non-atomic read-modify-write of adapter->flags from sysfs
context, and the interface is still up after the -EBUSY return.

igb_watchdog_task() updates the same word
(IGB_FLAG_NEED_LINK_UPDATE, IGB_FLAG_MEDIA_RESET, IGB_FLAG_EEE), and so
does igb_set_rx_mode() (IGB_FLAG_VLAN_PROMISC) under addr_list_lock.

Can an overlapping update lose either the DMAC bit or the other
context's bit? The earlier "adapter->flags &= ~IGB_FLAG_DMAC" in this
function already follows the same pattern.

>  out:
>  	return err;
>  }

[ ... ]

> @@ -4836,9 +4859,13 @@ void igb_setup_srrctl(struct igb_adapter *adapter, struct igb_ring *ring)
>  	u32 srrctl = 0;
>  	u32 buf_size;
>  
> -	if (ring->xsk_pool)
> -		buf_size = xsk_pool_get_rx_frame_size(ring->xsk_pool);
> -	else if (ring_uses_large_buffer(ring))
> +	/* SRRCTL.BSIZEPKT has a 1 KiB granularity, so programming the XSK
> +	 * frame size would round it down and split frames over several
> +	 * buffers. XSK rings use the same buffer size as the other rings.
> +	 * While XDP is attached, the receive length limit keeps frames
> +	 * within the XSK frame, see igb_xsk_frame_fits().
> +	 */
> +	if (ring_uses_large_buffer(ring))
>  		buf_size = IGB_RXBUFFER_3072;
>  	else
>  		buf_size = IGB_RXBUFFER_2048;

[Severity: Low]
This is only safe together with the receive length limit from the
earlier patch. The commit message says:

    This depends on "igb: do not let XDP frames span more than one Rx
    buffer" ... Without it, the hardware can write past the XSK frame.

The two patches carry different Fixes: tags (9cbc948b5a20 and
2c6196013f84), and the Cc: stable tag here has no prerequisite note. If
a stable tree takes this patch without the prerequisite, could the NIC
DMA up to 2 KiB or 3 KiB into a 1792-byte XSK frame and overrun the
next UMEM chunk?

Would a stable prerequisite annotation on the Cc: stable line make the
dependency explicit?

> diff --git a/drivers/net/ethernet/intel/igb/igb_xsk.c b/drivers/net/ethernet/intel/igb/igb_xsk.c
> index ce4a7b58cad2f..7f99e834c1edb 100644
> --- a/drivers/net/ethernet/intel/igb/igb_xsk.c
> +++ b/drivers/net/ethernet/intel/igb/igb_xsk.c
> @@ -91,7 +91,21 @@ struct xsk_buff_pool *igb_xsk_pool(struct igb_adapter *adapter,
>  	if (!igb_xdp_is_enabled(adapter))
>  		return NULL;
>  
> -	return (pool && pool->dev) ? pool : NULL;
> +	if (!pool || !pool->dev)
> +		return NULL;
> +
> +	/* igb_change_mtu() does not see a pool on a queue that is hidden, for
> +	 * example after a fallback from MSI-X to MSI. The hardware buffer can
> +	 * be larger than the XSK frame, so check again when the ring is set up.
> +	 */
> +	if (!igb_xsk_frame_fits(pool, adapter->max_frame_size)) {
> +		netdev_warn_once(adapter->netdev,
> +				 "XSK frame size is too small for the MTU, queue %d is not used in zero-copy mode\n",
> +				 qid);
> +		return NULL;
> +	}
> +
> +	return pool;
>  }

[Severity: High]
This is a pre-existing issue that was not introduced by this patch.
igb_xsk_pool() picks the pool from ring->queue_index alone.
igb_setup_desc_rings() in igb_ethtool.c never sets
test_rx_ring.queue_index, so it stays 0. It then calls
igb_configure_rx_ring(), which sets ring->xsk_pool from
igb_xsk_pool().

Suppose XDP is attached and a zero-copy pool is bound to queue 0. Then
ethtool -t ethX offline takes this path:

igb_diag_test()
  igb_loopback_test()
    igb_setup_desc_rings()
      igb_configure_rx_ring()   test ring gets the queue 0 pool
      igb_alloc_rx_buffers()    fills igb_rx_buffer page entries
    ...
    igb_free_desc_rings()
      igb_free_rx_resources()
        igb_clean_rx_ring()
          igb_clean_rx_ring_zc()

igb_clean_rx_ring_zc() then does:

	struct xdp_buff *xdp = rx_ring->rx_buffer_info_zc[ntc];

	xsk_buff_free(xdp);

Can this call xsk_buff_free() on words of the igb_rx_buffer array (DMA
addresses, struct page pointers) as if they were struct xdp_buff
pointers?

The test's pages would also leak and stay DMA-mapped. The live pool's
rxq info also gets pointed at the test ring's xdp_rxq. The new
netdev_warn_once() above can also fire for this test ring.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261111-igb-rx-ts-fix-v6-0-ba5b9903859c%40aweta.nl