[PATCH maint v2 0/4] batman-adv: mcast: TT/BLA fixes

[PATCH maint v2 0/4] batman-adv: mcast: TT/BLA fixes

[Linus Lüssing]

Hi,

These are four fixes for issues which occur when using the batman-adv
multicast-to-unicast feature.

The first one fixes an issue of an intermediate node snitching
multicast-to-unicast packets and either dropping it or rerouting it to
another node. Which causes lost packets on some and duplicate packets on
other nodes.

Patches 2 to 4 fix issues when using the multicast-to-unicast conversion
while BLA is enabled and some nodes are sharing the same LAN side. Here
it either causes "just" duplicates in the "good" scenario (Patch 4/4).
But can also cause multiple BLA backbones to send a frame from the mesh
into the same, shared LAN segment (Patch 3). Or in the worst case, even
reflect packets back to the host in the shared LAN, which completely
confuses switches/bridges and ICMPv6 Neighbor Discovery.

Changelog v2:
* Adding "Fixes:" lines

Regards, Linus

[PATCH maint v2 1/4] batman-adv: mcast/TT: fix wrongly dropped or rerouted packets

[Linus Lüssing]

The unicast packet rerouting code makes several assumptions. For
instance it assumes that there is always exactly one destination in the
TT. This breaks for multicast frames in a unicast packets in several ways:

For one thing if there is actually no TT entry and the destination node
was selected due to the multicast tvlv flags it announced. Then an
intermediate node will wrongly drop the packet.

For another thing if there is a TT entry but the TTVN of this entry is
newer than the originally addressed destination node: Then the
intermediate node will wrongly redirect the packet, leading to
duplicated multicast packets at a multicast listener and missing
packets at other multicast listeners or multicast routers.

Fixing this by not applying the unicast packet rerouting to batman-adv
unicast packets with a multicast payload. We are not able to detect a
roaming multicast listener at the moment and will just continue to send
the multicast frame to both the new and old destination for a while in
case of such a roaming multicast listener.

Fixes: cea194d90b11 ("batman-adv: improved client announcement mechanism")
Signed-off-by: Linus Lüssing <linus.luessing@c0d3.blue>
---
 net/batman-adv/routing.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/net/batman-adv/routing.c b/net/batman-adv/routing.c
