Re: [PATCH iwl-net 09/10] ice: program multicast magic wake before tearing down the main VSI

[Petr Oros <poros@redhat.com>]

On 10/2/26 15:28, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] ice: Use-After-Free in ice_setup_mc_magic_wake() during concurrent devlink reload
> --
>
> commit 6cf8b519dd4de7f3996192dbff4ffe7afc4d34f8
> Author: Petr Oros <poros@redhat.com>
>
> ice: program multicast magic wake before tearing down the main VSI
>
> This commit moves the ice_setup_mc_magic_wake() call earlier in ice_remove()
> before device structures are torn down. It fixes an issue where WoL magic
> packet wake was not set up correctly because it was being called after
> ice_deinit().
>
>> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
>> index b121faa345c1a..857ba7d92b140 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_main.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_main.c
>> @@ -5465,6 +5465,8 @@ static void ice_remove(struct pci_dev *pdev)
>>   	if (!ice_is_safe_mode(pf))
>>   		ice_remove_arfs(pf);
>>   
>> +	ice_setup_mc_magic_wake(pf);
>> +
>>   	devl_lock(priv_to_devlink(pf));
> [Severity: High]
> By moving ice_setup_mc_magic_wake() outside of devl_lock(), is it exposed to
> concurrent devlink operations like reload?
>
> If a devlink reload operation executes concurrently with the driver's removal
> routine when Wake-on-LAN is enabled, the reload can call ice_unload() and
> ice_decfg_netdev(). This unregisters and frees the netdev.

A devlink reload can not run concurrently with ice_remove(). Since
commit bf6b200bc80d18 ("devlink: Acquire device lock during reload
command") the reload takes the device lock before the devlink instance
lock, and both the PCI unbind path and device_shutdown() hold that same
device lock for the whole of ice_remove().

>
> When ice_setup_mc_magic_wake() runs unprotected by the devlink lock, can it
> experience a use-after-free when attempting to read the freed netdev's MAC
> address?
>
> drivers/net/ethernet/intel/ice/ice_main.c:ice_setup_mc_magic_wake() {
>      ...
> 	if (vsi->netdev)
> 		ether_addr_copy(mac_addr, vsi->netdev->dev_addr);
>      ...
> }
>
> Would it be safer to place the ice_setup_mc_magic_wake() call after acquiring
> the devlink lock?
No, the netdev can not go away underneath ice_setup_mc_magic_wake()
regardless of where the devlink lock is taken in ice_remove().

>
>>   	ice_dealloc_all_dynamic_ports(pf);
>>   	ice_deinit_devlink(pf);
>> @@ -5475,7 +5477,6 @@ static void ice_remove(struct pci_dev *pdev)
>>   	ice_deinit(pf);
>>   	ice_vsi_release_all(pf);
>>   
>> -	ice_setup_mc_magic_wake(pf);
>>   	ice_set_wake(pf);
>>   
>>   	ice_adapter_put(pdev);