| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <SJ1PR11MB62978AEB342A7E3CA506075C9BA0A@SJ1PR11MB6297.namprd11.prod.outlook.com>
Date: Wed, 10 Dec 2025 22:28:57 +0000
From: "Salin, Samuel" <samuel.salin@...el.com>
To: Simon Horman <horms@...nel.org>, "Tantilov, Emil S"
<emil.s.tantilov@...el.com>
CC: "intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>, "Loktionov, Aleksandr"
<aleksandr.loktionov@...el.com>, "Kitszel, Przemyslaw"
<przemyslaw.kitszel@...el.com>, "Nguyen, Anthony L"
<anthony.l.nguyen@...el.com>, "andrew+netdev@...n.ch"
<andrew+netdev@...n.ch>, "davem@...emloft.net" <davem@...emloft.net>,
"edumazet@...gle.com" <edumazet@...gle.com>, "kuba@...nel.org"
<kuba@...nel.org>, "pabeni@...hat.com" <pabeni@...hat.com>,
"decot@...gle.com" <decot@...gle.com>, "willemb@...gle.com"
<willemb@...gle.com>, "Hay, Joshua A" <joshua.a.hay@...el.com>, "Chittim,
Madhu" <madhu.chittim@...el.com>, "Lobakin, Aleksander"
<aleksander.lobakin@...el.com>, "Zaremba, Larysa" <larysa.zaremba@...el.com>,
"iamvivekkumar@...gle.com" <iamvivekkumar@...gle.com>
Subject: RE: [Intel-wired-lan] [PATCH iwl-net v2 2/5] idpf: detach and close
netdevs while handling a reset
> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@...osl.org> On Behalf Of
> Simon Horman
> Sent: Wednesday, November 26, 2025 9:11 AM
> To: Tantilov, Emil S <emil.s.tantilov@...el.com>
> Cc: intel-wired-lan@...ts.osuosl.org; netdev@...r.kernel.org; Loktionov,
> Aleksandr <aleksandr.loktionov@...el.com>; Kitszel, Przemyslaw
> <przemyslaw.kitszel@...el.com>; Nguyen, Anthony L
> <anthony.l.nguyen@...el.com>; andrew+netdev@...n.ch;
> davem@...emloft.net; edumazet@...gle.com; kuba@...nel.org;
> pabeni@...hat.com; decot@...gle.com; willemb@...gle.com; Hay, Joshua
> A <joshua.a.hay@...el.com>; Chittim, Madhu <madhu.chittim@...el.com>;
> Lobakin, Aleksander <aleksander.lobakin@...el.com>; Zaremba, Larysa
> <larysa.zaremba@...el.com>; iamvivekkumar@...gle.com
> Subject: Re: [Intel-wired-lan] [PATCH iwl-net v2 2/5] idpf: detach and close
> netdevs while handling a reset
>
> On Tue, Nov 25, 2025 at 06:58:37AM -0800, Tantilov, Emil S wrote:
> >
> >
> > On 11/25/2025 5:42 AM, Simon Horman wrote:
> > > On Thu, Nov 20, 2025 at 04:12:15PM -0800, Emil Tantilov wrote:
> > >
> > > ...
> > >
> > > > diff --git a/drivers/net/ethernet/intel/idpf/idpf_lib.c
> > > > b/drivers/net/ethernet/intel/idpf/idpf_lib.c
> > > > index 2a53f3d504d2..5c81f52db266 100644
> > > > --- a/drivers/net/ethernet/intel/idpf/idpf_lib.c
> > > > +++ b/drivers/net/ethernet/intel/idpf/idpf_lib.c
> > > > @@ -752,6 +752,65 @@ static int idpf_init_mac_addr(struct idpf_vport
> *vport,
> > > > return 0;
> > > > }
> > > > +static void idpf_detach_and_close(struct idpf_adapter *adapter) {
> > > > + int max_vports = adapter->max_vports;
> > > > +
> > > > + for (int i = 0; i < max_vports; i++) {
> > > > + struct net_device *netdev = adapter->netdevs[i];
> > > > +
> > > > + /* If the interface is in detached state, that means the
> > > > + * previous reset was not handled successfully for this
> > > > + * vport.
> > > > + */
> > > > + if (!netif_device_present(netdev))
> > > > + continue;
> > > > +
> > > > + /* Hold RTNL to protect racing with callbacks */
> > > > + rtnl_lock();
> > > > + netif_device_detach(netdev);
> > > > + if (netif_running(netdev)) {
> > > > + set_bit(IDPF_VPORT_UP_REQUESTED,
> > > > + adapter->vport_config[i]->flags);
> > > > + dev_close(netdev);
> > > > + }
> > > > + rtnl_unlock();
> > > > + }
> > > > +}
> > > > +
> > > > +static void idpf_attach_and_open(struct idpf_adapter *adapter) {
> > > > + int max_vports = adapter->max_vports;
> > > > +
> > > > + for (int i = 0; i < max_vports; i++) {
> > > > + struct idpf_vport *vport = adapter->vports[i];
> > > > + struct idpf_vport_config *vport_config;
> > > > + struct net_device *netdev;
> > > > +
> > > > + /* In case of a critical error in the init task, the vport
> > > > + * will be freed. Only continue to restore the netdevs
> > > > + * if the vport is allocated.
> > > > + */
> > > > + if (!vport)
> > > > + continue;
> > > > +
> > > > + /* No need for RTNL on attach as this function is called
> > > > + * following detach and dev_close(). We do take RTNL for
> > > > + * dev_open() below as it can race with external callbacks
> > > > + * following the call to netif_device_attach().
> > > > + */
> > > > + netdev = adapter->netdevs[i];
> > > > + netif_device_attach(netdev);
> > > > + vport_config = adapter->vport_config[vport->idx];
> > > > + if (test_and_clear_bit(IDPF_VPORT_UP_REQUESTED,
> > > > + vport_config->flags)) {
> > > > + rtnl_lock();
> > > > + dev_open(netdev, NULL);
> > > > + rtnl_unlock();
> > > > + }
> > > > + }
> > > > +}
> > > > +
> > > > /**
> > > > * idpf_cfg_netdev - Allocate, configure and register a netdev
> > > > * @vport: main vport structure
> > >
> > > ...
> > >
> > > > @@ -1807,27 +1860,6 @@ static int idpf_check_reset_complete(struct
> idpf_hw *hw,
> > > > return -EBUSY;
> > > > }
> > > > -/**
> > > > - * idpf_set_vport_state - Set the vport state to be after the
> > > > reset
> > > > - * @adapter: Driver specific private structure
> > > > - */
> > > > -static void idpf_set_vport_state(struct idpf_adapter *adapter) -{
> > > > - u16 i;
> > > > -
> > > > - for (i = 0; i < adapter->max_vports; i++) {
> > > > - struct idpf_netdev_priv *np;
> > > > -
> > > > - if (!adapter->netdevs[i])
> > > > - continue;
> > > > -
> > > > - np = netdev_priv(adapter->netdevs[i]);
> > > > - if (np->state == __IDPF_VPORT_UP)
> > > > - set_bit(IDPF_VPORT_UP_REQUESTED,
> > > > - adapter->vport_config[i]->flags);
> > > > - }
> > > > -}
> > > > -
> > > > /**
> > > > * idpf_init_hard_reset - Initiate a hardware reset
> > > > * @adapter: Driver specific private structure
> > >
> > > > @@ -1836,28 +1868,17 @@ static void idpf_set_vport_state(struct
> idpf_adapter *adapter)
> > > > * reallocate. Also reinitialize the mailbox. Return 0 on success,
> > > > * negative on failure.
> > > > */
> > > > -static int idpf_init_hard_reset(struct idpf_adapter *adapter)
> > > > +static void idpf_init_hard_reset(struct idpf_adapter *adapter)
> > > > {
> > > > struct idpf_reg_ops *reg_ops = &adapter->dev_ops.reg_ops;
> > > > struct device *dev = &adapter->pdev->dev;
> > > > - struct net_device *netdev;
> > > > int err;
> > > > - u16 i;
> > > > + idpf_detach_and_close(adapter);
> > > > mutex_lock(&adapter->vport_ctrl_lock);
> > > > dev_info(dev, "Device HW Reset initiated\n");
> > > > - /* Avoid TX hangs on reset */
> > > > - for (i = 0; i < adapter->max_vports; i++) {
> > > > - netdev = adapter->netdevs[i];
> > > > - if (!netdev)
> > > > - continue;
> > >
> > > Hi Emil,
> > >
> > > In this code that is removed there is a check for !netdev.
> > > And also there is a similar check in idpf_set_vport_state().
> > > But there is no such check in idpf_detach_and_close().
> > > Is this intentional?
> >
> > This logic is a bit confusing because the reset path is executed on
> > both driver load and a reset (since the initialization is identical it
> > makes sense to re-use the code). This is what roughly happens on load
> > and
> > reset:
> >
> > driver load -> reset -> configure vports -> create netdevs reset ->
> > de-allocate vports -> re-allocate vports
> >
> > The first patch:
> > https://lore.kernel.org/intel-wired-lan/20251121001218.4565-2-emil.s.t
> > antilov@...el.com/
> >
> > makes sure that we never lose the netdev on a reset, following a
> > successful driver load. Previously this could happen in the error path.
> > In other words during a reset there is no need to check for a netdev
> > as this is guaranteed, but we must make sure that vports are present
> > as those can be freed.
> >
> > The 5th patch:
> > https://lore.kernel.org/intel-wired-lan/20251121001218.4565-6-emil.s.t
> > antilov@...el.com/
> >
> > fixes another instance where we could fail in the reset error path by
> > ensuring the service task, which handles resets is cancelled as at
> > that point we have neither vports, nor netdevs, hence nothing to
> > "serve". Hope this makes sense, but the gist of it is that with this
> > series applied the reset can be protected by just making sure that the
> > vports are allocated. If for whatever reason netdevs happen to be
> > NULL, following this series it would be a bug introduced somewhere
> > else in the code that will have to be addressed.
>
> I did spend a bit of time trying to figure out the flow, but not entirely
> successfully. Thanks for setting me straight.
>
> ...
Tested-by: Samuel Salin <Samuel.salin@...el.com>
Powered by blists - more mailing lists