netdev - Re: packet stuck in qdisc : patch proposal

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, 25 May 2022 18:45:08 +0800
From:   Yunsheng Lin <linyunsheng@...wei.com>
To:     Vincent Ray <vray@...rayinc.com>,
        Eric Dumazet <eric.dumazet@...il.com>
CC:     davem <davem@...emloft.net>,
        方国炬 <guoju.fgj@...baba-inc.com>,
        kuba <kuba@...nel.org>, netdev <netdev@...r.kernel.org>,
        Samuel Jones <sjones@...rayinc.com>,
        "vladimir oltean" <vladimir.oltean@....com>,
        Guoju Fang <gjfang@...ux.alibaba.com>,
        "Remy Gauguey" <rgauguey@...rayinc.com>, will <will@...nel.org>,
        Eric Dumazet <edumazet@...gle.com>
Subject: Re: packet stuck in qdisc : patch proposal

On 2022/5/25 17:44, Vincent Ray wrote:
> ----- On May 24, 2022, at 10:17 PM, Eric Dumazet eric.dumazet@...il.com wrote:
> 
>> On 5/24/22 10:00, Vincent Ray wrote:
>>> All,
>>>
>>> I confirm Eric's patch works well too, and it's better and clearer than mine.
>>> So I think we should go for it, and the one from Guoju in addition.
>>>
>>> @Eric : I see you are one of the networking maintainers, so I have a few
>>> questions for you :
>>>
>>> a) are you going to take care of these patches directly yourself, or is there
>>> something Guoju or I should do to promote them ?
>>
>> I think this is totally fine you take ownership of the patch, please
>> send a formal V2.
>>
>> Please double check what patchwork had to say about your V1 :
>>
>>
>> https://patchwork.kernel.org/project/netdevbpf/patch/1684598287.15044793.1653314052575.JavaMail.zimbra@kalray.eu/
>>
>>
>> And make sure to address the relevant points
> 
> OK I will.
> If you agree, I will take your version of the fix (test_and_set_bit()), keeping the commit message
> similar to my original one.
> 
> What about Guoju's patch ? 

@Guoju, please speak up if you want to handle the patch yourself.

> (adding a smp_mb() between the spin_unlock() and test_bit() in qdisc_run_end()). 
> I think it is also necessary though potentially less critical.
> Do we embed it in the same patch ? or patch series ?

Guoju's patch fixes the commit a90c57f2cedd, so "patch series"
seems better if Guoju is not speaking up to handle the patch himself.


> 
> @Guoju : have you submitted it for integration ?
> 
> 
>> The most important one is the lack of 'Signed-off-by:' tag, of course.
>>
>>
>>> b) Can we expect to see them land in the mainline soon ?
>>
>> If your v2 submission is correct, it can be merged this week ;)
>>
>>
>>>
>>> c) Will they be backported to previous versions of the kernel ? Which ones ?
>>
>> You simply can include a proper Fixes: tag, so that stable teams can
>> backport
>>
>> the patch to all affected kernel versions.
>>
> 
> Here things get a little complicated in my head ;-)
> As explained, I think this mechanism has been bugged, in a way or an other, for some time, perhaps since the introduction
> of lockless qdiscs (4.16) or somewhere between 4.16 and 5.14.
> It's hard to tell at a glance since the code looks quite different back then.
> Because of these changes, a unique patch will also only apply up to a certain point in the past.
> 
> However, I think the bug became really critical only with the introduction of "true bypass" behavior 
> in lockless qdiscs by YunSheng in 5.14, though there may be scenarios where it is a big deal 
> even in no-bypass mode.


commit 89837eb4b246 tried to fix that, but it did not fix it completely, and that commit should has
been back-ported to the affected kernel versions as much as possible, so I think the Fixes tag
should be:

Fixes: 89837eb4b246 ("net: sched: add barrier to ensure correct ordering for lockless qdisc")

> 
> => I suggest we only tag it for backward fix up to the 5.14, where it should apply smoothly,
>  and we live with the bug for versions before that.
> This would mean that 5.15 LT can be patched but no earlier LT
>  
> What do you think ?
> 
> BTW : forgive my ignorance, but are there any kind of "Errata Sheet" or similar for known bugs that 
> won't be fixed in a given kernel ?
> 
>>
>>
>>>
>>> Thanks a lot, best,
>>
>> Thanks a lot for working on this long standing issue.
>>
>>
>>
>>
>> To declare a filtering error, please use the following link :
>> https://www.security-mail.net/reporter.php?mid=7009.628d3d4c.37c04.0&r=vray%40kalrayinc.com&s=eric.dumazet%40gmail.com&o=Re%3A+packet+stuck+in+qdisc+%3A+patch+proposal&verdict=C&c=0ca08e7b7e420d1ab014cda67db48db71df41f5f
> 
> 
> 
> 
> .
>