Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] ice: TOCTOU NULL dereference on cdev_info->adev in ice_set_channels() --
commit 41ef61e89b925d28893681cf06722f3b9581b193 Author: Petr Oros <[email protected]> ice: do not block on the aux device lock in ethtool set_channels This commit replaces device_lock() with device_trylock() in ice_set_channels() to avoid a lock inversion between the RTNL lock and the auxiliary device lock during RDMA auxiliary device probe, returning -EBUSY when the lock is contended. > diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c > b/drivers/net/ethernet/intel/ice/ice_ethtool.c > index aa87d4f6324e8..976402b6836de 100644 > --- a/drivers/net/ethernet/intel/ice/ice_ethtool.c > +++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c > @@ -3947,7 +3947,11 @@ static int ice_set_channels(struct net_device *dev, > struct ethtool_channels *ch) > > if (pf->cdev_info && pf->cdev_info->adev) { > mutex_lock(&pf->adev_mutex); > - device_lock(&pf->cdev_info->adev->dev); > + if (!device_trylock(&pf->cdev_info->adev->dev)) { > + mutex_unlock(&pf->adev_mutex); > + netdev_dbg(dev, "RDMA auxiliary device is busy, try > again\n"); > + return -EBUSY; > + } > locked = true; > if (pf->cdev_info->adev->dev.driver) { > netdev_err(dev, "Cannot change channels when RDMA is > active\n"); [Severity: High] This problem wasn't introduced by this patch, but can pf->cdev_info->adev become NULL between the unlocked check in ice_set_channels() and the device_trylock(&pf->cdev_info->adev->dev) call? Paths that do not hold rtnl_lock, such as ice_service_task() handling ICE_FLAG_UNPLUG_AUX_DEV, ice_prepare_for_reset(), or ice_devlink_enable_roce_set() and ice_devlink_enable_iw_set(), can run concurrently and call ice_unplug_aux_dev(): drivers/net/ethernet/intel/ice/ice_idc.c:ice_unplug_aux_dev() { ... mutex_lock(&pf->adev_mutex); adev = pf->cdev_info->adev; pf->cdev_info->adev = NULL; mutex_unlock(&pf->adev_mutex); auxiliary_device_delete(adev); auxiliary_device_uninit(adev); } If ice_unplug_aux_dev() clears pf->cdev_info->adev after ice_set_channels() checks it but before ice_set_channels() acquires pf->adev_mutex, device_trylock(&pf->cdev_info->adev->dev) will dereference a NULL pointer (and conversely, if ice_plug_aux_dev() assigns cdev->adev concurrently, the RDMA active check could be skipped). In contrast, ice_send_event_to_aux() checks cdev->adev after acquiring pf->adev_mutex: drivers/net/ethernet/intel/ice/ice_idc.c:ice_send_event_to_aux() { ... mutex_lock(&pf->adev_mutex); if (!cdev->adev) goto finish; if (!device_trylock(&cdev->adev->dev)) { ... } Should ice_set_channels() likewise acquire pf->adev_mutex before testing pf->cdev_info->adev? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
