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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [Medium] igb: PF MAC filter clobber in igb_enable_sriov() before XSK
  queue check
- [Medium] igb: skipped PF reinit in igb_disable_sriov() when
  igb_sriov_reinit() fails
- [Medium] igb: unlocked VF state and flag rollback in
  igb_enable_sriov() on error
- [Medium] igb: stale real queue counts in __igb_open() break
  igb_xsk_pool() check
--

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

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

This commit programs standard 2 KiB or 3 KiB hardware Rx buffer sizes for
AF_XDP zero-copy rings in igb_setup_srrctl() so frames are not split across
multiple descriptors, and rejects XSK pool bindings, MTU changes, and SR-IOV
queue reductions when frames or bound queues would no longer fit.

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;
> +		}
> +	}
> +
>  	if (netif_running(netdev))
>  		igb_close(netdev);
>  	else

[Severity: Medium]
When igb_enable_sriov() fails here with -EBUSY, does returning before
igb_close() or igb_reset() wipe active PF unicast MAC filters in the upper
RAR slots?

Before calling igb_sriov_reinit(), igb_enable_sriov() calls
igb_vf_configure() -> igb_set_vf_mac() with a zero MAC address for each VF,
which overwrites the top adapter->vfs_allocated_count entries of
adapter->mac_table, marks them IGB_MAC_STATE_IN_USE, and clears E1000_RAH_AV
in the hardware RAR registers:

drivers/net/ethernet/intel/igb/igb_main.c:igb_set_vf_mac() {
    ...
	int rar_entry = hw->mac.rar_entry_count - (vf + 1);
	unsigned char *vf_mac_addr = adapter->vf_data[vf].vf_mac_addresses;

	ether_addr_copy(vf_mac_addr, mac_addr);
	ether_addr_copy(adapter->mac_table[rar_entry].addr, mac_addr);
	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);
    ...
}

Previously igb_sriov_reinit() always ran igb_close() or igb_reset(), which
flushed the MAC table and unsynced netdev->uc via __dev_uc_unsync() before
re-syncing PF filters.  Returning early leaves netdev->uc marked synced while
any PF unicast filters (such as macvlan or bridge FDB entries) that occupied
those upper RAR slots are disabled in hardware and leaked as in-use until the
next reset.

[Severity: Medium]
Can this check in igb_sriov_reinit() also return -EBUSY when called from
igb_disable_sriov(), after SR-IOV has already been torn down?

igb_disable_sriov() calls pci_disable_sriov(), frees adapter->vf_data, resets
adapter->vfs_allocated_count to 0, and writes E1000_IOVCTL_REUSE_VFQ before
calling igb_sriov_reinit().  If min(rxq, txq) < end (for example, if CPUs
were offlined so num_online_cpus() is smaller than real_num_rx_queues, or if
VFs were enabled while the interface was down) and an XSK pool is bound to a
queue in that range, igb_sriov_reinit() aborts and leaves the PF running
without reinitializing its queue configuration and interrupt scheme for
non-SR-IOV operation.

> @@ -3844,6 +3864,7 @@ static int igb_enable_sriov(struct pci_dev *pdev, int num_vfs, bool reinit)
>  {
>  	struct net_device *netdev = pci_get_drvdata(pdev);
>  	struct igb_adapter *adapter = netdev_priv(netdev);
> +	bool dmac = adapter->flags & IGB_FLAG_DMAC;
>  	int old_vfs = pci_num_vf(pdev);
>  	struct vf_mac_filter *mac_list;
>  	int err = 0;
> @@ -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;
> +	if (dmac)
> +		adapter->flags |= IGB_FLAG_DMAC;
>  out:
>  	return err;
>  }

[Severity: Medium]
When igb_sriov_reinit() returns -EBUSY on a live PF, can the unlocked
rollback in err_out race with concurrent readers of adapter->vf_data and
adapter->flags?

