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

v1.14: backport IPv6 host and router IP macro fix #28435

Merged
merged 1 commit into from Oct 6, 2023

Conversation

ti-mo
Copy link
Contributor

@ti-mo ti-mo commented Oct 6, 2023

Once this PR is merged, you can update the PR labels via:

$ for pr in 28417; do contrib/backporting/set-labels.py $pr done 1.14; done

[ upstream commit 2c5fa7b ]

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 ti-mo requested a review from a team as a code owner October 6, 2023 09:10
@ti-mo ti-mo added kind/backports This PR provides functionality previously merged into master. backport/1.14 This PR represents a backport for Cilium 1.14.x of a PR that was merged to main. labels Oct 6, 2023
@ti-mo ti-mo changed the title v1.14 backports 2023-10-06 v1.14: backport IPv6 host and router IP macro fix Oct 6, 2023
@ti-mo
Copy link
Contributor Author

ti-mo commented Oct 6, 2023

/test-backport-1.14

@ti-mo
Copy link
Contributor Author

ti-mo commented Oct 6, 2023

Can't approve my own PR, so merging this.

@ti-mo ti-mo merged commit d39d92b into cilium:v1.14 Oct 6, 2023
59 checks passed
@ti-mo ti-mo deleted the pr/v1.14-backport-2023-10-06 branch October 6, 2023 10:41
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>
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
Labels
backport/1.14 This PR represents a backport for Cilium 1.14.x of a PR that was merged to main. kind/backports This PR provides functionality previously merged into master.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant