Skip to content

bpf: add IPv6 mcast addr helpers and cleanup #39579

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

Merged
merged 1 commit into from
May 29, 2025

Conversation

msune
Copy link
Member

@msune msune commented May 16, 2025

Add IPv6 multicast helper functions that construct the multicast IPv6 and MAC addresses respectively given a unicast IPv6 address.

Cleaned up repeated and/or unnecessary code from tests and pktgen.h.

Added simple (and stupid) unit, just in case.

Please ensure your pull request adheres to the following guidelines:


Minor cleanup, that simplifies IPv6 L2 announcement code. No release note needed

@msune msune requested a review from a team as a code owner May 16, 2025 12:32
@msune msune requested a review from gentoo-root May 16, 2025 12:32
@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 May 16, 2025
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label May 16, 2025
@msune msune force-pushed the ipv6_mc_helpers branch 2 times, most recently from e041068 to d76f3d9 Compare May 19, 2025 09:25
@msune msune force-pushed the ipv6_mc_helpers branch 2 times, most recently from 30042e5 to c7e2acf Compare May 20, 2025 08:21
@msune msune force-pushed the ipv6_mc_helpers branch from c7e2acf to 05dff6c Compare May 20, 2025 21:41
@msune msune requested a review from gentoo-root May 20, 2025 21:41
@msune
Copy link
Member Author

msune commented May 20, 2025

Added utility ipv6_is_mc_mac()

@msune msune force-pushed the ipv6_mc_helpers branch from 05dff6c to 10f0c26 Compare May 23, 2025 13:34
@msune msune force-pushed the ipv6_mc_helpers branch 2 times, most recently from b26352c to 90f80b1 Compare May 29, 2025 09:04
@msune
Copy link
Member Author

msune commented May 29, 2025

@gentoo-root on the complexity issue, pushed a revised version + , the diff from the original commit:

~/dev/cilium2/bpf/tests$ git diff HEAD^ -- ../lib/
diff --git a/bpf/lib/ipv6.h b/bpf/lib/ipv6.h
index c358ddedea..1e25aa82ac 100644
--- a/bpf/lib/ipv6.h
+++ b/bpf/lib/ipv6.h
@@ -197,17 +197,14 @@ static __always_inline bool ipv6_addr_equals(const union v6addr *a,
 static __always_inline
 void ipv6_mc_mac_set(const union v6addr *addr, union macaddr *mac)
 {
-       const union macaddr mac_base_addr = {{0x33, 0x33, 0x00,
-                                             0x00, 0x00, 0x00}};
-
-       memcpy(mac, (void *)&mac_base_addr, 2);
+       mac->addr[0] = mac->addr[1] = 0x33;
        memcpy((__u8 *)mac + 2, (__u8 *)addr + 12, 4);
 }
 
 static __always_inline
 bool ipv6_is_mc_mac(const union v6addr *addr, const union macaddr *mac)
 {
-       union macaddr mc_mac;
+       union macaddr mc_mac __align_stack_8;
 
        ipv6_mc_mac_set(addr, &mc_mac);
        return eth_addrcmp((const union macaddr *)&mc_mac, mac) == 0;
@@ -218,8 +215,7 @@ void ipv6_mc_addr_set(const union v6addr *addr, union v6addr *mc_addr)
 {
        const union v6addr base_addr = { .addr = {0xff, 0x02, 0, 0, 0, 0, 0, 0,
                                          0, 0, 0, 0x01, 0xFF, 0, 0, 0} };
-
-       memcpy(mc_addr, (void *)&base_addr, IPV6_ALEN);
+       *mc_addr = base_addr;
        mc_addr->addr[13] = addr->addr[13];
        mc_addr->addr[14] = addr->addr[14];
        mc_addr->addr[15] = addr->addr[15];

I think this looks much better now. Asking for a re-review, JIC.

@msune msune requested a review from gentoo-root May 29, 2025 09:07
Add IPv6 multicast helper functions that construct the multicast
IPv6 and MAC addresses respectively given a unicast IPv6 address.

Cleaned up repeated and/or unnecessary code from tests and pktgen.h.

Added simple (and stupid) unit, just in case.

Signed-off-by: Marc Suñé <marc.sune@isovalent.com>
@msune msune force-pushed the ipv6_mc_helpers branch from 90f80b1 to 1d75e73 Compare May 29, 2025 09:14
Copy link
Contributor

@gentoo-root gentoo-root left a comment

Choose a reason for hiding this comment

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

pushed a revised version

Looks good to me, thank you!

@msune
Copy link
Member Author

msune commented May 29, 2025

Can someone please merge? No more reviews needed, no need for a release note, no need for tests (will only be exercised by #39648.

Thx

Cc @joestringer

@gentoo-root gentoo-root added the release-note/misc This PR makes changes that have no direct user impact. label May 29, 2025
@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 May 29, 2025
@gentoo-root
Copy link
Contributor

/test

@joestringer
Copy link
Member

no need for tests (will only be exercised by #39648).

Not strictly true. Any refactor can cause build breakage, complexity issues, etc. The rule is: We run the tests. If they're failing, we can investigate. I'll share an example of the triage that can help to identify issues and communicate the status of CI for this specific PR.

Failure triage:

I retriggered the tests and marked the PR for auto-merge.

@joestringer joestringer added this pull request to the merge queue May 29, 2025
@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 May 29, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 29, 2025
@msune
Copy link
Member Author

msune commented May 29, 2025

Not strictly true. Any refactor can cause build breakage, complexity issues, etc

This is not a refactor. The 3 new functions are not called by any program other than the unit test ipv6_test.c, so they will not even be part of the binary. I believe some smoke tests that run on every PR, so not with /test, build with ipv6 enabled, so I don't see how this can be an issue in this very particular case. I understand and furiously agree in general.

@joestringer
Copy link
Member

I stand corrected, I scanned the PR too quickly to notice.

@joestringer joestringer added this pull request to the merge queue May 29, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 29, 2025
@joestringer joestringer added this pull request to the merge queue May 29, 2025
@joestringer
Copy link
Member

Hitting some issues with merge queue; latest is a sigstore infrastructure unavailability: https://github.com/cilium/cilium/actions/runs/15332419707/job/43142145733#step:23:32 . I'll retry.

Merged via the queue into cilium:main with commit 936ef8f May 29, 2025
67 of 68 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/community-contribution This was a contribution made by a community member. 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants