Re: [PATCH maint v2 3/4] batman-adv: mcast: fix duplicate mcast packets in BLA backbone from mesh

From: "Linus Lüssing" <linus.luessing@c0d3.blue>
To: The list for a Better Approach To Mobile Ad-hoc Networking
	<b.a.t.m.a.n@lists.open-mesh.org>
Subject: Re: [PATCH maint v2 3/4] batman-adv: mcast: fix duplicate mcast packets in BLA backbone from mesh
Date: Wed, 9 Sep 2020 16:53:57 +0200	[thread overview]
Message-ID: <20200909145357.GB2391@otheros> (raw)
In-Reply-To: <2973088.WcO1NEu1zB@prime>

On Wed, Sep 09, 2020 at 02:06:06PM +0200, Simon Wunderlich wrote:
> >         if (unlikely(atomic_read(&bat_priv->bla.num_requests)))
> >                 /* don't allow broadcasts while requests are in flight */
> > -               if (is_multicast_ether_addr(ethhdr->h_dest) && is_bcast)
> > +               if (is_multicast_ether_addr(ethhdr->h_dest) &&
> > +                   (!is_broadcast_ether_addr(ethhdr->h_dest) || is_bcast))
> >                         goto handled;
> 
> Isn't this exactly the same logic as it was before?
> 
> is_multicast == 0, is_bcast = 0 => 0
> is_multicast == 0, is_bcast = 1 => 0
> is_multicast == 1, is_bcast = 0 => 0
> is_multicast == 1, is_bcast = 1 => 1

The 3rd one should be different. Note that "is_bcast" is not the same as
is_broadcast_ether_addr(ethhdr->h_dest). The former refers to the
batman-adv packet header, while the latter refers to the
destination MAC of the inner ethernet header.

> On Friday, September 4, 2020 8:28:02 PM CEST Linus Lüssing wrote:
> > For DHCPv6: This is even trickier... DHCPv6 potentially uses
> > non-broadcast multicast addresses. However according to RFC8415, section
> > 7.1 it seems that currently multicast is only used from a DHCPv6 client
> > to a DHCPv6 server, but not the other way round.
> > 
> > Working through the gateway feature part in batadv_interface_tx() it can
> > be inferred that a DHCPv6 packet to a DHCP client would have been the only
> > option for a DHCPv6 multicast packet to be sent via unicast through the
> > gateway feature. Ergo, the newly introduced claim check won't wrongly
> > drop a DHCPv6 packet received via the gateway feature either.
> 
> I also don't get this part. Maybe it helps if you can explain the two 
> directions (client -> server, server -> client), and in which cases there can 
> be multicast, and then describe why your check is sufficient?

Hm, actually it's not just the description that is messed up, I
think. server->client is ok, but client->server isn't...

* DHCPv6 server -> client:
-> Easy, according to RFC8415, section 7.1 this would always be
   unicast. So neither the Gateway nor Multicast feature would
   touch it.

* DHCPv6 client -> server:
-> Actually both the gateway feature and multicast feature can use
   it. I misread the code...

I'm a bit uncertain how to solve the latter now... I see no way to
distinguish gw vs. mcast feature as is. We also have no flags
or reserved space in the batadv_unicast_packet available to make
them distinguishable.

So the only solution I could think of for now is
excluding DHCPv6 from multicast feature in TX of the originator...
(in batadv_mcast_forw_mode_check_ipv6(), adding excludes for
ff02::1:2 and ff05::1:3).

(Even though having the multicast feature handling it would
have been nice(r) as it'd work without needing a user to
set and maintain the gateway mode properly.)