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 20:40:22 +0800
From:   Guoju Fang <gjfang@...ux.alibaba.com>
To:     Yunsheng Lin <linyunsheng@...wei.com>,
        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>,
        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 18:45, Yunsheng Lin wrote:
> 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.

Hi Yunsheng, all,

I rewrite the comments of my patch and it looks a little clearer. :)

Thank you.

Best regards,

> 
>> (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
>>
>>
>>
>>
>> .
>>
View attachment "0001-net-sched-add-barrier-to-fix-packet-stuck-problem-fo.patch" of type "text/plain" (2236 bytes)