bpf: ICMP checksum handling in SNAT RevNAT#43196
Conversation
c3069d6 to
89f6154
Compare
|
Sorry for the delay. |
|
/test |
|
It looks like the CI may be failing because of #15474. You may need to rebase to pass. |
89f6154 to
5902061
Compare
7ab85c9 to
77eb7ca
Compare
77eb7ca to
d003de0
Compare
|
You'll need to address the warning from Check Patch. The two other failures look like flakes. |
julianwiedmann
left a comment
There was a problem hiding this comment.
Thank you! Looks good to me overall - I added a few comments, although I think we shouldn't attempt to optimize the logic further for now. Let's do that once we have addressed the bug.
One aspect I'd still want to confirm is what the effect on skb->csum is. Not sure whether we can easily validate that from within the BPF test suite today.
| static __always_inline void | ||
| snat_v4_calc_icmp_error_csum_diff(__be32 old_addr, __be32 new_addr, | ||
| __be16 old_port, __be16 new_port, | ||
| bool inner_has_l4_csum, __wsum *diff_for_csum) | ||
| { | ||
| __be32 old_port32 = (__be32)old_port; | ||
| __be32 new_port32 = (__be32)new_port; | ||
|
|
||
| *diff_for_csum = 0; | ||
|
|
||
| if (inner_has_l4_csum) { | ||
| /* Calculate diff value for checksum. | ||
| * Reflect the change in the inner L4 checksum caused by the pseudo-address update | ||
| * into diff_for_csum. | ||
| * All the other changes in inner packet cancel each other out. | ||
| */ | ||
| if (old_addr != new_addr) | ||
| *diff_for_csum = csum_diff(&new_addr, 4, &old_addr, 4, 0); | ||
| } else { | ||
| /* Calculate diff value for checksum. | ||
| * If the inner L4 header does not include the L4 checksum, | ||
| * only the port is modified within the inner L4 header. | ||
| */ | ||
| if (old_port != new_port) | ||
| *diff_for_csum = csum_diff(&old_port32, 4, &new_port32, 4, 0); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
I think eventually we'll want to integrate this logic into snat_v4_rewrite_headers() (so it literally tells the caller what csum-relevant changes it made, and the caller can then feed that information into the next stage of header rewrites). But not today.
🤯 for the "truncated L4" scenario, this is such an annoying corner case to handle ...
msune
left a comment
There was a problem hiding this comment.
I only reviewed the Scapy part. Overall LGTM. Minor changes to style + naming required.
Thanks!
e7ad1ad to
67a8a5c
Compare
|
/test |
When a LB node forwards packets to a backend-pod on another node, the packets are SNATed. The corresponding reverse NAT (RevNAT) is then applied to the reply packets. Checksum recaluculation is required when RevNAT. When the reply packet is an ICMP error packet, the checksum in the ICMP header must be recalculated to reflect the changes of the inner packet header. https://datatracker.ietf.org/doc/html/rfc5508#section-4.1 Since that logic was missing, it has been added in this commit. Fixes: cilium#40827 Signed-off-by: Yusho Yamaguchi <ysh.824@outlook.jp>
This commit adds a new test case of SNAT RevNAT using scapy. By this test case, we could also validate checksum of outputed packet. Signed-off-by: Yusho Yamaguchi <ysh.824@outlook.jp>
c4fff34 to
abde4fc
Compare
|
/test |
julianwiedmann
left a comment
There was a problem hiding this comment.
Thank you, looks good to me!
So as follow-up actions, we'd want to
- add more scapy tests (IPv4 and IPv6),
- fix up the forward path (
snat_v4_nat_handle_icmp_error()), - look at the suggested optimizations
for the record - the additional |
|
@julianwiedmann should this be backported? |
|
@yushoyamaguchi would you want to give the |
|
Thank you for your suggestion! To confirm the process:
Does that sound good? |
yep sounds good! There's full documentation here as well. |
|
I've made #43861. There is no |
Fixes: #40827
When a LB node forwards packets to a backend-pod on another node,
the packets are SNATed.
The corresponding reverse NAT (RevNAT) is then applied
to the reply packets.
Checksum recaluculation is required when RevNAT.
When the reply packet is an ICMP error packet, the checksum
in the ICMP header must be recalculated to reflect the changes
of the inner packet header.
https://datatracker.ietf.org/doc/html/rfc5508#section-4.1
Since that logic was missing, it has been added in this commit.
The below is an error message of
nat4_icmp_error_tcp_snat_revnat(new test) without the checksum recalculate patch.An error message of RevNAT BPF-test