[WireGuard] WireGuard ECN Implementation

[WireGuard] WireGuard ECN Implementation

[Jason A. Donenfeld]

Hey Dave,

I'm back! Catching up on the backlog now. You wrote in your blog post:

> In a day, the author (while on a plane!) tossed off an ecn encapsulation
> implementation (it worked! but it=E2=80=99s not currently as modern RFC c=
ompliant as it should be),

What part of it deviates from the RFC precisely? Here's a short
summary of the implementation:

- When a packet is transmitted:

    outer_packet->ds =3D
            ip_tunnel_ecn_encap(0, ip_hdr(inner_packet), inner_packet);

- When a packet is received:

    var =3D ip_tunnel_get_dsfield(ip_hdr(outer_packet), outer_packet);
    if (INET_ECN_is_ce(var))
        IP_ECN_set_ce(ip_hdr(inner_packet));

What specifically is wrong with this? Where does it deviate from the
RFC? I'd like to be as "correct" as possible.

Talk soon,
Jason

Re: [WireGuard] WireGuard ECN Implementation

[Dave Taht]

On Thu, Sep 29, 2016 at 10:50 AM, Jason A. Donenfeld <Jason@zx2c4.com> wrot=
e:
> Hey Dave,
>
> I'm back! Catching up on the backlog now. You wrote in your blog post:

Welcome back!

I got distracted also. See "TCP BBR".

I have no idea how sch_fq + pacing fits into your design, either.

>> In a day, the author (while on a plane!) tossed off an ecn encapsulation
>> implementation (it worked! but it=E2=80=99s not currently as modern RFC =
compliant as it should be),
>
> What part of it deviates from the RFC precisely? Here's a short
> summary of the implementation:
>
> - When a packet is transmitted:
>
>     outer_packet->ds =3D
>             ip_tunnel_ecn_encap(0, ip_hdr(inner_packet), inner_packet);
>
> - When a packet is received:
>
>     var =3D ip_tunnel_get_dsfield(ip_hdr(outer_packet), outer_packet);
>     if (INET_ECN_is_ce(var))
>         IP_ECN_set_ce(ip_hdr(inner_packet));

I think the correct behavior here is to only set ce on the inner
packet if the inner packet is marked as ecn capable.

> What specifically is wrong with this? Where does it deviate from the
> RFC? I'd like to be as "correct" as possible.

It looked to as tho leveraging the other newer ecn capable codebases
in the kernel was sane also.


>
> Talk soon,
> Jason



--=20
Dave T=C3=A4ht
Let's go make home routers and wifi faster! With better software!
http://blog.cerowrt.org

Re: [WireGuard] WireGuard ECN Implementation

[Jason A. Donenfeld]

On Thu, Sep 29, 2016 at 8:19 PM, Dave Taht <dave.taht@gmail.com> wrote:
> I think the correct behavior here is to only set ce on the inner
> packet if the inner packet is marked as ecn capable.

IP_ECN_set_ce already does this. It exits early if it isn't already
ECT(1) or ECT(0):

static inline int IP_ECN_set_ce(struct iphdr *iph)
{
       u32 check = (__force u32)iph->check;
       u32 ecn = (iph->tos + 1) & INET_ECN_MASK;

       /*
        * After the last operation we have (in binary):
        * INET_ECN_NOT_ECT => 01
        * INET_ECN_ECT_1   => 10
        * INET_ECN_ECT_0   => 11
        * INET_ECN_CE      => 00
        */
       if (!(ecn & 2))
               return !ecn;

       /*
        * The following gives us:
        * INET_ECN_ECT_1 => check += htons(0xFFFD)
        * INET_ECN_ECT_0 => check += htons(0xFFFE)
        */
       check += (__force u16)htons(0xFFFB) + (__force u16)htons(ecn);

       iph->check = (__force __sum16)(check + (check>=0xFFFF));
       iph->tos |= INET_ECN_CE;
       return 1;
}



>
> It looked to as tho leveraging the other newer ecn capable codebases
> in the kernel was sane also.

I tried to copy already existing code in the kernel for this. Is there
a certain driver that you think does it particularly well that I
should copy?

Re: [WireGuard] WireGuard ECN Implementation

[Dave Taht]

that + 1 was clever. I think you are done... and I should go change the blo=
g. :)

On Thu, Sep 29, 2016 at 11:59 AM, Jason A. Donenfeld <Jason@zx2c4.com> wrot=
e:
> On Thu, Sep 29, 2016 at 8:19 PM, Dave Taht <dave.taht@gmail.com> wrote:
>> I think the correct behavior here is to only set ce on the inner
>> packet if the inner packet is marked as ecn capable.
>
> IP_ECN_set_ce already does this. It exits early if it isn't already
> ECT(1) or ECT(0):
>
> static inline int IP_ECN_set_ce(struct iphdr *iph)
> {
>        u32 check =3D (__force u32)iph->check;
>        u32 ecn =3D (iph->tos + 1) & INET_ECN_MASK;
>
>        /*
>         * After the last operation we have (in binary):
>         * INET_ECN_NOT_ECT =3D> 01
>         * INET_ECN_ECT_1   =3D> 10
>         * INET_ECN_ECT_0   =3D> 11
>         * INET_ECN_CE      =3D> 00
>         */
>        if (!(ecn & 2))
>                return !ecn;
>
>        /*
>         * The following gives us:
>         * INET_ECN_ECT_1 =3D> check +=3D htons(0xFFFD)
>         * INET_ECN_ECT_0 =3D> check +=3D htons(0xFFFE)
>         */
>        check +=3D (__force u16)htons(0xFFFB) + (__force u16)htons(ecn);
>
>        iph->check =3D (__force __sum16)(check + (check>=3D0xFFFF));
>        iph->tos |=3D INET_ECN_CE;
>        return 1;
> }
>
>
>
>>
>> It looked to as tho leveraging the other newer ecn capable codebases
>> in the kernel was sane also.
>
> I tried to copy already existing code in the kernel for this. Is there
> a certain driver that you think does it particularly well that I
> should copy?



--=20
Dave T=C3=A4ht
Let's go make home routers and wifi faster! With better software!
http://blog.cerowrt.org

Re: [WireGuard] WireGuard ECN Implementation

[Jason A. Donenfeld]

On Thu, Sep 29, 2016 at 9:03 PM, Dave Taht <dave.taht@gmail.com> wrote:
> that + 1 was clever. I think you are done... and I should go change the blog. :)

Yea I like tricks like that. Anytime you have a range of values in the
middle, you can just add, modulo, and check the end.

Re: [WireGuard] WireGuard ECN Implementation

[Dave Taht]

On Thu, Sep 29, 2016 at 12:05 PM, Jason A. Donenfeld <Jason@zx2c4.com> wrot=
e:
> On Thu, Sep 29, 2016 at 9:03 PM, Dave Taht <dave.taht@gmail.com> wrote:
>> that + 1 was clever. I think you are done... and I should go change the =
blog. :)
>
> Yea I like tricks like that. Anytime you have a range of values in the
> middle, you can just add, modulo, and check the end.

ok. I pushed an update to http://blog.cerowrt.org/post/wireguard/ - it
will take a while to be cache updated.

Now... as for the other stuff in that blog entry (I never got around
to writing parts II and III), I am curious as to your raw PPS with
small packets presently, and if you've figured out how to apply
fq_codel internally yet? :-P

Also the new pacing + bbr stuff coming out from google looks mighty
interesting, do you lose the tcp clock kept there?



--=20
Dave T=C3=A4ht
Let's go make home routers and wifi faster! With better software!
http://blog.cerowrt.org

Re: [WireGuard] WireGuard ECN Implementation

[Jason A. Donenfeld]

On Thu, Sep 29, 2016 at 10:03 PM, Dave Taht <dave.taht@gmail.com> wrote:
> Now... as for the other stuff in that blog entry (I never got around
> to writing parts II and III), I am curious as to your raw PPS with
> small packets presently, and if you've figured out how to apply
> fq_codel internally yet? :-P

I haven't looked deeply into it yet, but one thing that makes me a bit
hesitant is that fq_codel does flow classification. WireGuard uses a
separate queue for each peer, so that it can start and stop it
independently depending on the state of various crypto handshake
things. This also means that each queue only has one flow.
Furthermore, flow classification is usually done because different
endpoints have different latencies. But in this case, the latency
isn't a factor of which endpoint, but rather of the CPU doing the
encryption/decryption operation. So, I'm not yet sure precisely how to
conceptualize fitting fq_codel over WireGuard.