bpf: misc ICMPv6 ND fixes (srcip, nollsrcopt...) by msune · Pull Request #36024 · cilium/cilium

msune · 2024-11-18T21:41:34Z

(Stubbed / dependency of PR #34983)

This patchset:

ebead7f9c0 bpf/{lib|tests}: fix ICMPv6 NS w/o LLSRC handling
ce604f1096 bpf: add ICMPv6 ND mcast coverage
bbdd2ac0e8 bpf/{lib|test}: send ICMP NA from target SRC_IP
8f433835a2 bpf/lib/icmpv6: remove ifdef IPV6

See commit logs for details.

Improve ICMPv6 Neighbor Discovery packet handling

msune · 2024-11-18T21:45:04Z

Please note commit 8f43383 makes these two BPF tests fail:

$ make run
go test ./bpftest -bpf-test-path /home/marc/dev/cilium2/bpf/tests -exec "sudo -E"
--- FAIL: TestBPF (9.97s)
    --- FAIL: TestBPF/bpf_nat_tests.o (0.01s)
        bpf_test.go:169: loading collection: inlining global data: global constants must be in .rodata.config, but found reference to .bss
    --- FAIL: TestBPF/fib_tests.o (0.00s)
        bpf_test.go:169: loading collection: inlining global data: global constants must be in .rodata.config, but found reference to .bss
FAIL
FAIL	github.com/cilium/cilium/bpf/tests/bpftest	10.009s
FAIL
make: *** [Makefile:86: run] Error 1

I can't see anything "problematic" in icmpv6.h, so I suspect it has more to do with the tests themselves. I would appreciate some guidance on this, cc @ti-mo.

msune · 2024-11-19T11:35:01Z

checkpatch warnings and errors should be fixed now.

gentoo-root

Looks good to me in general; I haven't analyzed the CI failures (yet?).

bpf/tests/ipv6_ndp_from_netdev_test.c

bpf/lib/icmp6.h

bpf/tests/ipv6_ndp_from_netdev_test.c

ti-mo

I can't see anything "problematic" in icmpv6.h, so I suspect it has more to do with the tests themselves. I would appreciate some guidance on this, cc @ti-mo.

There's indeed nothing problematic in linux/icmpv6.h. fib_tests.c has had some data in .bss since forever, redir_recorder and redir_neigh_recorder are global vars:

~ llvm-objdump -SD bpf/tests/fib_tests.o
[...]
Disassembly of section .bss:

0000000000000000 <redir_recorder>:
...

0000000000000018 <redir_neigh_recorder>:
...

The inliner happily accepted it since bpf tests all override configurables like THIS_INTERFACE_MAC using compile-time macros, meaning there are no references to .rodata.config. This makes the following statement return early:

cilium/pkg/bpf/collection.go

Lines 437 to 441 in 11da06e

    
           if offsets == nil { 
        
           	// Most likely all references to global data have been compiled 
        
           	// out. 
        
           	return nil 
        
           }

.. so the .bss reference doesn't trigger the error.

With the guard removed, lib/icmp6.h is compiled in unconditionally, creating a reference to .rodata.config, triggering the inliner. I'm removing this restriction in #35988 if you can wait.

Keep in mind the removal of the ifdef cannot be backported for this reason, so best to drop the change or send it as a separate PR. It will need to happen at some point if we ever want to go clang-free at runtime anyway.

bpf/lib/icmp6.h

msune · 2024-11-20T12:36:15Z

There's indeed nothing problematic in linux/icmpv6.h. fib_tests.c has had some data in .bss since forever, redir_recorder and redir_neigh_recorder are global vars: ..

Thx @ti-mo

Keep in mind the removal of the ifdef cannot be backported for this reason, so best to drop the change or send it as a separate PR. It will need to happen at some point if we ever want to go clang-free at runtime anyway.

Yeah, maybe it's a good. Idea; I wanted to see whether there was a way to fix the code in either icmpv6.h or those two tests, and already integrate it, but seems a futile job if #35988 makes it in. I will add it as part of #34983 (which is where originally was).

msune · 2024-11-21T15:30:52Z

/test

gandro · 2024-11-25T13:14:19Z

/test

msune · 2024-11-25T13:30:47Z

Rebased as all integration/conformance tests were failing on the Envoy thing...

gandro · 2024-11-25T13:45:57Z

/test

ldelossa · 2024-12-04T14:40:17Z

@msune is this ready to merge? Looks like convo with Timo is still going on. If its not, please remove the ready-to-merge label.

msune · 2024-12-04T15:19:14Z

@msune is this ready to merge? Looks like convo with Timo is still going on.

I am awaiting input from @ti-mo on the #pragma once vs guard. From my perspective is ready.

If its not, please remove the ready-to-merge label.

I can't, neither trigger the CI (not a cilium project member). Sad.. 😂

gentoo-root · 2024-12-04T15:28:10Z

I am awaiting input from ti-mo on the #pragma once vs guard.

I thought we moved to #pragma once: fe9272d

If there are leftovers, they should be cleaned up.

msune · 2024-12-04T15:30:04Z

I thought we moved to #pragma once: fe9272d

If there are leftovers, they should be cleaned up.

Thx, I didn't know that. I will move to #pragma once then!

joestringer · 2025-02-25T17:31:28Z

@dylandreimerink any thoughts on test coverage per the prior comment? #36024 (comment)

dylandreimerink · 2025-02-26T11:32:49Z

The PR needs a re-review on the BPF unit tests, as I refactored them quite a bit to reduce quite a few lines of code

I think the changes to the tests seem great. Having the common parts as functions like this seems neat.

any thoughts on test coverage per the prior comment?

I think we have test cases for the behavior we are interested in, but its hard to say from just reading the code if this covers all of the logic we have. This PR will definitely be an improvement over the current situation.

msune · 2025-03-19T15:01:04Z

Rebased, removed Draft again.

As I was saying here I would feel more comfortable if someone would answer question 2).

