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: fix ipv6 extension header parsing error #24309

Merged
merged 1 commit into from
Mar 30, 2023
Merged

bpf: fix ipv6 extension header parsing error #24309

merged 1 commit into from
Mar 30, 2023

Conversation

chenyuezhou
Copy link
Contributor

@chenyuezhou chenyuezhou commented Mar 11, 2023 •
edited

The ipv6_hdrlen function incorrectly sets the length of the extension header
during parsing, causing cilum to obtain the wrong next header and resulting
in packet loss.

This issue will affect the parsing of IPv6 packets that carry both the "auth"
and other extension headers, such as ipv6/auth/hopbyhop/tcp.

Fixes: 1ce3c7f ("bpf: Skip over IPv6 extension headers")
Fixes: #24187

Signed-off-by: chenyuezhou zcy.chenyue.zhou@gmail.com

@chenyuezhou chenyuezhou requested a review from a team as a code owner March 11, 2023 08:44
@chenyuezhou chenyuezhou requested a review from aspsk March 11, 2023 08:44
@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 Mar 11, 2023
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Mar 11, 2023
@chenyuezhou chenyuezhou marked this pull request as draft March 13, 2023 02:59
borkmann
borkmann reviewed Mar 13, 2023
View reviewed changes
bpf/lib/ipv6.h Outdated Show resolved Hide resolved
@borkmann borkmann added the release-note/bug This PR fixes an issue in a previous release of Cilium. label Mar 13, 2023
@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 Mar 13, 2023
@borkmann borkmann added dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch labels Mar 13, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Mar 13, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.13.1 Mar 13, 2023
@chenyuezhou chenyuezhou force-pushed the fix-ipv6-extension-header-resolve branch from 5c36213 to e46b859 Compare March 13, 2023 12:24
@chenyuezhou chenyuezhou marked this pull request as ready for review March 13, 2023 12:25
@pchaigno pchaigno added the feature/ipv6 Relates to IPv6 protocol support label Mar 13, 2023
@chenyuezhou chenyuezhou force-pushed the fix-ipv6-extension-header-resolve branch 2 times, most recently from 36666d8 to 630d7b4 Compare March 14, 2023 03:36
@chenyuezhou chenyuezhou force-pushed the fix-ipv6-extension-header-resolve branch from 630d7b4 to 9172bb3 Compare March 15, 2023 09:20
@julianwiedmann

This comment was marked as outdated.

Sign in to view
@nebril nebril added this to Needs backport from master in 1.13.2 Mar 15, 2023
@nebril nebril removed this from Needs backport from master in 1.13.1 Mar 15, 2023
@chenyuezhou chenyuezhou force-pushed the fix-ipv6-extension-header-resolve branch from 9172bb3 to 61f95b0 Compare March 16, 2023 02:14
@pchaigno pchaigno requested a review from borkmann March 16, 2023 19:51
@pchaigno
Copy link
Member

pchaigno commented Mar 16, 2023 •
edited by maintainer-s-little-helper bot

/test

Job 'Cilium-PR-K8s-1.16-kernel-4.19' failed:

Click to show.

Test Name

K8sUpdates Tests upgrade and downgrade from a Cilium stable image to master

Failure Output

FAIL: migrate-svc restart count values do not match

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.16-kernel-4.19 so I can create one.

dylandreimerink
dylandreimerink requested changes Mar 17, 2023
View reviewed changes
Copy link
Member

@dylandreimerink dylandreimerink left a comment •
edited

Choose a reason for hiding this comment

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

Very nice! Thank you for the additions to the tests and packet generator. I have one "little" thing we might want to look into

bpf/tests/pktgen.h Outdated Show resolved Hide resolved
@chenyuezhou
Copy link
Contributor Author

rebase is done, pls test

@chenyuezhou chenyuezhou force-pushed the fix-ipv6-extension-header-resolve branch from 5c81392 to 878dfaa Compare March 20, 2023 12:59
@chenyuezhou
Copy link
Contributor Author

