[Cake] Hopefully fixed ACK filter for v6

[Cake] Hopefully fixed ACK filter for v6

[Toke Høiland-Jørgensen]

Think I finally managed to fix the ACK filter so it works for both 6in4
and regular v6 (the latter seems to have been broken since the commit
"e6b72c2 ack_filter: make less aggressive by default" which accidentally
reversed the address compare logic). It should also be safe against
weirdly fragmented packets (ha! famous last words!).

If someone wants to either eyeball the code and/or test the ACK filter
please do. I'm planning to resubmit the patch to netdev sometime
tomorrow evening :)

-Toke

Re: [Cake] Hopefully fixed ACK filter for v6

[Ryan Mounce]

On 29 April 2018 at 07:19, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
> Think I finally managed to fix the ACK filter so it works for both 6in4
> and regular v6 (the latter seems to have been broken since the commit
> "e6b72c2 ack_filter: make less aggressive by default" which accidentally
> reversed the address compare logic). It should also be safe against
> weirdly fragmented packets (ha! famous last words!).
>
> If someone wants to either eyeball the code and/or test the ACK filter
> please do. I'm planning to resubmit the patch to netdev sometime
> tomorrow evening :)

Eyeballing now, only a couple of comments.

- Initialisation of iph and ipv6h can be moved before the while()
loop. The compiler probably does this anyway.
- Whether it is worth the complexity of restoring the
skb->encapsulation check within cake_get_tcphdr to handle other cases
of encapsulated v4/v6 traffic. I may find some time to test this later
today.

Everything else looks good to me, much cleaner.

Thank you so much for your efforts so far in getting cake upstream,
and in particular to rescue ACK filtering. We may have to send you a
real cake!

Ryan

>
> -Toke
> _______________________________________________
> Cake mailing list
> Cake@lists.bufferbloat.net
> https://lists.bufferbloat.net/listinfo/cake

Re: [Cake] Hopefully fixed ACK filter for v6

[Toke Høiland-Jørgensen]

On 29 April 2018 03:01:55 CEST, Ryan Mounce <ryan@mounce.com.au> wrote:
>On 29 April 2018 at 07:19, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
>> Think I finally managed to fix the ACK filter so it works for both
>6in4
>> and regular v6 (the latter seems to have been broken since the commit
>> "e6b72c2 ack_filter: make less aggressive by default" which
>accidentally
>> reversed the address compare logic). It should also be safe against
>> weirdly fragmented packets (ha! famous last words!).
>>
>> If someone wants to either eyeball the code and/or test the ACK
>filter
>> please do. I'm planning to resubmit the patch to netdev sometime
>> tomorrow evening :)
>
>Eyeballing now, only a couple of comments.
>
>- Initialisation of iph and ipv6h can be moved before the while()
>loop. The compiler probably does this anyway.

Yeah, figured I'd let the compiler deal with that...

>- Whether it is worth the complexity of restoring the
>skb->encapsulation check within cake_get_tcphdr to handle other cases
>of encapsulated v4/v6 traffic. I may find some time to test this later
>today.

The reason I removed the check for skb->encapsulation was that in my tests it wasn't actually set for 6in4 traffic.

And looking at the code that touches it, that field seems to mostly be related to offloads? Or did you actually see any effect of using it when you added it initially? :P

>Everything else looks good to me, much cleaner.

Awesome, thanks for taking a look!

>Thank you so much for your efforts so far in getting cake upstream,
>and in particular to rescue ACK filtering. We may have to send you a
>real cake!

Haha, you're very welcome. Real cake will be appreciated, but don't feel any obligations ;)

-Toke

Re: [Cake] Hopefully fixed ACK filter for v6

[Jonathan Morton]

> Real cake will be appreciated, but don't feel any obligations ;)

Since I live a lot closer to Denmark than Australia is, perhaps I should get baking...

 - Jonathan Morton

Re: [Cake] Hopefully fixed ACK filter for v6

[Sebastian Moeller]

[-- Attachment #1: Type: text/plain, Size: 2353 bytes --]

Hi Toke, 

I certainly am thankful for your work, and believe you deserve $CAKE and $BEVERAGE, I am also leaf to believe 'the cake is a lie' https://m.youtube.com/watch?v=qdrs3gr_GAs ;)



Mange Tak

Sebastian

On April 29, 2018 12:45:26 PM GMT+02:00, "Toke Høiland-Jørgensen" <toke@toke.dk> wrote:
>
>
>On 29 April 2018 03:01:55 CEST, Ryan Mounce <ryan@mounce.com.au> wrote:
>>On 29 April 2018 at 07:19, Toke Høiland-Jørgensen <toke@toke.dk>
>wrote:
>>> Think I finally managed to fix the ACK filter so it works for both
>>6in4
>>> and regular v6 (the latter seems to have been broken since the
>commit
>>> "e6b72c2 ack_filter: make less aggressive by default" which
>>accidentally
>>> reversed the address compare logic). It should also be safe against
>>> weirdly fragmented packets (ha! famous last words!).
>>>
>>> If someone wants to either eyeball the code and/or test the ACK
>>filter
>>> please do. I'm planning to resubmit the patch to netdev sometime
>>> tomorrow evening :)
>>
>>Eyeballing now, only a couple of comments.
>>
>>- Initialisation of iph and ipv6h can be moved before the while()
>>loop. The compiler probably does this anyway.
>
>Yeah, figured I'd let the compiler deal with that...
>
>>- Whether it is worth the complexity of restoring the
>>skb->encapsulation check within cake_get_tcphdr to handle other cases
>>of encapsulated v4/v6 traffic. I may find some time to test this later
>>today.
>
>The reason I removed the check for skb->encapsulation was that in my
>tests it wasn't actually set for 6in4 traffic.
>
>And looking at the code that touches it, that field seems to mostly be
>related to offloads? Or did you actually see any effect of using it
>when you added it initially? :P
>
>>Everything else looks good to me, much cleaner.
>
>Awesome, thanks for taking a look!
>
>>Thank you so much for your efforts so far in getting cake upstream,
>>and in particular to rescue ACK filtering. We may have to send you a
>>real cake!
>
>Haha, you're very welcome. Real cake will be appreciated, but don't
>feel any obligations ;)
>
>-Toke
>_______________________________________________
>Cake mailing list
>Cake@lists.bufferbloat.net
>https://lists.bufferbloat.net/listinfo/cake

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

[-- Attachment #2: Type: text/html, Size: 3797 bytes --]

Re: [Cake] Hopefully fixed ACK filter for v6

[Toke Høiland-Jørgensen]

Sebastian Moeller <moeller0@gmx.de> writes:

> Hi Toke, 
>
> I certainly am thankful for your work, and believe you deserve $CAKE
> and $BEVERAGE, I am also leaf to believe 'the cake is a lie'
> https://m.youtube.com/watch?v=qdrs3gr_GAs ;)

Haha, yes, of course I am aware that the cake really is a lie. Which
makes us in league with GLADOS, I suppose, since we're promising
everyone CAKE ;)

-Toke

Re: [Cake] Hopefully fixed ACK filter for v6

[Jonathan Morton]

>> I certainly am thankful for your work, and believe you deserve $CAKE
>> and $BEVERAGE, I am also leaf to believe 'the cake is a lie'
>> https://m.youtube.com/watch?v=qdrs3gr_GAs ;)
> 
> Haha, yes, of course I am aware that the cake really is a lie. Which
> makes us in league with GLADOS, I suppose, since we're promising
> everyone CAKE ;)

In lieu of sending a home-baked cake to Denmark, I have obtained an allegedly Danish apple cake from the local supermarket.  Though with the inherent ambiguity of the English language, it's unclear whether it is the cake itself that's supposed to be Danish, or the apples used to make it.  (According to the ingredients list, only 2% of the cake is actually apples.)

Similarly vague and imprecise are the serving instructions, which talk about a pre-heated oven but say nothing about the required temperature!  Luckily, I do enough baking to know that 175°C works well in this oven.

 - Jonathan Morton

Re: [Cake] Hopefully fixed ACK filter for v6

[Toke Høiland-Jørgensen]

Jonathan Morton <chromatix99@gmail.com> writes:

>>> I certainly am thankful for your work, and believe you deserve $CAKE
>>> and $BEVERAGE, I am also leaf to believe 'the cake is a lie'
>>> https://m.youtube.com/watch?v=qdrs3gr_GAs ;)
>> 
>> Haha, yes, of course I am aware that the cake really is a lie. Which
>> makes us in league with GLADOS, I suppose, since we're promising
>> everyone CAKE ;)
>
> In lieu of sending a home-baked cake to Denmark, I have obtained an
> allegedly Danish apple cake from the local supermarket. Though with
> the inherent ambiguity of the English language, it's unclear whether
> it is the cake itself that's supposed to be Danish, or the apples used
> to make it. (According to the ingredients list, only 2% of the cake is
> actually apples.)

Haha, enjoy. Though I will take no responsibility for the quality of
"Danish" cuisine sold at your local supermarket ;)

-Toke