Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

bpf/nat: fix current behavior that is silently ignoring errors in a revSNAT context #19753

Merged
merged 3 commits into from May 11, 2023
Merged

bpf/nat: fix current behavior that is silently ignoring errors in a revSNAT context #19753

merged 3 commits into from May 11, 2023

Conversation

sahid
Copy link
Member

@sahid sahid commented May 9, 2022 •
edited

The PR is fixing current behavior of nodeport in context of a revSNAT that is silently ignoring errors to process them to the main path anyway.

  • In the first patch we remove usage of DROP_NAT_UNSUPP_PROTO in favor of NAT_PUNT_TO_STACK considering that we want to be conservative in such situation.
  • In the second patch we we have determined errors that can happen from a revSNAT
    point of view to only accept those that are legitimate.
  • The last one clarify comment.

@sahid sahid requested review from a team and joamaki May 9, 2022 14:35
@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 May 9, 2022
brb
brb reviewed May 11, 2022
View reviewed changes
bpf/lib/nodeport.h Outdated Show resolved Hide resolved
@sahid sahid changed the title bpf/nodeport: no need to tall call for a DROP_INVALID bpf/nodeport: reduce number of paths that tail calls IPV{4|6}_FROM_LXC May 19, 2022
brb
brb reviewed May 23, 2022
View reviewed changes
bpf/lib/nodeport.h Outdated Show resolved Hide resolved
@sahid sahid force-pushed the gotoerr branch 3 times, most recently from a706022 to 36e6ce6 Compare June 17, 2022 07:59
@sahid sahid changed the title bpf/nodeport: reduce number of paths that tail calls IPV{4|6}_FROM_LXC bpf/nodeport: avoid calling tail call for a DROP_INVALID Jun 17, 2022
@aanm

This comment was marked as outdated.

Sign in to view
@maintainer-s-little-helper maintainer-s-little-helper bot mentioned this pull request Jun 17, 2022
Closed
@sayboras

This comment was marked as outdated.

Sign in to view
@aanm aanm requested a review from brb July 8, 2022 10:47
@aanm aanm added the release-note/misc This PR makes changes that have no direct user impact. label Jul 8, 2022
@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 Jul 8, 2022
@aanm

This comment was marked as outdated.

Sign in to view
@brb
Copy link
Member

brb commented Jul 11, 2022

Considering #20425, this PR might be not needed. Moving to draft for now.

@brb brb marked this pull request as draft July 11, 2022 12:40
@github-actions

This comment was marked as outdated.

Sign in to view
@github-actions github-actions bot added the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Aug 27, 2022
@sahid sahid marked this pull request as ready for review August 29, 2022 13:37
@sahid sahid requested a review from a team as a code owner August 29, 2022 13:37
@github-actions github-actions bot removed the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Aug 30, 2022
@sayboras

This comment was marked as outdated.

Sign in to view
@julianwiedmann
Copy link
Member

cc @YutaroHayakawa , as you recently introduced the same checks for handle_inter_cluster_revsnat() :)

@sahid sahid changed the title bpf/nodeport: re-circle back on main path for specified errors bpf/nodeport: fix current behavior that silently is silently ignoring errors in a revSNAT context Apr 23, 2023
@sahid
Copy link
Member Author

sahid commented Apr 23, 2023

/test

julianwiedmann
julianwiedmann reviewed Apr 28, 2023
View reviewed changes
bpf/lib/nat.h Outdated Show resolved Hide resolved
@sahid sahid requested a review from julianwiedmann May 3, 2023 11:05
sahid added 3 commits May 3, 2023 13:15
@sahid
bpf/rev-snat: replace DROP_NAT_UNSUPP_PROTO with NAT_PUNT_TO_STACK
846401e 
The change is replacing DROP_NAT_UNSUPP_PROTO for rev-SNAT
context. Any returns using DROP_NAT_UNSUPP_PROTO will be replaced with
NAT_PUNT_TO_STACK.

In case that rev-SNAT is not supporting a protocol we want the process
to continue anyway processing the packet to the main path, considering
that an other layer may use it.

NOTE: This does not add any functional changes and keeps the current
behavior that looks to be the less intrusive.