Please help to submit the test, I have made some simple adjustments on the test case. thx

julianwiedmann
julianwiedmann requested changes Mar 28, 2023
View reviewed changes
bpf/tests/pktgen.h Outdated Show resolved Hide resolved
@chenyuezhou chenyuezhou force-pushed the fix-ipv6-extension-header-resolve branch from 878dfaa to acef02a Compare March 28, 2023 11:30
@pchaigno
Copy link
Member

pchaigno commented Mar 28, 2023 •
edited by maintainer-s-little-helper bot

/test

Job 'Cilium-PR-K8s-1.25-kernel-4.19' failed:

Click to show.

Test Name

K8sDatapathServicesTest Checks E/W loadbalancing (ClusterIP, NodePort from inside cluster, etc) Tests NodePort inside cluster (kube-proxy) with IPSec and externalTrafficPolicy=Local

Failure Output

FAIL: Request from k8s1 to service http://[fd04::11]:30201 failed

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.25-kernel-4.19 so I can create one.

julianwiedmann
julianwiedmann requested changes Mar 28, 2023
View reviewed changes
Copy link
Member

@julianwiedmann julianwiedmann left a comment •
edited

Choose a reason for hiding this comment

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

coccicheck complains about
file ./tests/pktgen.h: variable ctx on line 181 should be declared constant

@chenyuezhou chenyuezhou force-pushed the fix-ipv6-extension-header-resolve branch 3 times, most recently from 9b9bbb4 to 1190862 Compare March 29, 2023 04:50
@chenyuezhou
Copy link
Contributor Author

fixed coccicheck and rebased

@julianwiedmann
Copy link
Member

Please back out the recent change to proxy.h. It's unrelated to this PR, and will get addressed in #24606.

@chenyuezhou
bpf: fix ipv6 extension header parsing error
48d564d 
The ipv6_hdrlen function incorrectly sets the length of the extension header
during parsing, causing cilum to obtain the wrong next header and resulting
in packet loss.

This issue will affect the parsing of IPv6 packets that carry both the "auth"
and other extension headers, such as `ipv6/auth/hopbyhop/tcp`.

Fixes: 1ce3c7f ("bpf: Skip over IPv6 extension headers")
Fixes: cilium#24187
Signed-off-by: chenyuezhou <zcy.chenyue.zhou@gmail.com>
@chenyuezhou chenyuezhou force-pushed the fix-ipv6-extension-header-resolve branch from 1190862 to 48d564d Compare March 29, 2023 08:21
@chenyuezhou
Copy link
Contributor Author

Revert the modifications to proxy.h.

julianwiedmann
julianwiedmann approved these changes Mar 29, 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
Copy link
Member

/test

@julianwiedmann julianwiedmann added sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. labels Mar 30, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Mar 30, 2023
@julianwiedmann julianwiedmann merged commit e76d074 into cilium:master Mar 30, 2023
42 checks passed
@jibi jibi mentioned this pull request Apr 3, 2023
Merged
11 tasks
@jibi jibi added backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. and removed needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch labels Apr 3, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.13 in 1.13.2 Apr 3, 2023
@jibi jibi added backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. and removed backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. labels Apr 7, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.13 to Backport done to v1.13 in 1.13.2 Apr 7, 2023
@gentoo-root gentoo-root mentioned this pull request Apr 14, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pchaigno pchaigno pchaigno left review comments

@borkmann borkmann borkmann approved these changes

@dylandreimerink dylandreimerink dylandreimerink approved these changes

@julianwiedmann julianwiedmann julianwiedmann approved these changes

@aspsk aspsk Awaiting requested review from aspsk aspsk was automatically assigned from cilium/sig-datapath

Assignees
No one assigned
Labels
backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. feature/ipv6 Relates to IPv6 protocol support 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. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages.
Projects
No open projects
1.13.2
Backport done to v1.13
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cilium is droping ipv6 packets with auth extension header
6 participants
@chenyuezhou @julianwiedmann @pchaigno @borkmann @dylandreimerink @jibi