| 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: <3e9dc8fd-9052-4c53-ba40-5904306d09fb@intel.com>
Date: Tue, 25 Nov 2025 06:58:37 -0800
From: "Tantilov, Emil S" <emil.s.tantilov@...el.com>
To: Simon Horman <horms@...nel.org>
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 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.tantilov@intel.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.tantilov@intel.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.
Thanks,
Emil
>
> 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