TC bpf_csum_diff problems post-5.6 kernel

TC bpf_csum_diff problems post-5.6 kernel

[Thomas Ptacek]

The problem I'm dealing with is XDP-adjacent, but not itself an XDP issue, so
disregard if I'm too far outside the charter of the list. :)

We run an XDP/TC-based UDP CDN.

"Edge" machine XDP takes UDP packets off eno1, slaps a proxy header on
them and bounces them to a wg0 (WireGuard)  interface, which shuttles them
to "Worker" machines.

"Worker" TC BPF on wg0 intercepts those UDP packets, strips off the
proxy header, reflects values from that header back into the UDP and IP
headers of the skb, and fixes up the checksums.

All this works fine up through kernel 5.6. But we're working on a fleet update,
and something >= 5.8 is breaking my code (or my code was always broken
and relying on some pre-5.8 bug to function).

With a bunch of perf debugging I've narrowed the problem down: it's the
checksum diff that accounts for the stripped header. I do (roughly):

    /* not shown: parse, copy proxy header to stack */

    bpf_skb_adjust_room(ctx, -12, BPF_ADJ_ROOM_NET, 0)
    skb_pull_data(ctx, sizeof(struct iphdr) + sizeof(struct udphdr))

    /* not shown: re-check packet pointers, set up pointers to headers ... */
    sum = ~((uint32_t)(udphdr->udp_sum))
    sum = bpf_csum_diff(&proxyHeader, 12, NULL, 0, sum);

    /* not shown: make other checksum fixups, write sum back to packet */

Using some perf-based printf debugging, I can see that the checksum
I'm getting from that bpf_csum_diff is wacky. Further: if I just leave the
proxy header in the UDP packet, but still change the UDP ports and
IP addresses, I get valid UDP checksums (albeit with a useless packet
that has a proxy header still in it).

I'm wondering if anyone can think of something that would have happened
post-5.6 that would have broken this (or if there's something obviously
abusive I'm doing with my "working" code, such that this never should have
worked to begin with).

---
Thomas H. Ptacek

Re: TC bpf_csum_diff problems post-5.6 kernel

[Daniel Borkmann]

On 11/2/21 10:46 PM, Thomas Ptacek wrote:
> The problem I'm dealing with is XDP-adjacent, but not itself an XDP issue, so
> disregard if I'm too far outside the charter of the list. :)
> 
> We run an XDP/TC-based UDP CDN.
> 
> "Edge" machine XDP takes UDP packets off eno1, slaps a proxy header on
> them and bounces them to a wg0 (WireGuard)  interface, which shuttles them
> to "Worker" machines.
> 
> "Worker" TC BPF on wg0 intercepts those UDP packets, strips off the
> proxy header, reflects values from that header back into the UDP and IP
> headers of the skb, and fixes up the checksums.
> 
> All this works fine up through kernel 5.6. But we're working on a fleet update,
> and something >= 5.8 is breaking my code (or my code was always broken
> and relying on some pre-5.8 bug to function).
> 
> With a bunch of perf debugging I've narrowed the problem down: it's the
> checksum diff that accounts for the stripped header. I do (roughly):
> 
>      /* not shown: parse, copy proxy header to stack */
> 
>      bpf_skb_adjust_room(ctx, -12, BPF_ADJ_ROOM_NET, 0)

Hmm, if you add BPF_F_ADJ_ROOM_NO_CSUM_RESET instead of 0 as flag above, would
that work?

>      skb_pull_data(ctx, sizeof(struct iphdr) + sizeof(struct udphdr))
> 
>      /* not shown: re-check packet pointers, set up pointers to headers ... */
>      sum = ~((uint32_t)(udphdr->udp_sum))
>      sum = bpf_csum_diff(&proxyHeader, 12, NULL, 0, sum);
> 
>      /* not shown: make other checksum fixups, write sum back to packet */
> 
> Using some perf-based printf debugging, I can see that the checksum
> I'm getting from that bpf_csum_diff is wacky. Further: if I just leave the
> proxy header in the UDP packet, but still change the UDP ports and
> IP addresses, I get valid UDP checksums (albeit with a useless packet
> that has a proxy header still in it).
> 
> I'm wondering if anyone can think of something that would have happened
> post-5.6 that would have broken this (or if there's something obviously
> abusive I'm doing with my "working" code, such that this never should have
> worked to begin with).
> 
> ---
> Thomas H. Ptacek
> 

Re: TC bpf_csum_diff problems post-5.6 kernel

[Thomas Ptacek]

!!!

That DID fix it! (I thought I tried it before, this time I just used
(1ULL<<5) as the flag).

Why did that fix it? Is that flag new? :)

Thank you so much!

On Tue, Nov 2, 2021 at 5:00 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 11/2/21 10:46 PM, Thomas Ptacek wrote:
> > The problem I'm dealing with is XDP-adjacent, but not itself an XDP issue, so
> > disregard if I'm too far outside the charter of the list. :)
> >
> > We run an XDP/TC-based UDP CDN.
> >
> > "Edge" machine XDP takes UDP packets off eno1, slaps a proxy header on
> > them and bounces them to a wg0 (WireGuard)  interface, which shuttles them
> > to "Worker" machines.
> >
> > "Worker" TC BPF on wg0 intercepts those UDP packets, strips off the
> > proxy header, reflects values from that header back into the UDP and IP
> > headers of the skb, and fixes up the checksums.
> >
> > All this works fine up through kernel 5.6. But we're working on a fleet update,
> > and something >= 5.8 is breaking my code (or my code was always broken
> > and relying on some pre-5.8 bug to function).
> >
> > With a bunch of perf debugging I've narrowed the problem down: it's the
> > checksum diff that accounts for the stripped header. I do (roughly):
> >
> >      /* not shown: parse, copy proxy header to stack */
> >
> >      bpf_skb_adjust_room(ctx, -12, BPF_ADJ_ROOM_NET, 0)
>
> Hmm, if you add BPF_F_ADJ_ROOM_NO_CSUM_RESET instead of 0 as flag above, would
> that work?
>
> >      skb_pull_data(ctx, sizeof(struct iphdr) + sizeof(struct udphdr))
> >
> >      /* not shown: re-check packet pointers, set up pointers to headers ... */
> >      sum = ~((uint32_t)(udphdr->udp_sum))
> >      sum = bpf_csum_diff(&proxyHeader, 12, NULL, 0, sum);
> >
> >      /* not shown: make other checksum fixups, write sum back to packet */
> >
> > Using some perf-based printf debugging, I can see that the checksum
> > I'm getting from that bpf_csum_diff is wacky. Further: if I just leave the
> > proxy header in the UDP packet, but still change the UDP ports and
> > IP addresses, I get valid UDP checksums (albeit with a useless packet
> > that has a proxy header still in it).
> >
> > I'm wondering if anyone can think of something that would have happened
> > post-5.6 that would have broken this (or if there's something obviously
> > abusive I'm doing with my "working" code, such that this never should have
> > worked to begin with).
> >
> > ---
> > Thomas H. Ptacek
> >
>


-- 
---
Thomas H. Ptacek
312-231-7805

Re: TC bpf_csum_diff problems post-5.6 kernel

[Daniel Borkmann]

On 11/2/21 11:08 PM, Thomas Ptacek wrote:
> !!!
> 
> That DID fix it! (I thought I tried it before, this time I just used
> (1ULL<<5) as the flag).
> 
> Why did that fix it? Is that flag new? :)

Some context on the full series:

   https://lore.kernel.org/bpf/cover.1591108731.git.daniel@iogearbox.net/

For the update, did you use bpf_csum_update() helper or directly writing into
the pkt?

> Thank you so much!

Np, thanks!

