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

  Scenario:
* Multicast frame send from mesh to a BLA backbone (multiple nodes
  with their bat0 bridged together, with BLA enabled)

Issue:
* BLA backbone nodes receive the frame multiple times on bat0,
  once from mesh->bat0 and once from each backbone_gw from LAN

For unicast, a node will send only to the best backbone gateway
according to the TQ. However for multicast we currently cannot determine
if multiple destination nodes share the same backbone if they don't share
the same backbone with us. So we need to keep sending the unicasts to
all backbone gateways and let the backbone gateways decide which one
will forward the frame. We can use the CLAIM mechanism to make this
decision.

One catch: The batman-adv gateway feature for DHCP packets potentially
sends multicast packets in the same batman-adv unicast header as the
multicast optimizations code. And we are not allowed to drop those even
if we did not claim the source address of the sender, as for such
packets there is only this one multicast-in-unicast packet.

How can we distinguish the two cases?

For DHCPv4: Here the broadcast MAC address is used and the multicast
optimizations will never send a broadcast frame via batman-adv unicast
packets (see the !is_broadcast_ether_addr() check in after the goto-send
in batadv_interface_tx().

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.

Fixes: e32470167379 ("batman-adv: check incoming packet type for bla")
Signed-off-by: Linus Lüssing <linus.luessing@c0d3.blue>
---
 net/batman-adv/bridge_loop_avoidance.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
  

On Friday, 4 September 2020 20:28:02 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 don't really get this part. Shouldn't it be the other way around in the 
code? But I haven't the time at the moment to check  the code - maybe we can 
discuss this on Monday.

And I would also like to ask Simon to check the BLA patches before I merge 
them.

Kind regards,
	Sven
  

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?

> 
> Fixes: e32470167379 ("batman-adv: check incoming packet type for bla")
> Signed-off-by: Linus Lüssing <linus.luessing@c0d3.blue>
> ---
>  net/batman-adv/bridge_loop_avoidance.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/net/batman-adv/bridge_loop_avoidance.c
> b/net/batman-adv/bridge_loop_avoidance.c index d8c5d317..9603a6d0 100644
> --- a/net/batman-adv/bridge_loop_avoidance.c
> +++ b/net/batman-adv/bridge_loop_avoidance.c
> @@ -1848,7 +1848,8 @@ bool batadv_bla_rx(struct batadv_priv *bat_priv,
> struct sk_buff *skb,
> 
>         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


> 
>         ether_addr_copy(search_claim.addr, ethhdr->h_source);
> @@ -1885,7 +1886,8 @@ bool batadv_bla_rx(struct batadv_priv *bat_priv,
> struct sk_buff *skb, }
> 
>         /* if it is a broadcast ... */
> -       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)) {
>                 /* ... drop it. the responsible gateway is in charge.
>                  *
>                  * We need to check is_bcast because with the gateway

Same here.
  

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.)
  

On Wed, Sep 09, 2020 at 04:53:57PM +0200, Linus Lüssing wrote:
> 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).

And there is also no way for a random node in the mesh to figure out
if two or more other nodes share the same LAN via BLA, right?

That would have been the other option, to avoid sending a
multicast-in-unicast packet via the multicast feature to multiple
such nodes in the first place.
  

On Wed, Sep 09, 2020 at 04:53:57PM +0200, Linus Lüssing wrote:
> 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).

Ah, wait, we could distinguish them. Just noticed that the gateway
feature uses a unicast 4 address header, while the multicast
feature uses a simple, 3 address unicast header.

That should work. But might look a bit hacky. And would disallow
using a 4 address header from the multicast feature in the future.
  

On Wednesday, September 9, 2020 4:53:57 PM CEST Linus Lüssing wrote:
> 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.

Oh right, one is is_multicast() and the other one is_broadcast().

This part definitely needs either some comment or, even better, split into 
multiple conditions checks or a helper function which makes it clear.

I've stared on this for a couple of minutes, but we should be able to review 
that kind of code faster. Maybe it's just me, but I think this can be 
improved. :P

Cheers,
       Simon