* TC bpf_csum_diff problems post-5.6 kernel
@ 2021-11-02 21:46 Thomas Ptacek
2021-11-02 22:00 ` Daniel Borkmann
0 siblings, 1 reply; 6+ messages in thread
From: Thomas Ptacek @ 2021-11-02 21:46 UTC (permalink / raw)
To: xdp-newbies
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: TC bpf_csum_diff problems post-5.6 kernel
2021-11-02 21:46 TC bpf_csum_diff problems post-5.6 kernel Thomas Ptacek
@ 2021-11-02 22:00 ` Daniel Borkmann
2021-11-02 22:08 ` Thomas Ptacek
0 siblings, 1 reply; 6+ messages in thread
From: Daniel Borkmann @ 2021-11-02 22:00 UTC (permalink / raw)
To: ThomasPtacek, xdp-newbies
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: TC bpf_csum_diff problems post-5.6 kernel
2021-11-02 22:00 ` Daniel Borkmann
@ 2021-11-02 22:08 ` Thomas Ptacek
2021-11-02 22:28 ` Daniel Borkmann
0 siblings, 1 reply; 6+ messages in thread
From: Thomas Ptacek @ 2021-11-02 22:08 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: xdp-newbies
!!!
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: TC bpf_csum_diff problems post-5.6 kernel
2021-11-02 22:08 ` Thomas Ptacek
@ 2021-11-02 22:28 ` Daniel Borkmann
2021-11-02 22:33 ` Thomas Ptacek
0 siblings, 1 reply; 6+ messages in thread
From: Daniel Borkmann @ 2021-11-02 22:28 UTC (permalink / raw)
To: ThomasPtacek; +Cc: xdp-newbies, lmb
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
>>>
>>
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: TC bpf_csum_diff problems post-5.6 kernel
2021-11-02 22:28 ` Daniel Borkmann
@ 2021-11-02 22:33 ` Thomas Ptacek
2021-11-03 10:46 ` Lorenz Bauer
0 siblings, 1 reply; 6+ messages in thread
From: Thomas Ptacek @ 2021-11-02 22:33 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: xdp-newbies, lmb
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: TC bpf_csum_diff problems post-5.6 kernel
2021-11-02 22:33 ` Thomas Ptacek
@ 2021-11-03 10:46 ` Lorenz Bauer
0 siblings, 0 replies; 6+ messages in thread
From: Lorenz Bauer @ 2021-11-03 10:46 UTC (permalink / raw)
To: thomasptacek; +Cc: Daniel Borkmann, xdp-newbies
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-11-03 10:46 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-02 21:46 TC bpf_csum_diff problems post-5.6 kernel Thomas Ptacek
2021-11-02 22:00 ` Daniel Borkmann
2021-11-02 22:08 ` Thomas Ptacek
2021-11-02 22:28 ` Daniel Borkmann
2021-11-02 22:33 ` Thomas Ptacek
2021-11-03 10:46 ` Lorenz Bauer
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.