bpf: don't sign-extend IPv6 address components #28417
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
maintainer-s-little-helper
bot
added
the
dont-merge/needs-release-note-label
ti-mo
added
sig/datapath
maintainer-s-little-helper
bot
removed
dont-merge/needs-release-note-label
|
/test |
2 tasks
ti-mo
force-pushed
the
tb/no-sign-extend-ipv6
branch
from
e5290b1
to
b389e2b
Compare
|
/test |
2 tasks
Unqualified integers are treated as `int` in C, but the behaviour when doing
binary operations on them against unsigned integers is surprising, to say the
least.
Consider the following scenario, before this patch:
```
#define DEFINE_IPV6(NAME, a1, a2, a3, a4, a5, a6, a7, a8, a9, a10, a11, a12, a13, a14, a15, a16) \
DEFINE_U64_I(NAME, 1) = bpf_cpu_to_be64( \
(__u64)(a1) << 56 | (__u64)(a2) << 48 | (__u64)(a3) << 40 | \
(__u64)(a4) << 32 | (a5) << 24 | (a6) << 16 | (a7) << 8 | (a8)) .. <elip>
DEFINE_IPV6(ROUTER_IP, 0x0, 0x0, 0x0, 0x0, 0x80, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x80, 0x0, 0x0, 0x0);
```
Both a5 and a13 are set to a byte with a most-significant bit of 1. This
results in the following output in the .data section during compilation:
```
0000000000000038 <ROUTER_IP_1>:
7: ff ff ff ff 80 00 00 00 <unknown>
0000000000000040 <ROUTER_IP_2>:
8: ff ff ff ff 80 00 00 00 <unknown>
```
Since the `<< 24` pushed the 1 bit to now be the most-significant bit of an int,
equivalent to an int32 on most 64-bit architectures, the subsequent OR operation
against a __u64 causes the value to become sign-extended, setting the upper 32
bits of the resulting (unsigned..) integer to 1. Ouch.
This commit treats each member as a u64, and additionally truncates each
macro argument to u8 to prevent any of the arguments overlapping as a
defensive measure.
Signed-off-by: Timo Beckers <timo@isovalent.com>
ti-mo
force-pushed
the
tb/no-sign-extend-ipv6
branch
from
b389e2b
to
0a28eb9
Compare
|
/test |
julianwiedmann
added
kind/bug
This was referenced Oct 5, 2023
ti-mo
commented
Oct 5, 2023
| (__u64)(__u8)(a1) << 56 | (__u64)(__u8)(a2) << 48 | \ | ||
| (__u64)(__u8)(a3) << 40 | (__u64)(__u8)(a4) << 32 | \ | ||
| (__u64)(__u8)(a5) << 24 | (__u64)(__u8)(a6) << 16 | \ | ||
| (__u64)(__u8)(a7) << 8 | (__u64)(__u8)(a8)); \ |
There was a problem hiding this comment.
We don't want any values to spill over into the higher bits, so it's okay to be defensive here.
|
Yes, agree, makes sense! Kudos, amazing catch! |
ti-mo
added
the
backport/author
1 task
ti-mo
added
backport-pending/1.14
maintainer-s-little-helper
bot
moved this from Needs backport from main
to Backport pending to v1.14
in 1.14.3
ti-mo
added
backport-done/1.14
maintainer-s-little-helper
bot
moved this from Backport pending to v1.14
to Backport done to v1.14
in 1.14.3
pippolo84
added a commit
to pippolo84/cilium
that referenced
this pull request
Oct 11, 2023
The update-label-backport-pr workflow cannot handle characters like single quote and double quotes in the body of the PR passed as input. An example of a PR body containing single quotes is cilium#28435 Calling the workflow with that PR as input led to this failure: ``` Run echo '- [x] cilium#28417 -- bpf: don't sign-extend IPv6 address components (@ti-mo) echo '- [x] cilium#28417 -- bpf: don't sign-extend IPv6 address components (@ti-mo) Once this PR is merged, you can update the PR labels via: ```upstream-prs $ for pr in 28417; do contrib/backporting/set-labels.py $pr done 1.14; done ```' | sed -En "/upstream-prs/ { n; p }" | cut -d ';' -f 1 | grep -Eo '[0-9]+' | while read -r pr; do echo "Removing label backport-pending/1.14 from pr #${pr}." gh pr edit ${pr} --repo "${GITHUB_REPOSITORY}" --remove-label backport-pending/"1.14" echo "Adding label backport-done/1.14 to pr #${pr}." gh pr edit ${pr} --repo "${GITHUB_REPOSITORY}" --add-label backport-done/"1.14" done shell: /usr/bin/bash -e {0} env: GITHUB_TOKEN: *** /home/runner/work/_temp/f513ca6f-a4be-4c83-b2fa-7c179befdf5f.sh: line 1: syntax error near unexpected token `(' ``` This is due to the single quote in the word "don't" contained in the backported PR title. To fix the issue an initial step to preprocess the body is added. The step will remove single quotes, double quotes, backtick and dollar signs before parsing the input to get the list of backported PRs. Fixes: cilium#27875 Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>
pippolo84
added a commit
to pippolo84/cilium
that referenced
this pull request
Oct 17, 2023
The update-label-backport-pr workflow cannot handle characters like single quote and double quotes in the body of the PR passed as input. An example of a PR body containing single quotes is cilium#28435 Calling the workflow with that PR as input led to this failure: ``` Run echo '- [x] cilium#28417 -- bpf: don't sign-extend IPv6 address components (@ti-mo) echo '- [x] cilium#28417 -- bpf: don't sign-extend IPv6 address components (@ti-mo) Once this PR is merged, you can update the PR labels via: ```upstream-prs $ for pr in 28417; do contrib/backporting/set-labels.py $pr done 1.14; done ```' | sed -En "/upstream-prs/ { n; p }" | cut -d ';' -f 1 | grep -Eo '[0-9]+' | while read -r pr; do echo "Removing label backport-pending/1.14 from pr #${pr}." gh pr edit ${pr} --repo "${GITHUB_REPOSITORY}" --remove-label backport-pending/"1.14" echo "Adding label backport-done/1.14 to pr #${pr}." gh pr edit ${pr} --repo "${GITHUB_REPOSITORY}" --add-label backport-done/"1.14" done shell: /usr/bin/bash -e {0} env: GITHUB_TOKEN: *** /home/runner/work/_temp/f513ca6f-a4be-4c83-b2fa-7c179befdf5f.sh: line 1: syntax error near unexpected token `(' ``` This is due to the single quote in the word "don't" contained in the backported PR title. To fix the issue an initial step to preprocess the body is added. The step will remove single quotes, double quotes, backtick and dollar signs before parsing the input to get the list of backported PRs. Fixes: cilium#27875 Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>
2 tasks
aanm
pushed a commit
that referenced
this pull request
Oct 17, 2023
The update-label-backport-pr workflow cannot handle characters like single quote and double quotes in the body of the PR passed as input. An example of a PR body containing single quotes is #28435 Calling the workflow with that PR as input led to this failure: ``` Run echo '- [x] #28417 -- bpf: don't sign-extend IPv6 address components (@ti-mo) echo '- [x] #28417 -- bpf: don't sign-extend IPv6 address components (@ti-mo) Once this PR is merged, you can update the PR labels via: ```upstream-prs $ for pr in 28417; do contrib/backporting/set-labels.py $pr done 1.14; done ```' | sed -En "/upstream-prs/ { n; p }" | cut -d ';' -f 1 | grep -Eo '[0-9]+' | while read -r pr; do echo "Removing label backport-pending/1.14 from pr #${pr}." gh pr edit ${pr} --repo "${GITHUB_REPOSITORY}" --remove-label backport-pending/"1.14" echo "Adding label backport-done/1.14 to pr #${pr}." gh pr edit ${pr} --repo "${GITHUB_REPOSITORY}" --add-label backport-done/"1.14" done shell: /usr/bin/bash -e {0} env: GITHUB_TOKEN: *** /home/runner/work/_temp/f513ca6f-a4be-4c83-b2fa-7c179befdf5f.sh: line 1: syntax error near unexpected token `(' ``` This is due to the single quote in the word "don't" contained in the backported PR title. To fix the issue an initial step to preprocess the body is added. The step will remove single quotes, double quotes, backtick and dollar signs before parsing the input to get the list of backported PRs. Fixes: #27875 Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Unqualified integers are treated as
intin C, but the behaviour when doing binary operations on them against unsigned integers is surprising, to say the least.Consider the following scenario, before this patch:
Both a5 and a13 are set to a byte with a most-significant bit of 1. This results in the following output in the .data section during compilation:
Since the
<< 24pushed the 1 bit to now be the most-significant bit of an int, equivalent to an int32 on most 64-bit architectures, the subsequent OR operation against a __u64 causes the value to become sign-extended, setting the upper 32 bits of the resulting (unsigned..) integer to 1. Ouch.This commit treats each member as a u64, and additionally truncates each macro argument to u8 to prevent any of the arguments overlapping as a defensive measure.
Fixes: #27898