netdev - Re: [PATCH net-next 1/7] ionic: tx and rx queues state follows link state

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: <af492e16-927e-e10f-1213-59d14634462d@pensando.io>
Date:   Thu, 12 Mar 2020 17:12:45 -0700
From:   Shannon Nelson <snelson@...sando.io>
To:     David Miller <davem@...emloft.net>
Cc:     netdev@...r.kernel.org
Subject: Re: [PATCH net-next 1/7] ionic: tx and rx queues state follows link
 state

On 3/12/20 3:41 PM, David Miller wrote:
> From: Shannon Nelson <snelson@...sando.io>
> Date: Thu, 12 Mar 2020 14:50:09 -0700
>
>> +		if (!test_bit(IONIC_LIF_F_UP, lif->state) &&
>> +		    netif_running(netdev)) {
>> +			rtnl_lock();
>> +			ionic_open(netdev);
>> +			rtnl_unlock();
>>   		}
> You're running into a major problem area here.
>
> ionic_open() can fail, particularly because it allocates resources.
>
> Yet you are doing this in an operational path that doesn't handle
> and unwind from errors.
>
> You must find a way to do this properly, because the current approach
> can result in an inoperable interface.

I don't see this as much different from how we use it in 
ionic_reset_queues(), which was modeled after some other drivers' uses 
of the open call.  In the fw reset case, though, the time between the 
close and the open is many seconds.

Yes, ionic_open() can fail, and it unwinds its own work.  There isn't 
anything here in ionic_link_status_check() to unwind, and no one to 
report the error to, so we don't catch the error here. However, it would 
be better if I move the addition of the IONIC_LIF_F_TRANS flag and a 
couple other bits from patch 7 into this patch - I can do that for a v2 
patchset.

sln