Skip to content

Conversation

smagnani96
Copy link
Contributor

@smagnani96 smagnani96 commented Oct 10, 2024

This PR adds additional checks after the ipv6_hdrlen function call to make sure it didn't fail before continuing processing the packet with ctx_is_wireguard. In case of failure, stop trying to mark it as encrypted, but do not drop the packet. In case we need to drop it, it'll be in the following tailcall handle_ipv6, whose processing logic actually relies on that value.

Offending commit hash: a732cd7
Offending PR: #35183

add checks to ipv6_hdrlen return value usage during wireguard tracing in ingress path

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Oct 10, 2024
@smagnani96 smagnani96 changed the title wireguard tracing add check on ipv6_hdrlen for failures wireguard tracing limiting and fixes on ipv6_hdrlen for failures Oct 10, 2024
@smagnani96 smagnani96 force-pushed the pr/wg-encrypt-ipv6-check branch from 9eb4bf5 to 9fba8ae Compare October 10, 2024 17:12
@smagnani96 smagnani96 changed the title wireguard tracing limiting and fixes on ipv6_hdrlen for failures wireguard add checkfixes on ipv6_hdrlen for failures Oct 10, 2024
@smagnani96 smagnani96 changed the title wireguard add checkfixes on ipv6_hdrlen for failures wireguard add check on ipv6_hdrlen for failures and restore trace reason Oct 10, 2024
@smagnani96
Copy link
Contributor Author

/test

@smagnani96 smagnani96 force-pushed the pr/wg-encrypt-ipv6-check branch from 9fba8ae to 0b55d00 Compare October 11, 2024 08:58
@smagnani96 smagnani96 changed the title wireguard add check on ipv6_hdrlen for failures and restore trace reason wireguard add check on ipv6_hdrlen for failures Oct 11, 2024
@smagnani96
Copy link
Contributor Author

/test

@smagnani96 smagnani96 force-pushed the pr/wg-encrypt-ipv6-check branch 2 times, most recently from db9d11c to 2a01586 Compare October 21, 2024 13:45
@smagnani96
Copy link
Contributor Author

/test

@smagnani96
Copy link
Contributor Author

/ci-eks

@smagnani96 smagnani96 marked this pull request as ready for review October 22, 2024 10:14
@smagnani96 smagnani96 requested a review from a team as a code owner October 22, 2024 10:14
@smagnani96 smagnani96 requested a review from jibi October 22, 2024 10:14
@julianwiedmann julianwiedmann self-requested a review October 22, 2024 11:45
@julianwiedmann julianwiedmann added area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. release-note/misc This PR makes changes that have no direct user impact. feature/wireguard Relates to Cilium's Wireguard feature labels Oct 23, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Oct 23, 2024
This commit adjusts the name of a variable in the `do_netdev` function.
Instead of directly naming `l4_off`, let's opt for `hdrlen` so that, when
adding check in the following commit, it is more clear what are we testing.
Adjusting values passed to the `ctx_is_wireguard` accordingly with ETH_HLEN.

Signed-off-by: Simone Magnani <simone.magnani@isovalent.com>
This commit introduces a check to the output of `ipv6_hdrlen`
before calling the ctx_is_wireguard` function for setting the tracing
encryption bit to WireGuard packets. In case of no errors, execute
`ctx_is_wireguard` and set the encryption bit in case of WireGuard
packets. Otherwise, continue without trying to set the encryption bit to
the trace event. We do not directly drop the packet here since tracing
does not necessarily rely on `ipv6_hdrlen`. The subsequent tail call
(`handle_ipv6`) will handle such error case.

Offending commit hash: a732cd7
Offending PR: cilium#35183

Signed-off-by: Simone Magnani <simone.magnani@isovalent.com>
@smagnani96 smagnani96 force-pushed the pr/wg-encrypt-ipv6-check branch from 2a01586 to c31da0a Compare October 23, 2024 13:22
@smagnani96
Copy link
Contributor Author

/test

@julianwiedmann julianwiedmann self-requested a review October 23, 2024 13:24
Copy link
Member

@julianwiedmann julianwiedmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, thank you!

@jibi jibi removed their request for review October 24, 2024 12:04
@jibi jibi added this pull request to the merge queue Oct 24, 2024
Merged via the queue into cilium:main with commit a2cf84e Oct 24, 2024
65 checks passed
@maintainer-s-little-helper maintainer-s-little-helper bot added ready-to-merge This PR has passed all tests and received consensus from code owners to merge. labels Oct 24, 2024
@smagnani96 smagnani96 deleted the pr/wg-encrypt-ipv6-check branch March 18, 2025 11:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. feature/wireguard Relates to Cilium's Wireguard feature ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants