netinet6: Pass IPv4-mapped ASM multicast joins/leaves to netinet.
Needs ReviewPublic
Actions

Authored by bms on Sun, Feb 1, 4:56 AM.

Details

Reviewers

hrs
glebius
kp

Group Reviewers

network

Summary

Add comment about future IPv4 source-address selection support.

This has been compile tested with LINT and LINT-NOINET,
but has not yet been runtime tested.

EAFNOSUPPORT change moved to D55233.
Track other dependent refactoring requested by Gleb:
D55344
D55345
D55382

Inspired by: xnu (Clean-roomed from APSL change)
PR: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=193246

Test Plan

Kyua tests will follow in a separate commit. I note that not all of the legacy IP multicast regression tests were ported to ATF over the years.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 70795
Build 67678: arc lint + arc unit

Event Timeline

bms created this revision.Sun, Feb 1, 4:56 AM

Herald added subscribers: melifaro, ae, imp. · View Herald TranscriptSun, Feb 1, 4:56 AM

bms requested review of this revision.Sun, Feb 1, 4:56 AM

Harbormaster completed remote builds in B70313: Diff 170927.Sun, Feb 1, 4:56 AM

bms edited the summary of this revision. (Show Details)Sun, Feb 1, 4:59 AM

bms edited the test plan for this revision. (Show Details)

bms added reviewers: bz, hrs, glebius, kp.

The IPv4 mapped behaviour is limited to ASM memberships only, and is not directly mandated by an RFC. The comments around in6u_lookup_v4mapped_mc() reflect this-- it's something that has seen wider use than one would anticipate, despite it being kind of dog stupid to do IPv4 operations on an IPv6 socket.

This revision is also missing a man page update. Historically Pavlin Radoslavov has handled the multicast routing end of things, here it needs to go in ip6(4).

@bz: The Apple xnu code has cleaned up scope ID handling, so that may be
worth abridging from also. They added scope checks to im6o_mc_filter(), conditionalised on their own in6_embedded_scope global.

They renamed in6_joingroup_locked() to in6_mc_join() which is a little confusing, and it looks like they demoted the per-socket SSM filters from an RB-tree to a growable array. That I can understand; stuff gets merged into the stack-wide RB-tree for each group anyway.

However I didn't check what changes they may have made to imo_multi_filter(), yet.

OT: Apple do not provide meaningful change history in their xnu drops on GitHub, which is extremely inconvenient.

OT: I am in touch separately with core-secretary@ to open a dialogue about regaining full commit access after a 7.5 year hiatus for health & bereavement reasons.

Bruce, very glad to see you back!!!

sys/netinet6/in6_mcast.c
1462	I'd suggest to commit all of the changes from EINVALs to EAFNOSUPPORT as a separate commit.
1894	Let's spell as AF_INET6 for consistency.
1899	What does letter `u` in `in6u_` stands for?
1918	IMHO, this macro should go away. We should search for first AF_INET address on the list of addresses of the given interface instead of searching on the global list. btw, this macro is used only in multicast related code. You may want to add the suggested code as a function, e.g. `in_firstaddr()` and place it in in.c next to `in_ifhasaddr()`.
1997–2003	This all can be a static initializer.
2005	Let's add brackets per style(9).
2337–2343	Can be static initializer.
2340	Is the void cast needed at all?
2345	Let's add brackets per style(9).
2354	Why do we need these extra braces?

bms edited the summary of this revision. (Show Details)Tue, Feb 10, 8:54 PM

bms added inline comments.Tue, Feb 10, 8:56 PM

sys/netinet6/in6_mcast.c
1462	Re-raised as D55233.

bz edited reviewers, added: network; removed: bz.Tue, Feb 10, 9:16 PM

bms edited the summary of this revision. (Show Details)Wed, Feb 18, 1:04 PM

bms added a child revision: D55345: netinet: Use in_ifprimaryaddr() in place of IFP_TO_IA()..Thu, Feb 19, 4:28 PM

Rebase

Harbormaster completed remote builds in B70792: Diff 172269.Thu, Feb 19, 7:10 PM

Correctly claim authorship.

Harbormaster completed remote builds in B70795: Diff 172275.Thu, Feb 19, 7:34 PM

bms edited the summary of this revision. (Show Details)Thu, Feb 19, 7:36 PM

Please check that this compiles properly with possible combinations INET, INET6, INET & INET6. It looks to be as if your using INET to protect INET6 only code.

Um, probably wrong on that, as these files well not be pulled in unless compiling INET6

In D55031#1266797, @rgrimes wrote:

Please check that this compiles properly with possible combinations INET, INET6, INET & INET6. It looks to be as if your using INET to protect INET6 only code.

Um, probably wrong on that, as these files well not be pulled in unless compiling INET6

The #ifdef INET is not being used in its normal sense; it is there to denote that the IPv4-mapped address support requires INET, despite being referenced from a file that is covered by the INET6 option. So your initial confusion is understandable.

Also, the only reason this trick works at all is that inpcb->inp_moptions exists and is populated even for PF_INET6 sockets.

pouria added a subscriber: pouria.Thu, Feb 19, 9:05 PM

pouria added inline comments.