CPU0                              CPU1
igb_enable_sriov()
  adapter->vfs_allocated_count = num_vfs;
  adapter->vf_data = kzalloc_objs(...);
  ...
  igb_sriov_reinit()
    rtnl_unlock()
    return -EBUSY;
                                  igb_set_rx_mode()
                                    igb_restore_vf_multicasts()
                                      i < adapter->vfs_allocated_count
  kfree(adapter->vf_data);
  adapter->vf_data = NULL;
                                      &adapter->vf_data[i]
                                        <-- NULL pointer dereference / UAF
  adapter->vfs_allocated_count = 0;

igb_enable_sriov() runs without rtnl_lock() or adapter->vfs_lock, and frees
adapter->vf_data before clearing adapter->vfs_allocated_count (unlike
igb_disable_sriov(), which takes adapter->vfs_lock).  In addition, saving and
restoring IGB_FLAG_DMAC in adapter->flags performs unlocked read-modify-write
updates that can race with RTNL-protected changes to adapter->flags.  Checking
for bound XSK queues before mutating adapter state in igb_enable_sriov()
would avoid having to roll back live VF state and RAR entries on error.

[ ... ]

> @@ -6778,6 +6805,7 @@ static int igb_change_mtu(struct net_device *netdev, int new_mtu)
>  {
>  	struct igb_adapter *adapter = netdev_priv(netdev);
>  	int max_frame = new_mtu + IGB_ETH_PKT_HDR_PAD;
> +	int i;
>  
>  	if (igb_xdp_is_enabled(adapter) &&
>  	    !igb_xdp_frame_fits(adapter, max_frame)) {
> @@ -6791,6 +6819,23 @@ static int igb_change_mtu(struct net_device *netdev, int new_mtu)
>  	if (max_frame < (ETH_FRAME_LEN + ETH_FCS_LEN))
>  		max_frame = ETH_FRAME_LEN + ETH_FCS_LEN;
>  
> +	/* the frame must fit the buffers of a bound XSK pool, also while no
> +	 * XDP program is attached, as attaching one does not check it again
> +	 */
> +	for (i = 0; i < adapter->num_rx_queues; i++) {
> +		struct xsk_buff_pool *pool = xsk_get_pool_from_qid(netdev, i);
> +
> +		if (!pool || !pool->dev)
> +			continue;
> +
> +		if (!igb_xsk_frame_fits(pool, max_frame)) {
> +			netdev_warn(adapter->netdev,
> +				    "Requested MTU size is not supported with AF_XDP. Frame size is %d\n",
> +				    max_frame);
> +			return -EINVAL;
> +		}
> +	}
> +
>  	while (test_and_set_bit(__IGB_RESETTING, &adapter->state))
>  		usleep_range(1000, 2000);
>  
> 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: Medium]
When a hidden queue is set up again during __igb_open(), does
xsk_get_pool_from_qid() in igb_xsk_pool() return NULL before reaching
igb_xsk_frame_fits()?

In __igb_open(), igb_configure() sets up the Tx and Rx rings (calling
igb_xsk_pool()) before netif_set_real_num_tx_queues() and
netif_set_real_num_rx_queues() update the netdev queue counts:

drivers/net/ethernet/intel/igb/igb_main.c:__igb_open() {
    ...
	igb_configure(adapter);

	err = igb_request_irq(adapter);
	if (err)
		goto err_req_irq;

	/* Notify the stack of the actual queue counts. */
	err = netif_set_real_num_tx_queues(adapter->netdev,
					   adapter->num_tx_queues);
	if (err)
		goto err_set_queues;

	err = netif_set_real_num_rx_queues(adapter->netdev,
					   adapter->num_rx_queues);
    ...
}

Because xsk_get_pool_from_qid() checks queue_id against
netdev->real_num_rx_queues and netdev->real_num_tx_queues, it still sees the
reduced queue count (such as 1 after an MSI fallback) during igb_configure()
and returns NULL for any restored queue >= 1.  As a result, the restored
queue stays out of zero-copy mode even when the MTU fits, and for the same
reason igb_change_mtu() can miss bound pools if queue counts were changed
while the interface was down and real_num_rx_queues has not been updated yet.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111-igb-rx-ts-fix-v6-0-ba5b9903859c@aweta.nl?part=3