bpf: nodeport: fix-up error check in rev_nodeport_lb*() for XDP #23119
Conversation
lb4_extract_tuple() calls ipv4_ct_extract_l4_ports(), which returns CTX_ACT_OK on success. This is 0 in TC, but 2 in XDP. This caused us to erronously bail out from the RevDNAT check in the XDP NAT reply path. Fix up the error check accordingly. Also adjust lb6_extract_tuple() for consistency. Fixes: fd52b2f ("bpf: nodeport: skip CT lookup for non-NodePort replies in RevDNAT path") Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
|
Brown-bag time :/. Unit tests will land via #22756... |
|
/test |
|
Overall this LGTM. I'll give it another day for a chance for additional reviews, then tomorrow either I can merge it or tophat can. I've assigned myself so I know who to follow up with 😅 |
| @@ -1001,7 +1001,7 @@ static __always_inline int rev_nodeport_lb6(struct __ctx_buff *ctx, __u32 *ifind | |||
| #endif | |||
|
|
|||
| ret = lb6_extract_tuple(ctx, ip6, &l4_off, &tuple); | |||
| if (ret) { | |||
| if (ret < 0) { | |||
There was a problem hiding this comment.
| if (ret < 0) { | |
| if (IS_ERR(ret)) { |
There was a problem hiding this comment.
I agree we should be using this style in general, but there are several other places in this file that are not yet using this. @julianwiedmann , would you mind following up on this separately to update these and other remaining if (ret < 0) cases in this file?
There was a problem hiding this comment.
@joestringer yeah I plan to spend a few cycles on the wider error handling scheme.
Think with a bit of effort we should be able to banish CTX_ACT_DROP from all/most our own code (just return a proper negative DROP_* everywhere), and translate all helper calls (eg. for ctx_redirect(), don't we want some DROP_MISSED_BPF_REDIRECT visibility anyway?). At which point IS_ERR() might just turn into a simple ret < 0 check.
| @@ -2006,7 +2006,7 @@ static __always_inline int rev_nodeport_lb4(struct __ctx_buff *ctx, __u32 *ifind | |||
| #endif /* ENABLE_EGRESS_GATEWAY */ | |||
|
|
|||
| ret = lb4_extract_tuple(ctx, ip4, &l4_off, &tuple); | |||
| if (ret) { | |||
| if (ret < 0) { | |||
There was a problem hiding this comment.
| if (ret < 0) { | |
| if (IS_ERR(ret)) { |
lb4_extract_tuple() calls ipv4_ct_extract_l4_ports(), which returns CTX_ACT_OK on success. This is 0 in TC, but 2 in XDP. This caused us to erronously bail out from the RevDNAT check in the XDP NAT reply path.
Fix up the error check accordingly. Also adjust lb6_extract_tuple() for consistency.
Fixes: fd52b2f ("bpf: nodeport: skip CT lookup for non-NodePort replies in RevDNAT path")