Page MenuHomeFreeBSD

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

Authored by bms on Sat, Aug 1, 10:17 AM.

Details

Reviewers
glebius
pouria
imp
ae
melifaro
adrian
Group Reviewers
network
Summary

Add support for allowing IPv4 multicast groups to be joined on IPv6 sockets,
as a number of applications began to rely on this over the years, despite it
only ever having been a convenience which appeared in Solaris & Linux over
the course of the 00s decade. It is limited to any-source joins (ASM).

To avoid further quibbling over the meaning of the term "undocumented" as it
applies to this change, I have chosen to use the wording "non-IETF-ratified
extension" in comments, with reference to the updated ip6(4) man page.

PR: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=193246

Test Plan

This is a best effort feature only. https://reviews.freebsd.org/D58589 extends mtest(8) to use an IPv4 mapped mreq, rather than going straight to a Kyua/ATF test; the code might be immediately recycled as such.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 75331
Build 72214: arc lint + arc unit

Event Timeline

bms requested review of this revision.Sat, Aug 1, 10:17 AM

Root cause analysis revealed 2 kernel-space issues:

in_ifprimaryaddr() itself looked completely fine. Any impact of functional
regression would have been limited to the new feature itself.

The EFAULT was happening because inp_join_group() assumes user-space for
copyin of its own socket options. SMAP might actually be preventing a panic
here, so whilst it may have saved the kernel operationally, it had arguably
obscured the bug's provenance. Whilst sooptcopyin() is mentioned in socket(9),
the manual page does not describe the SMAP behaviour; it only mentions it as a
helper function.

For FreeBSD, just set sopt_td to NULL to elide to a memcpy().
xnu special-cased the copyin by passing kernproc in its legacy sopt_p member.

The unnecessary cast back to imr_interface's own type in the IA_SIN() macro was
obfuscation, so it was removed.

Nice work, do you have plan to create an I-D for it?

sys/netinet6/in6_mcast.c
1922

Do we need a ifa_ref here?
We exit epoch below without any reference to our ia.

Nice work, do you have plan to create an I-D for it?

With all due respect... NO WAY! It's not my job to "mop up" after the others like this.

This whole thing constitutes an example of a "de facto" standard, as opposed to a "de jure" one:
https://en.wikipedia.org/wiki/De_facto_and_de_jure
https://en.wikipedia.org/wiki/De_facto_standard

Solaris/Linux started doing it in the mid 00s. At first the OpenJDK could deal with discrete IPv4/IPv6 multicast APIs, though it might not have adopted RFC 3678 in full. Eventually it just started doing the IPv4 mapped group thing, and Windows followed suit also.

Python certainly didn't adopt RFC 3678., and my patch for CPython at that time was ignored. FreeBSD was "Overcome by Events". Although the issue was fixed in xnu, that change never got upstreamed to us.

I haven't checked closely in datatracker.ietf.org to see if anyone addressed the gap in the normative specs, I really just want to get this out of the door so I can land future stuff.

https://datatracker.ietf.org/doc/html/rfc3678#autoid-27

sys/netinet6/in6_mcast.c
1922

There should be need to call ifa_ref() as the relevant value of ia->ia_addr is being copied by value before the epoch is left. See: https://reviews.freebsd.org/D55344

sys/netinet6/in6_mcast.c
1922

For some reason Phabricator is not letting me edit the previous inline comment: There should be NO NEED to bump ifa_ref() on the ia; it's already being copied from by value.

bms added inline comments.
sys/netinet6/in6_mcast.c
1922

Because it's reasonable to explain why protecting ia accesses with ifa_ref() is reasonable in most cases, and I have drunk a lot of coffee this Monday:

In this case, it would just be a "garden rake": Whilst this might arguably open a race, there really is no race: the join or leave is being instigated from userland, and the ia_addr is the key referent at that time. That's an IPv4 address, not a pointer, so there's no pointer reference to protect.

If the join fails because the ifnet and/or ia_addr associated with the join went away, userland SHOULD fail gracefully; the situation is no different here as if userland had invoked the IPv4 RFC 3678 APIs as originally foreseen, despite the call being proxied from within in6_mcast.c.

If a leave fails for the same reason, it would not race the teardown of mcast state associated with the ia_addr anyway.

In practice, for IPv4, this only concerns the ia referenced by in_ifprimaryaddr() anyway, until such time as someone adds IPv4 source address selection; @bz historically had changes for this which he stated that he did not commit for performance reasons.