From: Simon Horman <[email protected]> Date: Fri, 8 May 2026 14:28:29 +0100
> 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 > --- > ice: migrate to netdev ops lock > > This commit migrates queue management operations to use netdev locking. > It updates several NAPI and VSI configuration functions to use their > _locked variants to avoid deadlocks, taking the lock manually where needed. > >> diff --git a/drivers/net/ethernet/intel/ice/ice_dcb_lib.c >> b/drivers/net/ethernet/intel/ice/ice_dcb_lib.c >> index 16aa255351523..7d89c0acc5d8d 100644 >> --- a/drivers/net/ethernet/intel/ice/ice_dcb_lib.c >> +++ b/drivers/net/ethernet/intel/ice/ice_dcb_lib.c > [ ... ] >> @@ -1107,7 +1106,7 @@ ice_dcb_process_lldp_set_mib_change(struct ice_pf *pf, >> >> rtnl_lock(); >> /* disable VSIs affected by DCB changes */ >> - ice_dcb_ena_dis_vsi(pf, false, true); >> + ice_dcb_ena_dis_vsi(pf, false); >> >> ret = ice_query_port_ets(pi, &buf, sizeof(buf), NULL); >> if (ret) { >> @@ -1119,7 +1118,7 @@ ice_dcb_process_lldp_set_mib_change(struct ice_pf *pf, >> ice_pf_dcb_recfg(pf, false); >> >> /* enable previously downed VSIs */ >> - ice_dcb_ena_dis_vsi(pf, true, true); >> + ice_dcb_ena_dis_vsi(pf, true); >> unlock_rtnl: >> rtnl_unlock(); >> out: > > This wasn't introduced by this patch, but does this error path leave VSIs > permanently disabled? > > If ice_query_port_ets() fails, the code jumps to unlock_rtnl, bypassing the > call to ice_dcb_ena_dis_vsi(pf, true). This seems to leave the affected VSIs > (including the main PF interface) in a disabled state (ICE_VSI_DOWN) with > ICE_VSI_NEEDS_RESTART set, which could result in a denial of service. > >> diff --git a/drivers/net/ethernet/intel/ice/ice_eswitch.c >> b/drivers/net/ethernet/intel/ice/ice_eswitch.c >> index 2e4f0969035f7..af0cc77fbf718 100644 >> --- a/drivers/net/ethernet/intel/ice/ice_eswitch.c >> +++ b/drivers/net/ethernet/intel/ice/ice_eswitch.c >> @@ -23,10 +23,16 @@ static int ice_eswitch_setup_env(struct ice_pf *pf) >> struct net_device *netdev = uplink_vsi->netdev; >> bool if_running = netif_running(netdev); >> struct ice_vsi_vlan_ops *vlan_ops; >> + int ret; >> + >> + if (if_running && !test_and_set_bit(ICE_VSI_DOWN, uplink_vsi->state)) { >> + netdev_lock(netdev); >> + ret = ice_down(uplink_vsi); >> + netdev_unlock(netdev); >> >> - if (if_running && !test_and_set_bit(ICE_VSI_DOWN, uplink_vsi->state)) >> - if (ice_down(uplink_vsi)) >> + if (ret) >> return -ENODEV; >> + } > > Simon says: It seems that the topic of locks in this patch has come up > again ¯\_(ツ)_/¯ > > Does manipulating the state bit outside of netdev_lock create a race > condition with interface teardown? > > If the interface is concurrently brought down, ndo_stop (which holds > netdev_lock) would call ice_vsi_close(). Because ICE_VSI_DOWN is already > set by the test_and_set_bit() call above, ice_vsi_close() would skip calling > ice_down() and immediately proceed to free the Tx/Rx rings. > > This would leave the hardware queues active and performing DMA operations into > freed memory, potentially causing a use-after-free. I ran Sashiko internally a couple times and it hasn't found these =\ Anyway, now that I anyway need to adjust the series after .ndo_set_rx_mode_asyn() landed, I'll take a look at this, too. (I hate this series already. I feel like if Sashiko was alive when other vendors were switching to the netdev lock, they'd have the same drama) (I was planning to add devmem/io_uring support to idpf after this lands, but given that idpf is way more confusing and inconsistent in locking, I'm not sure I'll have enough patience :D) Thanks, Olek
