Re: [PATCH iwl-net 3/5] iavf: prevent VSI corruption when ring params changed during reset

[Simon Horman <horms@kernel.org>]

From: 'Simon Horman' <horms@kernel.org>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev
---
iavf: prevent VSI corruption when ring params changed during reset

Changing ring parameters via ethtool triggers a VF reset and queue
reconfiguration. If ethtool is called again before the first reset
completes, the second reset races with uninitialised queue state and
can corrupt the VSI resource tree on the PF side. Return -EAGAIN from
iavf_set_ringparam() when the adapter is already resetting or its queues
are disabled.

> diff --git a/drivers/net/ethernet/intel/iavf/iavf_ethtool.c b/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
> index 1cd1f3f2930a0..390913170315f 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
> @@ -495,6 +495,11 @@ static int iavf_set_ringparam(struct net_device *netdev,
>  	if ((ring->rx_mini_pending) || (ring->rx_jumbo_pending))
>  		return -EINVAL;
>  
> +	if (adapter->state == __IAVF_RESETTING ||
> +	    (adapter->state == __IAVF_RUNNING &&
> +	     adapter->flags & IAVF_FLAG_QUEUES_DISABLED))
> +		return -EAGAIN;
> +

Is there a lockless Time-Of-Check to Time-Of-Use race here?

iavf_set_ringparam() is called from ethtool under rtnl_lock(), but the
background iavf_reset_task() synchronizes using netdev_lock(). Because iavf
does not set netdev->request_ops_lock = true, the ethtool core does not
acquire netdev_lock() for iavf_set_ringparam().

This allows adapter->state to transition to __IAVF_RESETTING immediately
after this lockless check.

Furthermore, calling iavf_reset_step() later in this function without
holding netdev_lock() allows the hardware reset sequence to execute
concurrently from both the ethtool thread and the workqueue, which could
lead to VSI resource corruption.

Could this open-coded state check miss pending resets?

If a reset is scheduled (e.g., by a watchdog timeout setting
IAVF_FLAG_RESET_NEEDED) but the worker task hasn't started yet,
adapter->state will still be __IAVF_RUNNING. This check will pass, and
iavf_set_ringparam() will proceed to mutate ring parameters and execute
iavf_reset_step(), racing with the previously scheduled iavf_reset_task.

Can this be addressed by also checking for IAVF_FLAG_RESET_NEEDED and
IAVF_FLAG_RESET_PENDING?

Also, the commit message notes that triggering a VF reset before the first
one completes causes VSI resource tree corruption. Both iavf_set_channels()
and iavf_change_mtu() also dynamically reconfigure queues and
unconditionally trigger resets by calling iavf_reset_step(adapter).

Should these functions be updated to include a similar state check to
prevent the same VSI resource tree corruption on the PF side?

>  	if (ring->tx_pending > IAVF_MAX_TXD ||
>  	    ring->tx_pending < IAVF_MIN_TXD ||
>  	    ring->rx_pending > IAVF_MAX_RXD ||