Improvements/build issues for FreeBSD

[RICCIARDI-Adrien]

Hi,

We may integrate some FreeBSD upstream patches into next radvd release, like this one : https://github.com/freebsd/freebsd-ports/blob/master/net/radvd/files/patch-device-bsd44.c (I did not try yet to build master radvd with this patch applied, I currently have some autoconf issues with FreeBSD 12).
There is also this patch : https://github.com/freebsd/freebsd-ports/blob/master/net/radvd/files/patch-interface.c.

What do you think about these integrations ?

I will keep this issue updated, I will need help for autotool.

[stappersg]

What do you think about these integrations ?

Both patches lack a why

I will need help for autotool.

Out of scope of this issue.

[RICCIARDI-Adrien]

OK, it will be up to FreeBSD guys to show up if they want to upstream their patches.

Are the "check" tests, built and executed with command make check, intended to work on BSD ?

[robbat2]

I emailed the author of those patches

[fichtner]

So I did respond in October 2021 a little late but perhaps we want to pick this back up now?

[stappersg]

So I did respond in October 2021

Where?

perhaps we want to pick this back up now?

There is meanwhile #217

[fichtner]

Can we not play this game after another 2 years of silence? I got an email, I responded and followed up here. The patch reasons are known and documented and the scope in FreeBSD has also shifted over time.

The main question is what is the radvd plan here?

[fichtner]

And #217 simply lacks context. 😉

[robbat2]

Hi @fichtner

I checked my email archives and I don't see any response from you in 2021.

Can you please submit cleaned versions of those patches as PRs, with commit message descriptions/signed-off-by DCO statements (It's unclear who wrote the patches).
https://github.com/freebsd/freebsd-ports/blob/master/net/radvd/files/patch-device-bsd44.c
https://github.com/freebsd/freebsd-ports/blob/master/net/radvd/files/patch-interface.c

I also see that the first patch does NOT populate the cleanup_allrouters_membership function.
What is FreeBSDs behavior in this case? On Linux, the cleanup is needed so that the host leaves the all-routers multicast group.

[fichtner]

Hi @robbat2,

This was the message:

From: Franco Fichtner <[email protected]>
Subject: Re: FreeBSD patches to radvd
Date: Thu, 28 Oct 2021 11:04:39 +0200
To: "Robin H. Johnson" <[email protected]>

Hi Robin,

Sorry for the late response.

> On 2. Apr 2021, at 7:38 AM, Robin H. Johnson <[email protected]> wrote:
> 
> I've recently taken over a leadership role maintaining radvd.

Nice to hear.  Thank you for your work!

> Could you have a look at this issue regarding FreeBSD support in radvd:
> https://github.com/radvd-project/radvd/issues/145
> 
> and then consider submitted the FreeBSD patches to radvd to us as
> upstream?

It looks like the FreeBSD ports tree is a good fit for two reasons:

(1) Implementation might differ between BSDs slightly.

(2) There is a bug in FreeBSD 12 upwards that depletes the amount of
multicast joins but it is almost impossible to trigger.  The patches
mitigate this issue by trying to hold on to the multicast membership
instead of doing the previous leave/join dance that worked fine on
FreeBSD 11.  Unfortunately nobody found the reason and FreeBSD committers
are unaware of this issue although it has real world impact on OPNsense
and pfSense -- long story short radvd is only really used there in the
FreeBSD scope.

> I've read them, but I don't understand why FreeBSD needs those patches.
> That information should probably be captured in comments and/or commit
> messages.

Maybe that's clearer now.  I would also point to this issue where we
tracked the work...

https://github.com/opnsense/core/issues/4338

If you have more questions let me know.


Cheers,
Franco

The bug we spoke of was found and fixed in FreeBSD this year. opnsense/src@93e9cefd053

There was also an attempt predating this but it only fixed the lesser half of the issue. opnsense/src@3eebc6234b0

setup_allrouters_membership() appears to be portable now. cleanup_allrouters_membership() is more or less automatic on process stop. I'm not sure how dynamic radvd can be operated, but with prefix deprecation needing a restart or adding new interfaces it's essentially always doing what it should.

patch-interface.c changes were only required to address opnsense/src@93e9cefd053 and superseded by opnsense/src@3eebc6234b0 so I think I can submit a patch to remove this one from FreeBSD.

I hope this clears up the questions.

Cheers,
Franco

[robbat2]

setup_allrouters_membership() appears to be portable now. cleanup_allrouters_membership() is more or less automatic on process stop.

To be clear, I can just move those two functions into device-common.c and all of the BSDs will work?

opnsense/src@93e9cefd053 explains a lot of the issues.
Do we need to conditionally check for the fix to support older BSD releases?

I realize in checking the code, that we've never actually made any statements about which BSD(s) and versions are supported or not.

[fichtner]

Hmm, give me a bit of time to see if check_ip6_iface_forwarding() is still as it should be and clean up the other things in the port. I can also fill cleanup_allrouters_membership() for completeness. Testing this is easy enough.

Do we need to conditionally check for the fix to support older BSD releases?

As far as FreeBSD 13.2 and 14.0 are concerned they work ok with the EADDRINUSE safeguard. Older FreeBSD releases are no longer supported anyway.

To be clear, I can just move those two functions into device-common.c and all of the BSDs will work?

Well, knowing they can handle additional patching via their ports trees I don't think it's a huge issue going forward with this and if so they might upstream a small ifdef for their operating system, e.g. in the check_ip6_iface_forwarding() case.

Naturally I don't want to speak for any of the BSDs with OPNsense being a side show, but I think the goal of integrating "BSD" is worth doing and the feedback will trickle in if the integration needs tweaking for a particular BSD.

Cheers,
Franco

[heliosfa]

I've done some testing on this with FreeBSD 14 and the only patch needed to make things work is the changes to setup_allrouters_membership() in device-bsd44.c

diff --git a/device-bsd44.c b/device-bsd44.c
index e788c20..e85f88c 100644
--- a/device-bsd44.c
+++ b/device-bsd44.c
@@ -127,15 +127,25 @@ ret:

 int setup_allrouters_membership(int sock, struct Interface *iface)
 {
-       return 0;
-
-       /* Quoting:
-       https://github.com/radvd-project/radvd/issues/145#issuecomment-810466476
+       struct ipv6_mreq mreq;
+
+       memset(&mreq, 0, sizeof(mreq));
+       mreq.ipv6mr_interface = iface->props.if_index;
+
+       /* all-routers multicast address */
+       if (inet_pton(AF_INET6, "ff02::2",
+                       &mreq.ipv6mr_multiaddr.s6_addr) != 1) {
+               flog(LOG_ERR, "inet_pton failed");
+               return (-1);
+       }

-       OK, it will be up to FreeBSD guys to show up
-       if they want to upstream their patches.
+       if (setsockopt(sock, IPPROTO_IPV6, IPV6_JOIN_GROUP,
+                       &mreq, sizeof(mreq)) < 0 && errno != EADDRINUSE) {
+               flog(LOG_ERR, "can't join ipv6-allrouters on %s", iface->props.name);
+               return (-1);
+       }

-       */
+       return 0;
 }

Without this, radvd doesn't respond to router solicitations from clients.

Following the discussion above, the other patches don't seem necessary with the changes that have happened in FreeBSD.