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

[Petr Oros]

On 10/2/26 15:28, sashiko-...@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 <po...@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);