ip_mroute: Make the routing socket private

[markj]

Summary

I have some patches which make ip_mroute and ip6_mroute FIB-aware. This
enables running per-FIB routing daemons, each of which has a separate
routing socket.

Several places in the network stack check whether multicast routing is
configured by checking whether the multicast routing socket is non-NULL.
This doesn't directly translate in my proposed scheme, as each FIB would
have its own socket. I'd like to modify the ip(6)_mroute code to store
all state, including the socket, in a per-FIB structure. So, take a
step towards that and 1) hide the socket, 2) add a boolean flag which
indicates whether a multicast router is registered.

[zlei]

sys/netinet6/ip6_mroute.c

577

I'd like to enable it when it is certainly safe to.

600

Should the V_ip6_mrouting_enabled be turned off immediately ? So the fast path see it as soon as possible ?

I'm struggling with the net epoch... I'm not certain whether a non-atomic read access in the fast path ( probably also want atomic-write in this place ) is sufficient or not.

[pouria]

sys/netinet/ip_mroute.c

744–749

IMHO, @zlei comment on ip6 also applies here.

sys/netinet6/ip6_mroute.c

577

I'd like to enable it when it is certainly safe to.

+1

1911–1912

Shouldn't be (EBUSY)?
IMHO, admin request to unload is not invalid.

[pouria]

sys/netinet/ip_mroute.c

699

What is the meaning of version btw?
AFAIK, this line here is redundant. however, we may choose to kept it for backward compatibility.

in multicast(4):

After the multicast routing socket is open, it can be used to enable or
disable multicast forwarding in the kernel:

/* IPv4 */
int v = 1; /* 1 to enable, or 0 to disable */
setsockopt(mrouter_s4, IPPROTO_IP, MRT_INIT, (void *)&v, sizeof(v));

This is wrong obviously. we can't disable multicast forwarding by setting v to 0.
We should use MRT_DONE here.
Do you want to fix the manual or you want to change this logic? (I can help with manual if you want)

sys/netinet6/ip6_mroute.c

552

Same as above:

multicast(4):

/* IPv6 */
int v = 1; /* 1 to enable, or 0 to disable */
setsockopt(mrouter_s6, IPPROTO_IPV6, MRT6_INIT, (void *)&v, sizeof(v));

we should use MRT6_DONE instead of passing v = 0 to socket.

[glebius]

sys/netinet6/ip6_mroute.c

1911–1912

I agree, EBUSY is better.

[markj]

Handle review comments

[markj]

sys/netinet/ip_mroute.c

699

We should fix the manual. I was going to write an update to multicast.4 to describe the FIB-related changes (not included in the patch stack yet), but if you want to go ahead and fix the issue you found, that would be great.

744–749

I think this code is ok in fact: X_ip_mforward() is locked by the mrouter_lock(), so the ordering within this locked section does not matter. The v6 code is broken though.

sys/netinet6/ip6_mroute.c

577

That's not quite sufficient: the bug here is that we do not hold the mfc6 lock here, so there is no synchronization with the forwarding path.

600

Note that the multicast forwarding code is synchronized by a mutex, not by net_epoch. So it's not very important. But the locking here is insufficient, I'll update the patch shortly.

1911–1912

Sure.

[bms]

sys/netinet/ip_mroute.c

699

We should fix the manual. I was going to write an update to multicast.4 to describe the FIB-related changes (not included in the patch stack yet), but if you want to go ahead and fix the issue you found, that would be great.

I greatly appreciate the work you are doing to make multicast forwarding VRF capable in FreeBSD.

However, I really, really wish my use of the term FIB had not been misunderstood to refer to individual instances of routing tries all those years ago. I shouldn't really feel responsible, but I was a participant in the original thread.

This is a clash of terminology. What the rest of the world understands as IP VRF is a superset of what is called "FIB" in FreeBSD, where a "FIB index" is roughly analogous to a VRF instance. It is particularly problematic when discussing FreeBSD "FIB" with industry professionals who are unfamiliar with contemporary FreeBSD, or students.

[zlei]

Looks good!

[markj]

sys/netinet/ip_mroute.c

699

Unfortunately the "FIB" terminology is quite pervasive in the kernel implementation and tooling (setfib(1), route(8)'s -fib, etc.). On the other hand, I believe we currently don't have any central place which documents what a FIB index actually is. There are some networking man pages like netintro(4), route(4), which don't mention FIBs at all. We should update one of these, probably netintro(4), to 1) document the concept and 2) explain the terminology. It's unlikely I will get to this in the near future.

[bz]

"FIB" is a term way older than "VRF". VRF started as a "vendor term" for a specific implementation.
FreeBSD had "multi-FIB" support (that is what we call it) long before VRF found its way into a common standards language (which still is only an exception there).

And in a lot of ways "multi-FIB" is way more correct than VRF as all we do is having multiple "f"orwarding information bases. We don't provide the "r"outing part (which is the algorithms for best path selection etc.) on top and would be whatever runs in user space and programs the FIBs.

Lastly the term "VRF" is now also used by RFC 9381 - Verifiable Random Functions (VRFs). So it'll be fun to see how people will start talking about different things.