Re: packet stuck in qdisc : patch proposal

[Vincent Ray <vray@kalrayinc.com>]

----- On May 24, 2022, at 10:17 PM, Eric Dumazet eric.dumazet@gmail.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 ? 
(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 : 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.

=> 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