netdev - Re: [PATCH] iavf: fix locking of critical sections

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]

Date:   Wed, 17 Mar 2021 19:14:52 +0100
From:   Stefan Assmann <sassmann@...nic.de>
To:     "Laba, SlawomirX" <slawomirx.laba@...el.com>,
        "Brandeburg, Jesse" <jesse.brandeburg@...el.com>,
        Jakub Kicinski <kuba@...nel.org>
Cc:     "intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "Nguyen, Anthony L" <anthony.l.nguyen@...el.com>,
        "Yang, Lihong" <lihong.yang@...el.com>,
        "Nunley, Nicholas D" <nicholas.d.nunley@...el.com>
Subject: Re: [PATCH] iavf: fix locking of critical sections

On 17.03.21 18:27, Laba, SlawomirX wrote:
> We were discussing introducing mutexes in those critical spots for a long time now (in my team).
> Stefan, if you find time, you are most welcome to offer your solution with mutexes.

Hi Slawek,

I'll work on that conversion once the current patch went through Intel
testing and is merged. Smaller patches, smaller steps are usually
better, so let's make one change at a time.

Thanks!

  Stefan

> Slawek
> 
> 
> -----Original Message-----
> From: Stefan Assmann <sassmann@...nic.de> 
> Sent: Wednesday, March 17, 2021 8:50 AM
> To: Brandeburg, Jesse <jesse.brandeburg@...el.com>; Jakub Kicinski <kuba@...nel.org>
> Cc: intel-wired-lan@...ts.osuosl.org; netdev@...r.kernel.org; Nguyen, Anthony L <anthony.l.nguyen@...el.com>; Yang, Lihong <lihong.yang@...el.com>; Laba, SlawomirX <slawomirx.laba@...el.com>; Nunley, Nicholas D <nicholas.d.nunley@...el.com>
> Subject: Re: [PATCH] iavf: fix locking of critical sections
> 
> On 16.03.21 23:02, Jesse Brandeburg wrote:
>> Jakub Kicinski wrote:
>>>>> I personally think that the overuse of flags in Intel drivers 
>>>>> brings nothing but trouble. At which point does it make sense to 
>>>>> just add a lock / semaphore here rather than open code all this 
>>>>> with no clear semantics? No code seems to just test the 
>>>>> __IAVF_IN_CRITICAL_TASK flag, all the uses look like poor man's 
>>>>> locking at a quick grep. What am I missing?
>>>>
>>>> I agree with you that the locking could be done with other locking 
>>>> mechanisms just as good. I didn't invent the current method so I'll 
>>>> let Intel comment on that part, but I'd like to point out that what 
>>>> I'm making use of is fixing what is currently in the driver.
>>>
>>> Right, I should have made it clear that I don't blame you for the 
>>> current state of things. Would you mind sending a patch on top of 
>>> this one to do a conversion to a semaphore?
> 
> Sure, I'm happy to help working on the conversion once the current issue is resolved.
> 
>>> Intel folks any opinions?
>>
>> I know Slawomir has been working closely with Stefan on figuring out 
>> the right ways to fix this code.  Hopefully he can speak for himself, 
>> but I know he's on Europe time.
>>
>> As for conversion to mutexes I'm a big fan, and as long as we don't 
>> have too many collisions with the RTNL lock I think it's a reasonable 
>> improvement to do, and if Stefan doesn't want to work on it, we can 
>> look into whether Slawomir or his team can.
> 
> I'd appreciate to be involved.
> Thanks!
> 
>   Stefan
>