But at this point feel free to merge, and we shall see I guess.

(Give me 30min to fix checkpatch warnings on the rebase)

msune · 2025-03-19T15:45:58Z

(Give me 30min to fix checkpatch warnings on the rebase)

Note the checkpatch error comes from the rebase (so it's in main):

bpf: add ICMPv6 ND mcast coverage
=========================================================
Warning: WARNING:LONG_LINE: line length of 107 exceeds 100 columns
#23: FILE: bpf/tests/ipv6_ndp_from_netdev_test.c:20:
+DEFINE_IPV6(NODE_IPV6, 0xbe, 0xef, 0x0, 0x0, 0x0, 0x0, 0x0, 0x1, 0x0, 0x0, 0xa, 0x0, 0x2, 0xf, 0xff, 0xff);


NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

"[PATCH] bpf: add ICMPv6 ND mcast coverage" has style problems, please review.

I am just moving it to a different location in the file for readability.

rgo3

Looks like I don't own any of the files in this PR, so removing myself as reviewer. As a side note, this PR will probably need a rebase after #38244 goes in (would also solve the checkpatch complaint I think). Or vice versa of course.

ti-mo · 2025-03-26T14:44:56Z

@msune I'll take care of fixing this up and getting it merged, thanks for sticking with it. 🙂 #38244 should get merged in a few moments.

ti-mo · 2025-03-31T15:03:26Z

/test

ti-mo · 2025-04-01T07:18:25Z

/test

Prior to this commit, Neighbour Solicitation replies (Neighbour Advertisments) were sent from ROUTER_IP. RFC4861, section 4.4 explicitly mentions that the Source Address must be: > An address assigned to the interface from which the advertisement > is sent. In addition, the current BPF ARP implementation returns the ARP header with the target_ip as source IP. This commit changes the current implementation of icmp6_send_reply() to use the target_ip as source IP of ICMP NA messages. Signed-off-by: Marc Suñé <[email protected]> Signed-off-by: Timo Beckers <[email protected]>

This commit refactors and adds two more tests to ipv6_ndp_from_netdev_test.c covering multicast (broadcast) IPv6 neighbour solicitation, both for node IPs (to stack) and for endpoints (BPF). Signed-off-by: Marc Suñé <[email protected]> Signed-off-by: Timo Beckers <[email protected]>

Prior to this commit, the ICMPv6 Neighbour Solicitation BPF responder (Pods) implementation assumed that the Link Layer Source Address option was **always** present in the packet. According to spec, this option is - actually - optional for NS messages, and SHALL NOT be used in particular cases: > According to RFC4861, sections 4.3 and 7.2.2 unicast neighbour > solicitations (reachability check) SHOULD but are NOT REQUIRED to > include the SRC_LL_ADDR option in the NS message. > > Likewise, neighbour solicitations during Duplicate Address Detection > (DAD, RFC4862), SRC_LL_ADDR option must not present. This resulted in NS messages being dropped when attempting to access LLSRC option (out of bounds check). This commit: * Fixes handling of ICMPv6 NS packets by allocating via ctx_change_tail() enough space for LL_SRC option when not originally present. * Adds test coverage for all combinations Targeted/Bcast, Node/Pod and w/ and w/o LL_SRC option. Signed-off-by: Marc Suñé <[email protected]> Signed-off-by: Timo Beckers <[email protected]>

Remove redundancy in test code for ICMPv6, partially existent and partially introduced in previous commits. Add some more assertions e.g. ICMPv6 type/length. Signed-off-by: Marc Suñé <[email protected]> Signed-off-by: Timo Beckers <[email protected]>

ti-mo · 2025-04-02T09:54:18Z

/test

Rebased to pick up #38566.

joestringer · 2025-05-01T17:16:43Z

It looks like we missed the release note here in the PR description before merging. I've taken a stab at updating it, please double check it and fix it if I got it wrong.

msune requested a review from a team as a code owner November 18, 2024 21:41

msune requested a review from gentoo-root November 18, 2024 21:41

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 Nov 18, 2024

github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Nov 18, 2024

msune force-pushed the fix_ipv6_srcip_nollsrc branch from ebead7f to 6870183 Compare November 19, 2024 11:34

msune force-pushed the fix_ipv6_srcip_nollsrc branch from 6870183 to 009a04d Compare November 19, 2024 12:04

gentoo-root approved these changes Nov 19, 2024

View reviewed changes

bpf/tests/ipv6_ndp_from_netdev_test.c Outdated Show resolved Hide resolved

bpf/lib/icmp6.h Show resolved Hide resolved

bpf/tests/ipv6_ndp_from_netdev_test.c Outdated Show resolved Hide resolved

msune force-pushed the fix_ipv6_srcip_nollsrc branch from 009a04d to 1a88234 Compare November 19, 2024 15:02

ti-mo reviewed Nov 20, 2024

View reviewed changes

bpf/lib/icmp6.h Outdated Show resolved Hide resolved

ti-mo added the release-note/misc This PR makes changes that have no direct user impact. label Nov 20, 2024

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 Nov 20, 2024

msune force-pushed the fix_ipv6_srcip_nollsrc branch from 1a88234 to 97a44c2 Compare November 20, 2024 12:39

msune force-pushed the fix_ipv6_srcip_nollsrc branch from 97a44c2 to c9eb43b Compare November 25, 2024 13:30

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 Nov 28, 2024

aanm enabled auto-merge November 28, 2024 15:27

ldelossa added the dont-merge/discussion A discussion is ongoing and should be resolved before merging, regardless of reviews & tests status. label Dec 4, 2024

joestringer removed the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Dec 9, 2024

auto-merge was automatically disabled December 11, 2024 11:36
Head branch was pushed to by a user without write access

msune force-pushed the fix_ipv6_srcip_nollsrc branch from c9eb43b to 6d0068d Compare December 11, 2024 11:36

joestringer added the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Feb 25, 2025

msune force-pushed the fix_ipv6_srcip_nollsrc branch from 2f25345 to 742178c Compare March 19, 2025 14:59

msune marked this pull request as ready for review March 19, 2025 14:59

msune mentioned this pull request Mar 24, 2025

Neighbor Advertisement Not Returned for duplicate address detect (DAD) #18928

Closed

2 tasks

rgo3 reviewed Mar 26, 2025

View reviewed changes

rgo3 requested review from rgo3 and removed request for rgo3 March 26, 2025 10:13

ti-mo force-pushed the fix_ipv6_srcip_nollsrc branch from 742178c to 742d9ee Compare March 31, 2025 15:03

ti-mo removed the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Apr 1, 2025

ti-mo force-pushed the fix_ipv6_srcip_nollsrc branch from 742d9ee to d15b163 Compare April 1, 2025 07:18

ti-mo enabled auto-merge April 1, 2025 07:18

msune added 4 commits April 2, 2025 11:52

bpf: refactor ICMPv6 ND tests

3317a6a

Remove redundancy in test code for ICMPv6, partially existent and partially introduced in previous commits. Add some more assertions e.g. ICMPv6 type/length. Signed-off-by: Marc Suñé <[email protected]> Signed-off-by: Timo Beckers <[email protected]>

ti-mo force-pushed the fix_ipv6_srcip_nollsrc branch from d15b163 to 3317a6a Compare April 2, 2025 09:53

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 Apr 2, 2025

ti-mo added this pull request to the merge queue Apr 2, 2025

Merged via the queue into cilium:main with commit 050ce81 Apr 2, 2025
67 checks passed

msune mentioned this pull request May 20, 2025

[DRAFT][RFC] L2 announce: add IPv6 support #34983

Closed

	if offsets == nil {
	// Most likely all references to global data have been compiled
	// out.
	return nil
	}

Conversation

msune commented Nov 18, 2024 • edited by joestringer Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

msune commented Nov 18, 2024

Uh oh!

msune commented Nov 19, 2024

Uh oh!

gentoo-root left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ti-mo left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

msune commented Nov 20, 2024

Uh oh!

msune commented Nov 21, 2024

Uh oh!

gandro commented Nov 25, 2024

Uh oh!

msune commented Nov 25, 2024

Uh oh!

gandro commented Nov 25, 2024

Uh oh!

ldelossa commented Dec 4, 2024

Uh oh!

msune commented Dec 4, 2024

Uh oh!

gentoo-root commented Dec 4, 2024

Uh oh!

msune commented Dec 4, 2024

Uh oh!

joestringer commented Feb 25, 2025

Uh oh!

dylandreimerink commented Feb 26, 2025

Uh oh!

msune commented Mar 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

msune commented Mar 19, 2025

Uh oh!

rgo3 left a comment

Choose a reason for hiding this comment

Uh oh!

ti-mo commented Mar 26, 2025

Uh oh!

ti-mo commented Mar 31, 2025

Uh oh!

ti-mo commented Apr 1, 2025

Uh oh!

ti-mo commented Apr 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

joestringer commented May 1, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

msune commented Nov 18, 2024 •

edited by joestringer

Loading

ti-mo left a comment •

edited

Loading

msune commented Mar 19, 2025 •

edited

Loading

ti-mo commented Apr 2, 2025 •

edited

Loading