> On Tue, Nov 2, 2021 at 5:00 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>>
>> On 11/2/21 10:46 PM, Thomas Ptacek wrote:
>>> The problem I'm dealing with is XDP-adjacent, but not itself an XDP issue, so
>>> disregard if I'm too far outside the charter of the list. :)
>>>
>>> We run an XDP/TC-based UDP CDN.
>>>
>>> "Edge" machine XDP takes UDP packets off eno1, slaps a proxy header on
>>> them and bounces them to a wg0 (WireGuard)  interface, which shuttles them
>>> to "Worker" machines.
>>>
>>> "Worker" TC BPF on wg0 intercepts those UDP packets, strips off the
>>> proxy header, reflects values from that header back into the UDP and IP
>>> headers of the skb, and fixes up the checksums.
>>>
>>> All this works fine up through kernel 5.6. But we're working on a fleet update,
>>> and something >= 5.8 is breaking my code (or my code was always broken
>>> and relying on some pre-5.8 bug to function).
>>>
>>> With a bunch of perf debugging I've narrowed the problem down: it's the
>>> checksum diff that accounts for the stripped header. I do (roughly):
>>>
>>>       /* not shown: parse, copy proxy header to stack */
>>>
>>>       bpf_skb_adjust_room(ctx, -12, BPF_ADJ_ROOM_NET, 0)
>>
>> Hmm, if you add BPF_F_ADJ_ROOM_NO_CSUM_RESET instead of 0 as flag above, would
>> that work?
>>
>>>       skb_pull_data(ctx, sizeof(struct iphdr) + sizeof(struct udphdr))
>>>
>>>       /* not shown: re-check packet pointers, set up pointers to headers ... */
>>>       sum = ~((uint32_t)(udphdr->udp_sum))
>>>       sum = bpf_csum_diff(&proxyHeader, 12, NULL, 0, sum);
>>>
>>>       /* not shown: make other checksum fixups, write sum back to packet */
>>>
>>> Using some perf-based printf debugging, I can see that the checksum
>>> I'm getting from that bpf_csum_diff is wacky. Further: if I just leave the
>>> proxy header in the UDP packet, but still change the UDP ports and
>>> IP addresses, I get valid UDP checksums (albeit with a useless packet
>>> that has a proxy header still in it).
>>>
>>> I'm wondering if anyone can think of something that would have happened
>>> post-5.6 that would have broken this (or if there's something obviously
>>> abusive I'm doing with my "working" code, such that this never should have
>>> worked to begin with).
>>>
>>> ---
>>> Thomas H. Ptacek
>>>
>>
> 
> 

Re: TC bpf_csum_diff problems post-5.6 kernel

[Thomas Ptacek]

Thanks for the context!

I write back to the packet directly (I chain all the checksum updates
through a series of bpf_csum_diff() calls and then write back the
ultimate value).

Since we have a mixed fleet right now, I'm guessing I'm going to need
multiple versions of the TC BPF .o binary, since it looks like
bpf_skb_net_shrink() EINVAL's on 5.6 with that
BPF_F_ADJ_ROOM_NO_CSUM_RESET flag.

A small price to pay! Again: many thanks for this; this group is really great.

On Tue, Nov 2, 2021 at 5:28 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
> For the update, did you use bpf_csum_update() helper or directly writing into
> the pkt?

---
Thomas H. Ptacek

Re: TC bpf_csum_diff problems post-5.6 kernel

[Lorenz Bauer]

On Tue, 2 Nov 2021 at 22:33, Thomas Ptacek <thomasptacek@gmail.com> wrote:
>
> Thanks for the context!
>
> I write back to the packet directly (I chain all the checksum updates
> through a series of bpf_csum_diff() calls and then write back the
> ultimate value).
>
> Since we have a mixed fleet right now, I'm guessing I'm going to need
> multiple versions of the TC BPF .o binary, since it looks like
> bpf_skb_net_shrink() EINVAL's on 5.6 with that
> BPF_F_ADJ_ROOM_NO_CSUM_RESET flag.

Funny, I saw your tweet and went "uh oh" :D

Daniel will know better, but the way I understood it
BPF_F_ADJ_ROOM_NO_CSUM_RESET just tells the kernel to not verify the
checksum of the packet again as it goes through the network stack. If
setting that flag fixes your problem it makes me suspect that
something in your checksum code is wonky.

Lorenz

-- 
Lorenz Bauer  |  Systems Engineer
6th Floor, County Hall/The Riverside Building, SE1 7PB, UK

www.cloudflare.com