Signed-off-by: Sahid Orentino Ferdjaoui <sahid.ferdjaoui@industrialdiscipline.com>
@sahid
bpf/nodeport: re-circle back on main path for specified errors
b912c85 
The current behavior is re-circling back to the main path for all
errors returned from the rev-SNAT process, eg. when the revSNAT code
tries to rewrite the packet headers.

In this commit we have determined errors from a rev-SNAT context to
only accept those that are legitimate.

This fix try to be conservative, for that is still accepts
DROP_NAT_NO_MAPPING where in a normal situation should never happen.

Signed-off-by: Sahid Orentino Ferdjaoui <sahid.ferdjaoui@industrialdiscipline.com>
@sahid
nodeport: add clarity in comments when recircle back to main path
c88440d 
This has been suggested by @zacharysarah

Co-authored-by: sarah@corleissen.com
Signed-off-by: Sahid Orentino Ferdjaoui <sahid.ferdjaoui@industrialdiscipline.com>
@joestringer
Copy link
Member

/test

julianwiedmann
julianwiedmann approved these changes May 8, 2023
View reviewed changes
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.

Thanks!

@julianwiedmann julianwiedmann dismissed brb’s stale review May 8, 2023 10:59

discussed offline

@julianwiedmann julianwiedmann added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label May 8, 2023
@julianwiedmann
Copy link
Member

FWIW, I have a feeling we might want to promote this to release-note/bug and backport to v1.13.

@julianwiedmann julianwiedmann changed the title bpf/nodeport: fix current behavior that silently is silently ignoring errors in a revSNAT context bpf/nat: fix current behavior that is silently ignoring errors in a revSNAT context May 8, 2023
@youngnick youngnick added release-note/bug This PR fixes an issue in a previous release of Cilium. and removed release-note/misc This PR makes changes that have no direct user impact. labels May 11, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed ready-to-merge This PR has passed all tests and received consensus from code owners to merge. labels May 11, 2023
@youngnick
Copy link
Contributor

I've changed this to release-note/bug. I'll leave discussion of backports to others though.

@youngnick youngnick merged commit 29e92e4 into cilium:main May 11, 2023
56 checks passed
julianwiedmann added a commit to julianwiedmann/cilium that referenced this pull request May 19, 2023
@julianwiedmann
bpf: nat: tolerate unhandled protocols in revSNAT path
24bef32 
In the past we would just pass up packets with unknown protocols, and let
the next layer (or the stack) deal with them.

cilium#19753 recently added error handling
for the revSNAT paths, so that we can catch errors while eg. rewriting the
packet headers.

But we want to preserve the old behaviour of tolerating unknown protocol
types. So return NAT_PUNT_TO_STACK from a few additional ICMP paths.

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
julianwiedmann added a commit to julianwiedmann/cilium that referenced this pull request May 29, 2023
@julianwiedmann
bpf: nat: tolerate unhandled protocol types in revSNAT path
aa54c25 
In the past we would just pass up packets with unknown protocols, and let
the next layer (or the stack) deal with them.

cilium#19753 recently added error handling
for the revSNAT paths, so that we can catch errors while eg. rewriting the
packet headers.

But we want to preserve the old behaviour of tolerating unknown protocol
types. So return NAT_PUNT_TO_STACK from a few additional ICMP paths.

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
@julianwiedmann julianwiedmann mentioned this pull request May 29, 2023
Merged
julianwiedmann added a commit that referenced this pull request May 30, 2023
@julianwiedmann
bpf: nat: tolerate unhandled protocol types in revSNAT path
3f04408 
In the past we would just pass up packets with unknown protocols, and let
the next layer (or the stack) deal with them.

#19753 recently added error handling
for the revSNAT paths, so that we can catch errors while eg. rewriting the
packet headers.

But we want to preserve the old behaviour of tolerating unknown protocol
types. So return NAT_PUNT_TO_STACK from a few additional ICMP paths.

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pchaigno pchaigno pchaigno approved these changes

@zacharysarah zacharysarah zacharysarah approved these changes

@YutaroHayakawa YutaroHayakawa YutaroHayakawa approved these changes

@julianwiedmann julianwiedmann julianwiedmann approved these changes

@joamaki joamaki Awaiting requested review from joamaki

@brb brb Awaiting requested review from brb

Assignees
No one assigned
Labels
kind/community-contribution This was a contribution made by a community member. release-note/bug This PR fixes an issue in a previous release of Cilium.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet