Re: [PATCH net-next v2 02/14] ice: use shared devlink to store ice_adapters instead of custom xarray

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] ice: DMA UAF due to missing control queue teardown in recovery mode
- [High] ice: NULL pointer deref in ice_adapter_put() on recovery mode teardown
--

commit 298f011950fb287bfb913324af33c969b5683c8d
Author: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Subject: ice: use shared devlink to store ice_adapters instead of custom xarray

This commit refactors the storage and deduplication logic for ice_adapters
by moving it to a shared devlink instance. It replaces the custom xarray
implementation with nested devlink instances for PF devices.

Link: https://lore.kernel.org/all/20260312100407.551173-1-jiri@resnulli.us

> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
> index 454fba07d4ecc..6373cd5ac2884 100644
> --- a/drivers/net/ethernet/intel/ice/ice_main.c
> +++ b/drivers/net/ethernet/intel/ice/ice_main.c

[ ... ]

> @@ -4946,7 +4946,12 @@ static int ice_init_devlink(struct ice_pf *pf)
>  		return err;
>  
>  	ice_devlink_init_regions(pf);
> -	ice_devlink_register(pf);
> +	err = ice_devlink_register(pf);
> +	if (err) {
> +		ice_devlink_destroy_regions(pf);
> +		ice_devlink_unregister_params(pf);
> +		return err;
> +	}

[Severity: High]
Can this new error path lead to a DMA use-after-free regression in recovery
mode?

In ice_probe_recovery_mode(), the hardware control queues are created via
ice_create_all_ctrlq(), which allocates DMA memory using devres. If
ice_init_devlink() fails and returns an error here, the function exits
without calling ice_destroy_all_ctrlq().

The devres framework will unmap and free the DMA memory upon probe failure,
but the hardware ATQ/ARQ registers might still point to those freed
physical addresses since the queues were never gracefully disabled. Could
this allow the active device to continue performing DMA operations on freed
memory?

[ ... ]

> diff --git a/drivers/net/ethernet/intel/ice/ice_adapter.c b/drivers/net/ethernet/intel/ice/ice_adapter.c
> index 2dc3629d6d0f6..c6301341ccaef 100644
> --- a/drivers/net/ethernet/intel/ice/ice_adapter.c
> +++ b/drivers/net/ethernet/intel/ice/ice_adapter.c

[ ... ]

> @@ -93,59 +80,37 @@ static void ice_adapter_free(struct ice_adapter *adapter)

[ ... ]

> -void ice_adapter_put(struct pci_dev *pdev)
> +void ice_adapter_put(struct ice_adapter *adapter)
>  {
> -	struct ice_adapter *adapter;
> -	unsigned long index;
> -
> -	index = ice_adapter_xa_index(pdev);
> -	scoped_guard(mutex, &ice_adapters_mutex) {
> -		adapter = xa_load(&ice_adapters, index);
> -		if (WARN_ON(!adapter))
> -			return;
> -		if (!refcount_dec_and_test(&adapter->refcount))
> -			return;
> -
> -		WARN_ON(xa_erase(&ice_adapters, index) != adapter);
> -	}
> -	ice_adapter_free(adapter);
> +	devlink_shd_put(adapter->devlink);
>  }

[Severity: High]
Will this trigger a NULL pointer dereference regression if the driver is
probed in recovery mode?

If the driver probes in recovery mode, pf->adapter is never allocated and
remains NULL. During ice_remove(), the code checks
ice_is_recovery_mode(&pf->hw) to determine if it should perform a
recovery-mode early return or a full teardown.

Since ice_is_recovery_mode() reads a live volatile hardware register, if
the firmware clears this bit after probe, the check bypasses the early
return and falls through to normal teardown:

drivers/net/ethernet/intel/ice/ice_main.c:ice_remove() {
    ...
    ice_adapter_put(pf->adapter);
    ice_deinit_hw(&pf->hw);
    ...
}

The previous xarray lookup safely returned early on NULL, but the new code
unconditionally dereferences adapter->devlink. Will this crash on driver
unload or device unbind when transitioning out of recovery mode?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009121433.30347-1-przemyslaw.kitszel@intel.com?part=2