Re: [PATCH iwl-net 2/3] ice: do not block on the aux device lock in ethtool set_channels

[sashiko-bot@kernel.org]

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 <poros@redhat.com>

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/20261019065301.3983537-1-poros@redhat.com?part=2