| 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: <aSWyYIsoXNlpsQn-@horms.kernel.org>
Date: Tue, 25 Nov 2025 13:42:56 +0000
From: Simon Horman <horms@...nel.org>
To: Emil Tantilov <emil.s.tantilov@...el.com>
Cc: intel-wired-lan@...ts.osuosl.org, netdev@...r.kernel.org,
Aleksandr.Loktionov@...el.com, przemyslaw.kitszel@...el.com,
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,
joshua.a.hay@...el.com, madhu.chittim@...el.com,
aleksander.lobakin@...el.com, larysa.zaremba@...el.com,
iamvivekkumar@...gle.com
Subject: Re: [PATCH iwl-net v2 2/5] idpf: detach and close netdevs while
handling a reset
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?
Likewise, is such a check needed in idpf_attach_and_open()?
My understanding is that netdevs[i] is populated asynchronously
via idpf_init_task. But I'm unsure if that is known to have
completed or not.
Flagged by Claude Code with https://github.com/masoncl/review-prompts/
> -
> - netif_carrier_off(netdev);
> - netif_tx_disable(netdev);
> - }
> -
> /* Prepare for reset */
> if (test_and_clear_bit(IDPF_HR_DRV_LOAD, adapter->flags)) {
> reg_ops->trigger_reset(adapter, IDPF_HR_DRV_LOAD);
> @@ -1866,7 +1887,6 @@ static int idpf_init_hard_reset(struct idpf_adapter *adapter)
>
> idpf_idc_issue_reset_event(adapter->cdev_info);
>
> - idpf_set_vport_state(adapter);
> idpf_vc_core_deinit(adapter);
> if (!is_reset)
> reg_ops->trigger_reset(adapter, IDPF_HR_FUNC_RESET);
> @@ -1913,11 +1933,14 @@ static int idpf_init_hard_reset(struct idpf_adapter *adapter)
> unlock_mutex:
> mutex_unlock(&adapter->vport_ctrl_lock);
>
> - /* Wait until all vports are created to init RDMA CORE AUX */
> - if (!err)
> - err = idpf_idc_init(adapter);
> -
> - return err;
> + /* Attempt to restore netdevs and initialize RDMA CORE AUX device,
> + * provided vc_core_init succeeded. It is still possible that
> + * vports are not allocated at this point if the init task failed.
> + */
> + if (!err) {
> + idpf_attach_and_open(adapter);
> + idpf_idc_init(adapter);
> + }
> }
>
> /**
> --
> 2.37.3
>
>
Powered by blists - more mailing lists