index 27cdf5e4..9e5c71e4 100644
--- a/net/batman-adv/routing.c
+++ b/net/batman-adv/routing.c
@@ -826,6 +826,10 @@ static bool batadv_check_unicast_ttvn(struct batadv_priv *bat_priv,
 	vid = batadv_get_vid(skb, hdr_len);
 	ethhdr = (struct ethhdr *)(skb->data + hdr_len);
 
+	/* do not reroute multicast frames in a unicast header */
+	if (is_multicast_ether_addr(ethhdr->h_dest))
+		return true;
+
 	/* check if the destination client was served by this node and it is now
 	 * roaming. In this case, it means that the node has got a ROAM_ADV
 	 * message and that it knows the new destination in the mesh to re-route
-- 
2.28.0

Re: [PATCH maint v2 1/4] batman-adv: mcast/TT: fix wrongly dropped or rerouted packets

[Simon Wunderlich]

[-- Attachment #1: Type: text/plain, Size: 1371 bytes --]

On Friday, September 4, 2020 8:28:00 PM CEST Linus Lüssing wrote:
> The unicast packet rerouting code makes several assumptions. For
> instance it assumes that there is always exactly one destination in the
> TT. This breaks for multicast frames in a unicast packets in several ways:
> 
> For one thing if there is actually no TT entry and the destination node
> was selected due to the multicast tvlv flags it announced. Then an
> intermediate node will wrongly drop the packet.
> 
> For another thing if there is a TT entry but the TTVN of this entry is
> newer than the originally addressed destination node: Then the
> intermediate node will wrongly redirect the packet, leading to
> duplicated multicast packets at a multicast listener and missing
> packets at other multicast listeners or multicast routers.
> 
> Fixing this by not applying the unicast packet rerouting to batman-adv
> unicast packets with a multicast payload. We are not able to detect a
> roaming multicast listener at the moment and will just continue to send
> the multicast frame to both the new and old destination for a while in
> case of such a roaming multicast listener.
> 
> Fixes: cea194d90b11 ("batman-adv: improved client announcement mechanism")
> Signed-off-by: Linus Lüssing <linus.luessing@c0d3.blue>

Acked-by: Simon Wunderlich <sw@simonwunderlich.de>


[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH maint v2 2/4] batman-adv: mcast: fix duplicate mcast packets in BLA backbone from LAN

[Linus Lüssing]

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

Issue:
* BLA backbone nodes receive the frame multiple times on bat0

For multicast frames received via batman-adv broadcast packets the
originator of the broadcast packet is checked before decapsulating and
forwarding the frame to bat0 (batadv_bla_is_backbone_gw()->
batadv_recv_bcast_packet()). If it came from a node which shares the
same BLA backbone with us then it is not forwarded to bat0 to avoid a
loop.

When sending a multicast frame in a non-4-address batman-adv unicast
packet we are currently missing this check - and cannot do so because
the batman-adv unicast packet has no originator address field.

However, we can simply fix this on the sender side by only sending the
multicast frame via unicasts to interested nodes which do not share the
same BLA backbone with us. This also nicely avoids some unnecessary
transmissions on mesh side.

Note that no infinite loop was observed, probably because of dropping
via batadv_interface_tx()->batadv_bla_tx(). However the duplicates still
utterly confuse switches/bridges, ICMPv6 duplicate address detection and
neighbor discovery and therefore leads to long delays before being able
to establish TCP connections, for instance. And it also leads to the Linux
bridge printing messages like:
"br-lan: received packet on eth1 with own address as source address ..."

Fixes: 405cc1e5a81e ("batman-adv: Modified forwarding behaviour for multicast packets")
Signed-off-by: Linus Lüssing <linus.luessing@c0d3.blue>
---
 net/batman-adv/send.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/net/batman-adv/send.c b/net/batman-adv/send.c
index d267b948..67f493c0 100644
--- a/net/batman-adv/send.c
+++ b/net/batman-adv/send.c
@@ -29,6 +29,7 @@
 #include <linux/stddef.h>
 #include <linux/workqueue.h>
 
+#include "bridge_loop_avoidance.h"
 #include "distributed-arp-table.h"
 #include "fragmentation.h"
 #include "gateway_client.h"
@@ -343,6 +344,18 @@ int batadv_send_skb_unicast(struct batadv_priv *bat_priv,
 	if (!orig_node)
 		goto out;
 
+	/* Avoid sending multicast-in-unicast packets to other BLA
+	 * gateways - they already got the frame from the LAN side
+	 * we share with them.
+	 * TODO: Refactor multicast code to anticipate this, to
+	 * avoid this check here.
+	 */
+	if (is_multicast_ether_addr(eth_hdr(skb)->h_dest) &&
+	    batadv_bla_is_backbone_gw_orig(bat_priv, orig_node->orig, vid)) {
+		dev_kfree_skb(skb);
+		return NET_XMIT_SUCCESS;
+	}
+
 	switch (packet_type) {
 	case BATADV_UNICAST:
 		if (!batadv_send_skb_prepare_unicast(skb, orig_node))
-- 
2.28.0

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

[Simon Wunderlich]

[-- Attachment #1: Type: text/plain, Size: 3822 bytes --]

On Friday, September 4, 2020 8:28:01 PM CEST Linus Lüssing wrote:
> Scenario:
> * Multicast frame send from a BLA backbone (multiple nodes with
>   their bat0 bridged together, with BLA enabled)
> 
> Issue:
> * BLA backbone nodes receive the frame multiple times on bat0
> 
> For multicast frames received via batman-adv broadcast packets the
> originator of the broadcast packet is checked before decapsulating and
> forwarding the frame to bat0 (batadv_bla_is_backbone_gw()->
> batadv_recv_bcast_packet()). If it came from a node which shares the
> same BLA backbone with us then it is not forwarded to bat0 to avoid a
> loop.
> 
> When sending a multicast frame in a non-4-address batman-adv unicast
> packet we are currently missing this check - and cannot do so because
> the batman-adv unicast packet has no originator address field.
> 
> However, we can simply fix this on the sender side by only sending the
> multicast frame via unicasts to interested nodes which do not share the
> same BLA backbone with us. This also nicely avoids some unnecessary
> transmissions on mesh side.
> 
> Note that no infinite loop was observed, probably because of dropping
> via batadv_interface_tx()->batadv_bla_tx(). However the duplicates still
> utterly confuse switches/bridges, ICMPv6 duplicate address detection and
> neighbor discovery and therefore leads to long delays before being able
> to establish TCP connections, for instance. And it also leads to the Linux
> bridge printing messages like:
> "br-lan: received packet on eth1 with own address as source address ..."
> 
> Fixes: 405cc1e5a81e ("batman-adv: Modified forwarding behaviour for
> multicast packets") Signed-off-by: Linus Lüssing <linus.luessing@c0d3.blue>
> ---
>  net/batman-adv/send.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/net/batman-adv/send.c b/net/batman-adv/send.c
> index d267b948..67f493c0 100644
> --- a/net/batman-adv/send.c
> +++ b/net/batman-adv/send.c
> @@ -29,6 +29,7 @@
>  #include <linux/stddef.h>
>  #include <linux/workqueue.h>
> 
> +#include "bridge_loop_avoidance.h"
>  #include "distributed-arp-table.h"
>  #include "fragmentation.h"
>  #include "gateway_client.h"
> @@ -343,6 +344,18 @@ int batadv_send_skb_unicast(struct batadv_priv
> *bat_priv, if (!orig_node)
>  		goto out;
> 
> +	/* Avoid sending multicast-in-unicast packets to other BLA
> +	 * gateways - they already got the frame from the LAN side
> +	 * we share with them.
> +	 * TODO: Refactor multicast code to anticipate this, to
> +	 * avoid this check here.
> +	 */
> +	if (is_multicast_ether_addr(eth_hdr(skb)->h_dest) &&
> +	    batadv_bla_is_backbone_gw_orig(bat_priv, orig_node->orig, vid)) {
> +		dev_kfree_skb(skb);
> +		return NET_XMIT_SUCCESS;
> +	}
> +

Would it make sense to perform this check in the BATADV_UNICAST case, without 
checking the ethernet destination for multicast?

A backbone gateway should never send a unicast frame to another backbone 
gateway, regardless of multicast or not - those things should go over the 
backbone either way.

For 4addr unicasts, I see two cases: TT Unicasts could be dropped in the same 
way, as TT is ignored between backbone gateways. For DAT, there is currently 
no specific BLA handling for the unicast handling as far as I see, there are 
only some checks to make sure that ARP replies coming out of the correct 
backbone gateway. Since DAT is "best effort" and requests may get dropped, it's 
probably safe to drop this too.

That would allow us to use the same check as you have here, but dropping the 
check multicast ethernet address check.

Cheers,
       Simon

>  	switch (packet_type) {
>  	case BATADV_UNICAST:
>  		if (!batadv_send_skb_prepare_unicast(skb, orig_node))


[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Linus Lüssing]

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

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;
 
 	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
-- 
2.28.0

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

[Sven Eckelmann]

[-- Attachment #1: Type: text/plain, Size: 992 bytes --]

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Simon Wunderlich]

[-- Attachment #1: Type: text/plain, Size: 2722 bytes --]

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.

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Linus Lüssing]

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

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

[Linus Lüssing]

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.

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

[Linus Lüssing]

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.

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

[Simon Wunderlich]

[-- Attachment #1: Type: text/plain, Size: 735 bytes --]

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Linus Lüssing]

Scenario:
* Multicast frame send from BLA backbone gateways (multiple nodes
  with their bat0 bridged together, with BLA enabled) sharing the same
  LAN to nodes in the mesh

Issue:
* Nodes receive the frame multiple times on bat0 from the mesh,
  once from each foreign BLA backbone gateway which shares the same LAN
  with another

For multicast frames via batman-adv broadcast packets coming from the
same BLA backbone but from different backbone gateways duplicates are
currently detected via a CRC history of previously received packets.

However this CRC so far was not performed for multicast frames received
via batman-adv unicast packets. Fixing this by appyling the same check
for such packets, too.

Room for improvements in the future: Ideally we would introduce the
possibility to not only claim a client, but a complete originator, too.
This would allow us to only send a multicast-in-unicast packet from a BLA
backbone gateway claiming the node and by that avoid potential redundant
transmissions in the first place.

Fixes: e5cf86d30a9b ("batman-adv: add broadcast duplicate check")
Signed-off-by: Linus Lüssing <linus.luessing@c0d3.blue>
---
 net/batman-adv/bridge_loop_avoidance.c | 86 +++++++++++++++++++++-----
 1 file changed, 70 insertions(+), 16 deletions(-)

diff --git a/net/batman-adv/bridge_loop_avoidance.c b/net/batman-adv/bridge_loop_avoidance.c
index 9603a6d0..c509a83d 100644
--- a/net/batman-adv/bridge_loop_avoidance.c
+++ b/net/batman-adv/bridge_loop_avoidance.c
@@ -1580,14 +1580,16 @@ int batadv_bla_init(struct batadv_priv *bat_priv)
 }
 
 /**
- * batadv_bla_check_bcast_duplist() - Check if a frame is in the broadcast dup.
+ * batadv_bla_check_duplist() - Check if a frame is in the broadcast dup.
  * @bat_priv: the bat priv with all the soft interface information
- * @skb: contains the bcast_packet to be checked
- *
- * check if it is on our broadcast list. Another gateway might
- * have sent the same packet because it is connected to the same backbone,
- * so we have to remove this duplicate.
+ * @skb: contains the multicast packet to be checked
+ * @payload_ptr: pointer to position inside the head buffer of the skb
+ *  marking the start of the data to be CRC'ed
  *
+ * Check if it is on our broadcast list. Another gateway might have sent the
+ * same packet because it is connected to the same backbone, so we have to
+ * remove this duplicate.
+
  * This is performed by checking the CRC, which will tell us
  * with a good chance that it is the same packet. If it is furthermore
  * sent by another host, drop it. We allow equal packets from
@@ -1595,19 +1597,17 @@ int batadv_bla_init(struct batadv_priv *bat_priv)
  *
  * Return: true if a packet is in the duplicate list, false otherwise.
  */
-bool batadv_bla_check_bcast_duplist(struct batadv_priv *bat_priv,
-				    struct sk_buff *skb)
+static bool batadv_bla_check_duplist(struct batadv_priv *bat_priv,
+				     struct sk_buff *skb, u8 *payload_ptr,
+				     const u8 *orig)
 {
+	struct batadv_bcast_duplist_entry *entry;
+	bool ret = false;
 	int i, curr;
 	__be32 crc;
-	struct batadv_bcast_packet *bcast_packet;
-	struct batadv_bcast_duplist_entry *entry;
-	bool ret = false;
-
-	bcast_packet = (struct batadv_bcast_packet *)skb->data;
 
 	/* calculate the crc ... */
-	crc = batadv_skb_crc32(skb, (u8 *)(bcast_packet + 1));
+	crc = batadv_skb_crc32(skb, payload_ptr);
 
 	spin_lock_bh(&bat_priv->bla.bcast_duplist_lock);
 
@@ -1626,7 +1626,8 @@ bool batadv_bla_check_bcast_duplist(struct batadv_priv *bat_priv,
 		if (entry->crc != crc)
 			continue;
 
-		if (batadv_compare_eth(entry->orig, bcast_packet->orig))
+		if (!is_zero_ether_addr(entry->orig) &&
+		    batadv_compare_eth(entry->orig, orig))
 			continue;
 
 		/* this entry seems to match: same crc, not too old,
@@ -1643,7 +1644,7 @@ bool batadv_bla_check_bcast_duplist(struct batadv_priv *bat_priv,
 	entry = &bat_priv->bla.bcast_duplist[curr];
 	entry->crc = crc;
 	entry->entrytime = jiffies;
-	ether_addr_copy(entry->orig, bcast_packet->orig);
+	ether_addr_copy(entry->orig, orig);
 	bat_priv->bla.bcast_duplist_curr = curr;
 
 out:
@@ -1652,6 +1653,52 @@ bool batadv_bla_check_bcast_duplist(struct batadv_priv *bat_priv,
 	return ret;
 }
 
+/**
+ * batadv_bla_check_ucast_duplist() - Check if a frame is in the broadcast dup.
+ * @bat_priv: the bat priv with all the soft interface information
+ * @skb: contains the multicast packet to be checked, decapsulated from a
+ *  unicast_packet
+ *
+ * Check if it is on our broadcast list. Another gateway might have sent the
+ * same packet because it is connected to the same backbone, so we have to
+ * remove this duplicate.
+ *
+ * Return: true if a packet is in the duplicate list, false otherwise.
+ */
+static bool batadv_bla_check_ucast_duplist(struct batadv_priv *bat_priv,
+					   struct sk_buff *skb)
+{
+	u8 orig[ETH_ALEN];
+
+	eth_zero_addr(orig);
+
+	return batadv_bla_check_duplist(bat_priv, skb, (u8 *)skb->data, orig);
+}
+
+/**
+ * batadv_bla_check_bcast_duplist() - Check if a frame is in the broadcast dup.
+ * @bat_priv: the bat priv with all the soft interface information
+ * @skb: contains the bcast_packet to be checked
+ *
+ * Check if it is on our broadcast list. Another gateway might have sent the
+ * same packet because it is connected to the same backbone, so we have to
+ * remove this duplicate.
+ *
+ * Return: true if a packet is in the duplicate list, false otherwise.
+ */
+bool batadv_bla_check_bcast_duplist(struct batadv_priv *bat_priv,
+				    struct sk_buff *skb)
+{
+	struct batadv_bcast_packet *bcast_packet;
+	u8 *payload_ptr;
+
+	bcast_packet = (struct batadv_bcast_packet *)skb->data;
+	payload_ptr = (u8 *)(bcast_packet + 1);
+
+	return batadv_bla_check_duplist(bat_priv, skb, payload_ptr,
+					bcast_packet->orig);
+}
+
 /**
  * batadv_bla_is_backbone_gw_orig() - Check if the originator is a gateway for
  *  the VLAN identified by vid.
@@ -1852,6 +1899,13 @@ bool batadv_bla_rx(struct batadv_priv *bat_priv, struct sk_buff *skb,
 		    (!is_broadcast_ether_addr(ethhdr->h_dest) || is_bcast))
 			goto handled;
 
+	/* potential duplicates from foreign BLA backbone gateways via
+	 * multicast-in-unicast packets
+	 */
+	if (is_multicast_ether_addr(ethhdr->h_dest) && !is_bcast &&
+	    batadv_bla_check_ucast_duplist(bat_priv, skb))
+		goto handled;
+
 	ether_addr_copy(search_claim.addr, ethhdr->h_source);
 	search_claim.vid = vid;
 	claim = batadv_claim_hash_find(bat_priv, &search_claim);
-- 
2.28.0

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

[Simon Wunderlich]

[-- Attachment #1: Type: text/plain, Size: 994 bytes --]

On Friday, September 4, 2020 8:28:03 PM CEST Linus Lüssing wrote:
> @@ -1626,7 +1626,8 @@ bool batadv_bla_check_bcast_duplist(struct batadv_priv
> *bat_priv, if (entry->crc != crc)
>                         continue;
> 
> -               if (batadv_compare_eth(entry->orig, bcast_packet->orig))
> +               if (!is_zero_ether_addr(entry->orig) &&
> +                   batadv_compare_eth(entry->orig, orig))
>                         continue;
> 
>                 /* this entry seems to match: same crc, not too old,

Shouldn't this check also be skipped if the orig parameter is a zero mac 
address? i.e.:

if (!is_zero_ether_addr(orig)) {
	if (!is_zero_ether_addr(entry->orig) && batadv_compare_eth(entry->orig, 
orig))
		continue;
}

Whether orig is zero can probably be checked once before the loop and the 
result cached in a bool variable.

A little comment may also be nice to explain this part a bit better.

The rest looks good.

Cheers,
      Simon

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Linus Lüssing]

On Wed, Sep 09, 2020 at 02:15:51PM +0200, Simon Wunderlich wrote:
> On Friday, September 4, 2020 8:28:03 PM CEST Linus Lüssing wrote:
> > @@ -1626,7 +1626,8 @@ bool batadv_bla_check_bcast_duplist(struct batadv_priv
> > *bat_priv, if (entry->crc != crc)
> >                         continue;
> > 
> > -               if (batadv_compare_eth(entry->orig, bcast_packet->orig))
> > +               if (!is_zero_ether_addr(entry->orig) &&
> > +                   batadv_compare_eth(entry->orig, orig))
> >                         continue;
> > 
> >                 /* this entry seems to match: same crc, not too old,
> 
> Shouldn't this check also be skipped if the orig parameter is a zero mac 
> address? i.e.:
> 
> if (!is_zero_ether_addr(orig)) {
> 	if (!is_zero_ether_addr(entry->orig) && batadv_compare_eth(entry->orig, 
> orig))
> 		continue;
> }

Would be redundant. If entry->orig is non-zero and
the compare_eth() says they are equal, then orig must also be
non-zero.

I initially wanted to leave the code as unchanged as possible for
net / maint. Should I do the restructuring to enhance readability, with
the bool in this patch or in additional patch for net-next?

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

[Simon Wunderlich]

[-- Attachment #1: Type: text/plain, Size: 1808 bytes --]

On Wednesday, September 9, 2020 5:27:56 PM CEST Linus Lüssing wrote:
> On Wed, Sep 09, 2020 at 02:15:51PM +0200, Simon Wunderlich wrote:
> > On Friday, September 4, 2020 8:28:03 PM CEST Linus Lüssing wrote:
> > > @@ -1626,7 +1626,8 @@ bool batadv_bla_check_bcast_duplist(struct
> > > batadv_priv *bat_priv, if (entry->crc != crc)
> > > 
> > >                         continue;
> > > 
> > > -               if (batadv_compare_eth(entry->orig, bcast_packet->orig))
> > > +               if (!is_zero_ether_addr(entry->orig) &&
> > > +                   batadv_compare_eth(entry->orig, orig))
> > > 
> > >                         continue;
> > >                 
> > >                 /* this entry seems to match: same crc, not too old,
> > 
> > Shouldn't this check also be skipped if the orig parameter is a zero mac
> > address? i.e.:
> > 
> > if (!is_zero_ether_addr(orig)) {
> > 
> > 	if (!is_zero_ether_addr(entry->orig) && batadv_compare_eth(entry->orig,
> > 
> > orig))
> > 
> > 		continue;
> > 
> > }
> 
> Would be redundant. If entry->orig is non-zero and
> the compare_eth() says they are equal, then orig must also be
> non-zero.

OK good point, that's not really obvious (at least to me).

> 
> I initially wanted to leave the code as unchanged as possible for
> net / maint. Should I do the restructuring to enhance readability, with
> the bool in this patch or in additional patch for net-next?

Personally, I would prefer having a bit more readability or verbose comments 
in front of those kind of logic if statements. Or avoid those logic 
connections and have multiple "ifs" in a row where possible to enhance 
readbility.

This patch is pretty heavy already as is, adding a bool doesn't make a big 
difference IMHO.

Cheers,
       Simon


[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]