[PATCH] babel: Drop check for IF_MULTICAST interface flag

[PATCH] babel: Drop check for IF_MULTICAST interface flag

[Toke Høiland-Jørgensen]

The babel protocol code was checking interfaces for the IF_MULTICAST flag
and refusing to run if this isn't present. However, there are cases where
this flag doesn't correspond to the actual capability of sending multicast
packets. For instance, Wireguard interfaces on FreeBSD doesn't set the
required flags, but Babel will run just fine over such an interface given
the right configuration.

Since we're also checking for the presence of a link-local addresses right
below the flag check, we don't really need it. So let's just drop the check
and trust that users will only configure Babel on interfaces that can
handle the traffic.

Reported-by: Stefan Haller <stefan.haller@stha.de>
Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>
---
 proto/babel/babel.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/proto/babel/babel.c b/proto/babel/babel.c
index 4b6b9d7f9f6f..297b86b06a46 100644
--- a/proto/babel/babel.c
+++ b/proto/babel/babel.c
@@ -1658,10 +1658,6 @@ babel_if_notify(struct proto *P, unsigned flags, struct iface *iface)
     if (!(iface->flags & IF_UP))
       return;
 
-    /* We only speak multicast */
-    if (!(iface->flags & IF_MULTICAST))
-      return;
-
     /* Ignore ifaces without link-local address */
     if (!iface->llv6)
       return;
@@ -1736,10 +1732,6 @@ babel_reconfigure_ifaces(struct babel_proto *p, struct babel_config *cf)
     if (!(iface->flags & IF_UP))
       continue;
 
-    /* Ignore non-multicast ifaces */
-    if (!(iface->flags & IF_MULTICAST))
-      continue;
-
     /* Ignore ifaces without link-local address */
     if (!iface->llv6)
       continue;
-- 
2.31.1

Re: [PATCH] babel: Drop check for IF_MULTICAST interface flag

[Ondrej Zajicek]

On Thu, Apr 15, 2021 at 03:44:50PM +0200, Toke Høiland-Jørgensen wrote:
> The babel protocol code was checking interfaces for the IF_MULTICAST flag
> and refusing to run if this isn't present. However, there are cases where
> this flag doesn't correspond to the actual capability of sending multicast
> packets. For instance, Wireguard interfaces on FreeBSD doesn't set the
> required flags, but Babel will run just fine over such an interface given
> the right configuration.

Hi

Is there a reason why to disregard the IF_MULTICAST flag? This seems to me
more like a bug in FreeBSD Wireguard implementation that should be fixed
there. Is this flag properly checked on Linux, or is there some reason why
the flag is missing?

Routing protocols in BIRD generally follow this flag (and perhaps use it
to switch to unicast-only mode), so i do not see why Babel should behave
differently.

-- 
Elen sila lumenn' omentielvo

Ondrej 'Santiago' Zajicek (email: santiago@crfreenet.org)
OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net)
"To err is human -- to blame it on a computer is even more so."

Re: [PATCH] babel: Drop check for IF_MULTICAST interface flag

[Toke Høiland-Jørgensen]

Ondrej Zajicek <santiago@crfreenet.org> writes:

> On Thu, Apr 15, 2021 at 03:44:50PM +0200, Toke Høiland-Jørgensen wrote:
>> The babel protocol code was checking interfaces for the IF_MULTICAST flag
>> and refusing to run if this isn't present. However, there are cases where
>> this flag doesn't correspond to the actual capability of sending multicast
>> packets. For instance, Wireguard interfaces on FreeBSD doesn't set the
>> required flags, but Babel will run just fine over such an interface given
>> the right configuration.
>
> Hi
>
> Is there a reason why to disregard the IF_MULTICAST flag? This seems to me
> more like a bug in FreeBSD Wireguard implementation that should be fixed
> there. Is this flag properly checked on Linux, or is there some reason why
> the flag is missing?

We did fix Wireguard - see:
https://git.zx2c4.com/wireguard-freebsd/patch/?id=a7a84a17faf784857f076e37aa4818f6b6c12a95

However, that didn't help, Babel still refused to use the interface.
Looking at krt-sock.c, the IF_MULTICAST flag is only set on
IFF_POINTOPOINT or IFF_BROADCAST on bsd. The Linux code (in netlink.c)
has a further:

      if (fl & IFF_MULTICAST)
	f.flags |= IF_MULTICAST;

beneath the other flag checks, so maybe that's really what's missing on
the BSD side?

> Routing protocols in BIRD generally follow this flag (and perhaps use
> it to switch to unicast-only mode), so i do not see why Babel should
> behave differently.

Yeah, I do believe I originally copied that check from one of the other
protocols. I can see how it makes sense to check the flag and change
operation mode based on it, but given that Babel doesn't do that it just
seems kinda redundant? If the interface *actually* is unable to send
multicast packets, the subsequent socket operation is going to fail, and
at least that produces an error message instead of just silently
ignoring the interface like that flag check does :)

-Toke

Re: [PATCH] babel: Drop check for IF_MULTICAST interface flag

[Ondrej Zajicek]

On Mon, Apr 19, 2021 at 03:55:18PM +0200, Toke Høiland-Jørgensen wrote:
> Ondrej Zajicek <santiago@crfreenet.org> writes:
> 
> > Is there a reason why to disregard the IF_MULTICAST flag? This seems to me
> > more like a bug in FreeBSD Wireguard implementation that should be fixed
> > there. Is this flag properly checked on Linux, or is there some reason why
> > the flag is missing?
> 
> We did fix Wireguard - see:
> https://git.zx2c4.com/wireguard-freebsd/patch/?id=a7a84a17faf784857f076e37aa4818f6b6c12a95
> 
> However, that didn't help, Babel still refused to use the interface.
> Looking at krt-sock.c, the IF_MULTICAST flag is only set on
> IFF_POINTOPOINT or IFF_BROADCAST on bsd. The Linux code (in netlink.c)
> has a further:
> 
>       if (fl & IFF_MULTICAST)
> 	f.flags |= IF_MULTICAST;
> 
> beneath the other flag checks, so maybe that's really what's missing on
> the BSD side?

Yes, it is likely that it is an issue in sysdep/bsd code.


> > Routing protocols in BIRD generally follow this flag (and perhaps use
> > it to switch to unicast-only mode), so i do not see why Babel should
> > behave differently.
> 
> Yeah, I do believe I originally copied that check from one of the other
> protocols. I can see how it makes sense to check the flag and change
> operation mode based on it, but given that Babel doesn't do that it just
> seems kinda redundant? If the interface *actually* is unable to send
> multicast packets, the subsequent socket operation is going to fail, and
> at least that produces an error message instead of just silently
> ignoring the interface like that flag check does :)

Well, i am OK with generating a warning in cases of non-matching interface
type, instead of ignoring it silently. (In contrast to iface down or missing
lladdr, which should be silent, as it may correct later.)

-- 
Elen sila lumenn' omentielvo

Ondrej 'Santiago' Zajicek (email: santiago@crfreenet.org)
OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net)
"To err is human -- to blame it on a computer is even more so."

Re: [PATCH] babel: Drop check for IF_MULTICAST interface flag

[Toke Høiland-Jørgensen]

Ondrej Zajicek <santiago@crfreenet.org> writes:

> On Mon, Apr 19, 2021 at 03:55:18PM +0200, Toke Høiland-Jørgensen wrote:
>> Ondrej Zajicek <santiago@crfreenet.org> writes:
>> 
>> > Is there a reason why to disregard the IF_MULTICAST flag? This seems to me
>> > more like a bug in FreeBSD Wireguard implementation that should be fixed
>> > there. Is this flag properly checked on Linux, or is there some reason why
>> > the flag is missing?
>> 
>> We did fix Wireguard - see:
>> https://git.zx2c4.com/wireguard-freebsd/patch/?id=a7a84a17faf784857f076e37aa4818f6b6c12a95
>> 
>> However, that didn't help, Babel still refused to use the interface.
>> Looking at krt-sock.c, the IF_MULTICAST flag is only set on
>> IFF_POINTOPOINT or IFF_BROADCAST on bsd. The Linux code (in netlink.c)
>> has a further:
>> 
>>       if (fl & IFF_MULTICAST)
>> 	f.flags |= IF_MULTICAST;
>> 
>> beneath the other flag checks, so maybe that's really what's missing on
>> the BSD side?
>
> Yes, it is likely that it is an issue in sysdep/bsd code.

Alright, I'll send a patch for that then :)

>> > Routing protocols in BIRD generally follow this flag (and perhaps use
>> > it to switch to unicast-only mode), so i do not see why Babel should
>> > behave differently.
>> 
>> Yeah, I do believe I originally copied that check from one of the other
>> protocols. I can see how it makes sense to check the flag and change
>> operation mode based on it, but given that Babel doesn't do that it just
>> seems kinda redundant? If the interface *actually* is unable to send
>> multicast packets, the subsequent socket operation is going to fail, and
>> at least that produces an error message instead of just silently
>> ignoring the interface like that flag check does :)
>
> Well, i am OK with generating a warning in cases of non-matching interface
> type, instead of ignoring it silently. (In contrast to iface down or missing
> lladdr, which should be silent, as it may correct later.)

OK, fine with me; I'll send an updated patch that adds a warning instead
of dropping the check...

-Toke