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.

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

bpf: fix skip_tunnel_nodeport_revnat #33113

Merged
merged 1 commit into from
Jun 20, 2024
Merged

Conversation

lmb
Copy link
Contributor

@lmb lmb commented Jun 13, 2024

bpf: fix skip_tunnel_nodeport_revnat

The test contains a bug since it passes min_port and max_port in struct
ip{4,6}_nat_target in network instead of host byte order. It doesn't fail
because of a series of "fortunate" circumstances, which boil down to
invoking the following function

    __u16 __snat_try_keep_port(__u16 start, __u16 end, __u16 val)
   {
       return val >= start && val <= end ? val :
            __snat_clamp_port_range(start, end, (__u16)get_prandom_u32());
   }

with the following arguments:

    __snat_try_keep_port(htons(port), htons(port), port)

since htons(port) <= port <= htons(port) is (almost) never true, we end up
invoking

    __u16 __snat_clamp_port_range(__u16 start, __u16 end,  __u16 val)
   {
       return (val % (__u16)(end - start)) + start;
   }

Since start == end, we end up executing val % 0. This seems to cause UB
which somehow leads to the code just using val. Of course, this all comes
crashing down when trying to modify any of the code in this path. Fix the
test by converting to host byte order as appropriate.

Fixes: c1761f75ec ("bpf: Implement handling of flag_skip_tunnel for revNAT
nodeport traffic") Signed-off-by: Lorenz Bauer <lmb@isovalent.com>

@lmb lmb added sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. release-note/minor This PR changes functionality that users may find relevant to operating Cilium. labels Jun 13, 2024
@lmb lmb requested a review from a team as a code owner June 13, 2024 07:19
@lmb lmb requested a review from gentoo-root June 13, 2024 07:19
@lmb
Copy link
Contributor Author

lmb commented Jun 13, 2024

cc @learnitall took me a while to figure this out 😆

@lmb
Copy link
Contributor Author

lmb commented Jun 13, 2024

/test

@lmb
Copy link
Contributor Author

lmb commented Jun 13, 2024

/test

1 similar comment
@lmb
Copy link
Contributor Author

lmb commented Jun 13, 2024

/test

@lmb lmb enabled auto-merge June 13, 2024 11:15
@learnitall
Copy link
Contributor

Woah, nice catch. That's quite the bug 😲. My apologies for the extra work here, I owe you one.

@aanm aanm added release-note/misc This PR makes changes that have no direct user impact. and removed release-note/minor This PR changes functionality that users may find relevant to operating Cilium. labels Jun 17, 2024
@aanm
Copy link
Member

aanm commented Jun 17, 2024

Changed release note label to misc since this is not a user-facing change.

@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 Jun 18, 2024
@lmb
Copy link
Contributor Author

lmb commented Jun 18, 2024

/test

@ti-mo
Copy link
Contributor

ti-mo commented Jun 19, 2024

Removing r2m here, looks like ci-e2e is hitting a failure that doesn't resolve on rerun.

@ti-mo ti-mo removed the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jun 19, 2024
@lmb
Copy link
Contributor Author

lmb commented Jun 19, 2024

/test

@lmb
Copy link
Contributor Author

lmb commented Jun 19, 2024

/test

1 similar comment
@lmb
Copy link
Contributor Author

lmb commented Jun 19, 2024

/test

The test contains a bug since it passes min_port and max_port in
struct ip{4,6}_nat_target in network instead of host byte order.
It doesn't fail because of a series of "fortunate" circumstances,
which boil down to invoking the following function

    __u16 __snat_try_keep_port(__u16 start, __u16 end, __u16 val)
    {
        return val >= start && val <= end ? val :
             __snat_clamp_port_range(start, end, (__u16)get_prandom_u32());
    }

with the following arguments:

    __snat_try_keep_port(htons(port), htons(port), port)

since htons(port) <= port <= htons(port) is (almost) never true, we
end up invoking

    __u16 __snat_clamp_port_range(__u16 start, __u16 end,  __u16 val)
    {
        return (val % (__u16)(end - start)) + start;
    }

Since start == end, we end up executing val % 0. This seems to cause
UB which somehow leads to the code just using val. Of course, this all
comes crashing down when trying to modify any of the code in this path.
Fix the test by converting to host byte order as appropriate and by
extending the valid port range by one.

Fixes: c1761f7 ("bpf: Implement handling of flag_skip_tunnel for revNAT nodeport traffic")
Signed-off-by: Lorenz Bauer <lmb@isovalent.com>
@lmb
Copy link
Contributor Author

lmb commented Jun 19, 2024

/test

@lmb lmb added this pull request to the merge queue Jun 20, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jun 20, 2024
Merged via the queue into cilium:main with commit 8b24e96 Jun 20, 2024
67 checks passed
@lmb lmb deleted the pr/lmb/fix-revnat-test branch June 20, 2024 09:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants