| Message ID | 20261002130752.2597259-10-poros@redhat.com |
|---|---|
| State | Under Review |
| Delegated to: | Anthony Nguyen |
| Headers | show |
| Series | ice: port missing i40e fixes | expand |
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. 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? > 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);
On October 2, 2026 3:07:51 PM GMT+02:00, Petr Oros <poros@redhat.com> wrote: >Since commit 5b246e533d01 ("ice: split probe into smaller functions") >ice_remove() calls ice_setup_mc_magic_wake() after ice_deinit(), which >frees pf->vsi. ice_get_main_vsi() then returns NULL and the function >returns before it sends the Manage MAC Write command, so multicast magic >packet wake and keeping a locally administered address across the PF >reset are never set up on shutdown with WoL enabled. > >Program it before the netdev and the VSIs are torn down, as it was done >before that commit, so the current netdev address is used again. > >i40e had a similar problem, where the multicast magic wake setup ran >after the admin queue had already been shut down, and fixed it in >commit e661414c98df ("i40e: Remove duplicated prepare call in >i40e_shutdown"). > >Fixes: 5b246e533d01 ("ice: split probe into smaller functions") >Assisted-by: LLM >Signed-off-by: Petr Oros <poros@redhat.com> >--- > drivers/net/ethernet/intel/ice/ice_main.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > >diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c >index ceb9fec2af21e7..a885b3e0baaa5d 100644 >--- a/drivers/net/ethernet/intel/ice/ice_main.c >+++ b/drivers/net/ethernet/intel/ice/ice_main.c >@@ -5456,6 +5456,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)); > ice_dealloc_all_dynamic_ports(pf); > ice_deinit_devlink(pf); >@@ -5466,7 +5468,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); Reviewed-by: Ivan Vecera <ivecera@redhat.com>
> -----Original Message----- > From: Petr Oros <poros@redhat.com> > Sent: Friday, October 2, 2026 3:08 PM > To: netdev@vger.kernel.org > Cc: Oros, Petr <poros@redhat.com>; Nguyen, Anthony L > <anthony.l.nguyen@intel.com>; Kitszel, Przemyslaw > <przemyslaw.kitszel@intel.com>; Andrew Lunn <andrew+netdev@lunn.ch>; > David S. Miller <davem@davemloft.net>; Eric Dumazet > <edumazet@kernel.org>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni > <pabeni@redhat.com>; Lobakin, Aleksander > <aleksander.lobakin@intel.com>; Alexei Starovoitov <ast@kernel.org>; > Daniel Borkmann <daniel@iogearbox.net>; Jesper Dangaard Brouer > <hawk@kernel.org>; John Fastabend <john.fastabend@gmail.com>; > Stanislav Fomichev <sdf@fomichev.me>; Henry Tieman > <henry.w.tieman@intel.com>; Anirudh Venkataramanan > <anirudh.venkataramanan@intel.com>; Michal Swiatkowski > <michal.swiatkowski@linux.intel.com>; Jesse Brandeburg > <jbrandeb@kernel.org>; Preethi Banala <preethi.banala@intel.com>; > Kiran Patil <kiran.patil@intel.com>; Dan Nowlin > <dan.nowlin@intel.com>; Stephen Hemminger > <stephen@networkplumber.org>; intel-wired-lan@lists.osuosl.org; linux- > kernel@vger.kernel.org; bpf@vger.kernel.org > Subject: [PATCH iwl-net 09/10] ice: program multicast magic wake > before tearing down the main VSI > > Since commit 5b246e533d01 ("ice: split probe into smaller functions") > ice_remove() calls ice_setup_mc_magic_wake() after ice_deinit(), which > frees pf->vsi. ice_get_main_vsi() then returns NULL and the function > returns before it sends the Manage MAC Write command, so multicast > magic packet wake and keeping a locally administered address across > the PF reset are never set up on shutdown with WoL enabled. > > Program it before the netdev and the VSIs are torn down, as it was > done before that commit, so the current netdev address is used again. > > i40e had a similar problem, where the multicast magic wake setup ran > after the admin queue had already been shut down, and fixed it in > commit e661414c98df ("i40e: Remove duplicated prepare call in > i40e_shutdown"). > > Fixes: 5b246e533d01 ("ice: split probe into smaller functions") > Assisted-by: LLM > Signed-off-by: Petr Oros <poros@redhat.com> > --- > drivers/net/ethernet/intel/ice/ice_main.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/intel/ice/ice_main.c > b/drivers/net/ethernet/intel/ice/ice_main.c > index ceb9fec2af21e7..a885b3e0baaa5d 100644 > --- a/drivers/net/ethernet/intel/ice/ice_main.c > +++ b/drivers/net/ethernet/intel/ice/ice_main.c > @@ -5456,6 +5456,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)); > ice_dealloc_all_dynamic_ports(pf); > ice_deinit_devlink(pf); > @@ -5466,7 +5468,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); > -- > 2.55.0 Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.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);
diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c index ceb9fec2af21e7..a885b3e0baaa5d 100644 --- a/drivers/net/ethernet/intel/ice/ice_main.c +++ b/drivers/net/ethernet/intel/ice/ice_main.c @@ -5456,6 +5456,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)); ice_dealloc_all_dynamic_ports(pf); ice_deinit_devlink(pf); @@ -5466,7 +5468,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);
Since commit 5b246e533d01 ("ice: split probe into smaller functions") ice_remove() calls ice_setup_mc_magic_wake() after ice_deinit(), which frees pf->vsi. ice_get_main_vsi() then returns NULL and the function returns before it sends the Manage MAC Write command, so multicast magic packet wake and keeping a locally administered address across the PF reset are never set up on shutdown with WoL enabled. Program it before the netdev and the VSIs are torn down, as it was done before that commit, so the current netdev address is used again. i40e had a similar problem, where the multicast magic wake setup ran after the admin queue had already been shut down, and fixed it in commit e661414c98df ("i40e: Remove duplicated prepare call in i40e_shutdown"). Fixes: 5b246e533d01 ("ice: split probe into smaller functions") Assisted-by: LLM Signed-off-by: Petr Oros <poros@redhat.com> --- drivers/net/ethernet/intel/ice/ice_main.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)