> -----Original Message----- > From: Intel-wired-lan <[email protected]> On Behalf Of Simon > Horman > Sent: Wednesday, April 15, 2026 3:29 PM > To: Loktionov, Aleksandr <[email protected]> > Cc: 'Simon Horman' <[email protected]>; [email protected]; > Nguyen, Anthony L <[email protected]>; [email protected]; > [email protected] > Subject: Re: [Intel-wired-lan] [PATCH iwl-net 3/5] iavf: prevent VSI > corruption > when ring params changed during reset > > From: 'Simon Horman' <[email protected]> > > 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 ||
Tested-by: Rafal Romanowski <[email protected]>
