netdev - Re: [Intel-wired-lan] [PATCH v2] e1000e: free IRQ regardless of __E1000

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <dff136c7-cb6a-8a61-d311-01959bac16b9@codeaurora.org>
Date:   Thu, 10 Nov 2016 15:35:17 -0700
From:   "Baicar, Tyler" <tbaicar@...eaurora.org>
To:     "Neftin, Sasha" <sasha.neftin@...el.com>,
        jeffrey.t.kirsher@...el.com, intel-wired-lan@...ts.osuosl.org,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        okaya@...eaurora.org, timur@...eaurora.org
Subject: Re: [Intel-wired-lan] [PATCH v2] e1000e: free IRQ regardless of
 __E1000_DOWN

Hello Sasha,

On 11/9/2016 11:19 PM, Neftin, Sasha wrote:
> On 11/9/2016 11:41 PM, Tyler Baicar wrote:
>> Move IRQ free code so that it will happen regardless of the
>> __E1000_DOWN bit. Currently the e1000e driver only releases its IRQ
>> if the __E1000_DOWN bit is cleared. This is not sufficient because
>> it is possible for __E1000_DOWN to be set without releasing the IRQ.
>> In such a situation, we will hit a kernel bug later in e1000_remove
>> because the IRQ still has action since it was never freed. A
>> secondary bus reset can cause this case to happen.
>>
>> Signed-off-by: Tyler Baicar <tbaicar@...eaurora.org>
>> ---
>>   drivers/net/ethernet/intel/e1000e/netdev.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
>> index 7017281..36cfcb0 100644
>> --- a/drivers/net/ethernet/intel/e1000e/netdev.c
>> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
>> @@ -4679,12 +4679,13 @@ int e1000e_close(struct net_device *netdev)
>>   
>>   	if (!test_bit(__E1000_DOWN, &adapter->state)) {
>>   		e1000e_down(adapter, true);
>> -		e1000_free_irq(adapter);
>>   
>>   		/* Link status message must follow this format */
>>   		pr_info("%s NIC Link is Down\n", adapter->netdev->name);
>>   	}				
>>   
>> +	e1000_free_irq(adapter);
>> +
>>   	napi_disable(&adapter->napi);
>>   
>>   	e1000e_free_tx_resources(adapter->tx_ring);
>>
> I would like not recommend insert this change. This change related
> driver state machine, we afraid from lot of synchronization problem and
> issues.
> We need keep e1000_free_irq in loop and check for 'test_bit' ready.

What do you mean here? There is no loop. If __E1000_DOWN is set then we
will never free the IRQ.

> Another point, does before execute secondary bus reset your SW back up
> pcie configuration space as properly?

After a secondary bus reset, the link needs to recover and go back to a
working state after 1 second.

 From the callstack, the issue is happening while removing the endpoint
from the system, before applying the secondary bus reset.

The order of events is
1. remove the drivers
2. cause a secondary bus reset
3. wait 1 second
4. recover the link

callstack:
free_msi_irqs+0x6c/0x1a8
pci_disable_msi+0xb0/0x148
e1000e_reset_interrupt_capability+0x60/0x78
e1000_remove+0xc8/0x180
pci_device_remove+0x48/0x118
__device_release_driver+0x80/0x108
device_release_driver+0x2c/0x40
pci_stop_bus_device+0xa0/0xb0
pci_stop_bus_device+0x3c/0xb0
pci_stop_root_bus+0x54/0x80
acpi_pci_root_remove+0x28/0x64
acpi_bus_trim+0x6c/0xa4
acpi_device_hotplug+0x19c/0x3f4
acpi_hotplug_work_fn+0x28/0x3c
process_one_work+0x150/0x460
worker_thread+0x50/0x4b8
kthread+0xd4/0xe8
ret_from_fork+0x10/0x50

Thanks,
Tyler

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.