[PATCH maint v5 0/3] batman-adv: mcast: BLA fixes

[PATCH maint v5 0/3] batman-adv: mcast: BLA fixes

[Linus Lüssing]

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

They 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 3/3).
But can also cause multiple BLA backbones to send a frame from the mesh
into the same, shared LAN segment (Patch 2/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 (Patch 1/3).

Regards, Linus

---

Changelog v5:
* Patch 3/3:
  - added missing "*" in kernel doc for batadv_bla_check_duplist()

Changelog v4:
* Patch 1/3:
  - fixed build with CONFIG_BATMAN_ADV_MCAST=n
    -> dummy for batadv_mcast_forw_send_orig()
* Patch 2/3:
  - fixed parameters for batadv_bla_rx() with CONFIG_BATMAN_ADV_BLA=n
    (bool is_bcast -> int packet_type)

Changelog v3:
* removed 1/4, as it was already applied
* renamed title (excluded TT, as already applied)

* Patch 1/3:
  - moved bla-backbone check into multicast code:
    -> to stay on the safe side for net, to touch less other code
       paths - the issue was observed specifically for multicast
       and the fix was tested for that
  - added a note regarding implications for mcast-fanout
* Patch 2/3:
  - previous approach was broken, it would break the DHCPv6 gateway
    code; instead distinguish by batadv 3 vs. 4 addr unicast header
  - added some more code comments
  - updated commit message with new approach
* Patch 3/3:
  - made code a bit more verbose and explicit
  - added some more code comments
  - added missing kernel doc for "orig" parameter to
    new batadv_bla_check_duplist()

Changelog v2:
* Adding "Fixes:" lines

[PATCH maint v5 1/3] 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/multicast.c      | 46 ++++++++++++++++++++++++++-------
 net/batman-adv/multicast.h      | 15 +++++++++++
 net/batman-adv/soft-interface.c |  5 ++--
 3 files changed, 53 insertions(+), 13 deletions(-)

diff --git a/net/batman-adv/multicast.c b/net/batman-adv/multicast.c
index bdc4a1fb..ca24a2e5 100644
--- a/net/batman-adv/multicast.c
+++ b/net/batman-adv/multicast.c
@@ -51,6 +51,7 @@
 #include <uapi/linux/batadv_packet.h>
 #include <uapi/linux/batman_adv.h>
 
+#include "bridge_loop_avoidance.h"
 #include "hard-interface.h"
 #include "hash.h"
 #include "log.h"
@@ -1434,6 +1435,35 @@ batadv_mcast_forw_mode(struct batadv_priv *bat_priv, struct sk_buff *skb,
 	return BATADV_FORW_ALL;
 }
 
+/**
+ * batadv_mcast_forw_send_orig() - send a multicast packet to an originator
+ * @bat_priv: the bat priv with all the soft interface information
+ * @skb: the multicast packet to send
+ * @vid: the vlan identifier
+ * @orig_node: the originator to send the packet to
+ *
+ * Return: NET_XMIT_DROP in case of error or NET_XMIT_SUCCESS otherwise.
+ */
+int batadv_mcast_forw_send_orig(struct batadv_priv *bat_priv,
+				struct sk_buff *skb,
+				unsigned short vid,
+				struct batadv_orig_node *orig_node)
+{
+	/* 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 to take BLA into account earlier, to avoid
+	 * reducing the mcast_fanout count.
+	 */
+	if (batadv_bla_is_backbone_gw_orig(bat_priv, orig_node->orig, vid)) {
+		dev_kfree_skb(skb);
+		return NET_XMIT_SUCCESS;
+	}
+
+	return batadv_send_skb_unicast(bat_priv, skb, BATADV_UNICAST, 0,
+				       orig_node, vid);
+}
+
 /**
  * batadv_mcast_forw_tt() - forwards a packet to multicast listeners
  * @bat_priv: the bat priv with all the soft interface information
@@ -1471,8 +1501,8 @@ batadv_mcast_forw_tt(struct batadv_priv *bat_priv, struct sk_buff *skb,
 			break;
 		}
 
-		batadv_send_skb_unicast(bat_priv, newskb, BATADV_UNICAST, 0,
-					orig_entry->orig_node, vid);
+		batadv_mcast_forw_send_orig(bat_priv, newskb, vid,
+					    orig_entry->orig_node);
 	}
 	rcu_read_unlock();
 
@@ -1513,8 +1543,7 @@ batadv_mcast_forw_want_all_ipv4(struct batadv_priv *bat_priv,
 			break;
 		}
 
-		batadv_send_skb_unicast(bat_priv, newskb, BATADV_UNICAST, 0,
-					orig_node, vid);
+		batadv_mcast_forw_send_orig(bat_priv, newskb, vid, orig_node);
 	}
 	rcu_read_unlock();
 	return ret;
@@ -1551,8 +1580,7 @@ batadv_mcast_forw_want_all_ipv6(struct batadv_priv *bat_priv,
 			break;
 		}
 
-		batadv_send_skb_unicast(bat_priv, newskb, BATADV_UNICAST, 0,
-					orig_node, vid);
+		batadv_mcast_forw_send_orig(bat_priv, newskb, vid, orig_node);
 	}
 	rcu_read_unlock();
 	return ret;
@@ -1618,8 +1646,7 @@ batadv_mcast_forw_want_all_rtr4(struct batadv_priv *bat_priv,
 			break;
 		}
 
-		batadv_send_skb_unicast(bat_priv, newskb, BATADV_UNICAST, 0,
-					orig_node, vid);
+		batadv_mcast_forw_send_orig(bat_priv, newskb, vid, orig_node);
 	}
 	rcu_read_unlock();
 	return ret;
@@ -1656,8 +1683,7 @@ batadv_mcast_forw_want_all_rtr6(struct batadv_priv *bat_priv,
 			break;
 		}
 
-		batadv_send_skb_unicast(bat_priv, newskb, BATADV_UNICAST, 0,
-					orig_node, vid);
+		batadv_mcast_forw_send_orig(bat_priv, newskb, vid, orig_node);
 	}
 	rcu_read_unlock();
 	return ret;
diff --git a/net/batman-adv/multicast.h b/net/batman-adv/multicast.h
index ebf82599..3e114bc5 100644
--- a/net/batman-adv/multicast.h
+++ b/net/batman-adv/multicast.h
@@ -46,6 +46,11 @@ enum batadv_forw_mode
 batadv_mcast_forw_mode(struct batadv_priv *bat_priv, struct sk_buff *skb,
 		       struct batadv_orig_node **mcast_single_orig);
 
+int batadv_mcast_forw_send_orig(struct batadv_priv *bat_priv,
+				struct sk_buff *skb,
+				unsigned short vid,
+				struct batadv_orig_node *orig_node);
+
 int batadv_mcast_forw_send(struct batadv_priv *bat_priv, struct sk_buff *skb,
 			   unsigned short vid);
 
@@ -71,6 +76,16 @@ batadv_mcast_forw_mode(struct batadv_priv *bat_priv, struct sk_buff *skb,
 	return BATADV_FORW_ALL;
 }
 
+static inline int
+batadv_mcast_forw_send_orig(struct batadv_priv *bat_priv,
+			    struct sk_buff *skb,
+			    unsigned short vid,
+			    struct batadv_orig_node *orig_node)
+{
+	kfree_skb(skb);
+	return NET_XMIT_DROP;
+}
+
 static inline int
 batadv_mcast_forw_send(struct batadv_priv *bat_priv, struct sk_buff *skb,
 		       unsigned short vid)
diff --git a/net/batman-adv/soft-interface.c b/net/batman-adv/soft-interface.c
index 23833a0b..3d037b17 100644
--- a/net/batman-adv/soft-interface.c
+++ b/net/batman-adv/soft-interface.c
@@ -364,9 +364,8 @@ static netdev_tx_t batadv_interface_tx(struct sk_buff *skb,
 				goto dropped;
 			ret = batadv_send_skb_via_gw(bat_priv, skb, vid);
 		} else if (mcast_single_orig) {
-			ret = batadv_send_skb_unicast(bat_priv, skb,
-						      BATADV_UNICAST, 0,
-						      mcast_single_orig, vid);
+			ret = batadv_mcast_forw_send_orig(bat_priv, skb, vid,
+							  mcast_single_orig);
 		} else if (forw_mode == BATADV_FORW_SOME) {
 			ret = batadv_mcast_forw_send(bat_priv, skb, vid);
 		} else {
-- 
2.28.0

Re: [PATCH maint v5 1/3] batman-adv: mcast: fix duplicate mcast packets in BLA backbone from LAN

[Simon Wunderlich]

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

On Monday, September 14, 2020 9:53:45 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>
> ---

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 v5 2/3] 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?

The gateway feature uses a batman-adv unicast 4 address header. While
the multicast-to-unicasts feature uses a simple, 3 address batman-adv
unicast header. So let's use this to distinguish.

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 | 34 +++++++++++++++++++-------
 net/batman-adv/bridge_loop_avoidance.h |  4 +--
 net/batman-adv/soft-interface.c        |  6 ++---
 3 files changed, 30 insertions(+), 14 deletions(-)

diff --git a/net/batman-adv/bridge_loop_avoidance.c b/net/batman-adv/bridge_loop_avoidance.c
index d8c5d317..3d2a66f2 100644
--- a/net/batman-adv/bridge_loop_avoidance.c
+++ b/net/batman-adv/bridge_loop_avoidance.c
@@ -1813,7 +1813,7 @@ batadv_bla_loopdetect_check(struct batadv_priv *bat_priv, struct sk_buff *skb,
  * @bat_priv: the bat priv with all the soft interface information
  * @skb: the frame to be checked
  * @vid: the VLAN ID of the frame
- * @is_bcast: the packet came in a broadcast packet type.
+ * @packet_type: the batman packet type this frame came in
  *
  * batadv_bla_rx avoidance checks if:
  *  * we have to race for a claim
@@ -1825,7 +1825,7 @@ batadv_bla_loopdetect_check(struct batadv_priv *bat_priv, struct sk_buff *skb,
  * further process the skb.
  */
 bool batadv_bla_rx(struct batadv_priv *bat_priv, struct sk_buff *skb,
-		   unsigned short vid, bool is_bcast)
+		   unsigned short vid, int packet_type)
 {
 	struct batadv_bla_backbone_gw *backbone_gw;
 	struct ethhdr *ethhdr;
@@ -1847,9 +1847,24 @@ bool batadv_bla_rx(struct batadv_priv *bat_priv, struct sk_buff *skb,
 		goto handled;
 
 	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)
-			goto handled;
+		/* don't allow multicast packets while requests are in flight */
+		if (is_multicast_ether_addr(ethhdr->h_dest))
+			/* Both broadcast flooding or multicast-via-unicasts
+			 * delivery might send to multiple backbone gateways
+			 * sharing the same LAN and therefore need to coordinate
+			 * which backbone gateway forwards into the LAN,
+			 * by claiming the payload source address.
+			 *
+			 * Broadcast flooding and multicast-via-unicasts
+			 * delivery use the following two batman packet types.
+			 * Note: explicitly exclude BATADV_UNICAST_4ADDR,
+			 * as the DHCP gateway feature will send explicitly
+			 * to only one BLA gateway, so the claiming process
+			 * should be avoided there.
+			 */
+			if (packet_type == BATADV_BCAST ||
+			    packet_type == BATADV_UNICAST)
+				goto handled;
 
 	ether_addr_copy(search_claim.addr, ethhdr->h_source);
 	search_claim.vid = vid;
@@ -1884,13 +1899,14 @@ bool batadv_bla_rx(struct batadv_priv *bat_priv, struct sk_buff *skb,
 		goto allow;
 	}
 
-	/* if it is a broadcast ... */
-	if (is_multicast_ether_addr(ethhdr->h_dest) && is_bcast) {
+	/* if it is a multicast ... */
+	if (is_multicast_ether_addr(ethhdr->h_dest) &&
+	    (packet_type == BATADV_BCAST || packet_type == BATADV_UNICAST)) {
 		/* ... drop it. the responsible gateway is in charge.
 		 *
-		 * We need to check is_bcast because with the gateway
+		 * We need to check packet type because with the gateway
 		 * feature, broadcasts (like DHCP requests) may be sent
-		 * using a unicast packet type.
+		 * using a unicast 4 address packet type. See comment above.
 		 */
 		goto handled;
 	} else {
diff --git a/net/batman-adv/bridge_loop_avoidance.h b/net/batman-adv/bridge_loop_avoidance.h
index 41edb2c4..a81c41b6 100644
--- a/net/batman-adv/bridge_loop_avoidance.h
+++ b/net/batman-adv/bridge_loop_avoidance.h
@@ -35,7 +35,7 @@ static inline bool batadv_bla_is_loopdetect_mac(const uint8_t *mac)
 
 #ifdef CONFIG_BATMAN_ADV_BLA
 bool batadv_bla_rx(struct batadv_priv *bat_priv, struct sk_buff *skb,
-		   unsigned short vid, bool is_bcast);
+		   unsigned short vid, int packet_type);
 bool batadv_bla_tx(struct batadv_priv *bat_priv, struct sk_buff *skb,
 		   unsigned short vid);
 bool batadv_bla_is_backbone_gw(struct sk_buff *skb,
@@ -66,7 +66,7 @@ bool batadv_bla_check_claim(struct batadv_priv *bat_priv, u8 *addr,
 
 static inline bool batadv_bla_rx(struct batadv_priv *bat_priv,
 				 struct sk_buff *skb, unsigned short vid,
-				 bool is_bcast)
+				 int packet_type)
 {
 	return false;
 }
diff --git a/net/batman-adv/soft-interface.c b/net/batman-adv/soft-interface.c
index 3d037b17..cdde943c 100644
--- a/net/batman-adv/soft-interface.c
+++ b/net/batman-adv/soft-interface.c
@@ -424,10 +424,10 @@ void batadv_interface_rx(struct net_device *soft_iface,
 	struct vlan_ethhdr *vhdr;
 	struct ethhdr *ethhdr;
 	unsigned short vid;
-	bool is_bcast;
+	int packet_type;
 
 	batadv_bcast_packet = (struct batadv_bcast_packet *)skb->data;
-	is_bcast = (batadv_bcast_packet->packet_type == BATADV_BCAST);
+	packet_type = batadv_bcast_packet->packet_type;
 
 	skb_pull_rcsum(skb, hdr_size);
 	skb_reset_mac_header(skb);
@@ -470,7 +470,7 @@ void batadv_interface_rx(struct net_device *soft_iface,
 	/* Let the bridge loop avoidance check the packet. If will
 	 * not handle it, we can safely push it up.
 	 */
-	if (batadv_bla_rx(bat_priv, skb, vid, is_bcast))
+	if (batadv_bla_rx(bat_priv, skb, vid, packet_type))
 		goto out;
 
 	if (orig_node)
-- 
2.28.0

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

[Simon Wunderlich]

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

On Monday, September 14, 2020 9:53:46 PM CEST Linus Lüssing wrote:
> 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?
> 
> The gateway feature uses a batman-adv unicast 4 address header. While
> the multicast-to-unicasts feature uses a simple, 3 address batman-adv
> unicast header. So let's use this to distinguish.
> 
> Fixes: e32470167379 ("batman-adv: check incoming packet type for bla")
> 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 v5 3/3] 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 | 107 +++++++++++++++++++++----
 1 file changed, 92 insertions(+), 15 deletions(-)

diff --git a/net/batman-adv/bridge_loop_avoidance.c b/net/batman-adv/bridge_loop_avoidance.c
index 3d2a66f2..e4f66dd8 100644
--- a/net/batman-adv/bridge_loop_avoidance.c
+++ b/net/batman-adv/bridge_loop_avoidance.c
@@ -1580,13 +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
+ * @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
+ * @orig: originator mac address, NULL if unknown
  *
- * 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.
+ * 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
@@ -1595,23 +1598,23 @@ 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);
 
 	for (i = 0; i < BATADV_DUPLIST_SIZE; i++) {
+		bool is_from_same_orig = false;
+
 		curr = (bat_priv->bla.bcast_duplist_curr + i);
 		curr %= BATADV_DUPLIST_SIZE;
 		entry = &bat_priv->bla.bcast_duplist[curr];
@@ -1626,7 +1629,24 @@ 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))
+		/* are the originators both known and not anonymous? */
+		if (orig && !is_zero_ether_addr(orig) &&
+		    !is_zero_ether_addr(entry->orig)) {
+			/* if known, check if the new frame came from
+			 * the same originator
+			 */
+			if (batadv_compare_eth(entry->orig, orig))
+				is_from_same_orig = true;
+		}
+
+		/* we are safe to take identical frames from the
+		 * same orig, if known, as multiplications in
+		 * the mesh are detected via the (orig, seqno) pair;
+		 * so we can be a bit more liberal here and allow
+		 * identical frames from the same orig which the source
+		 * host might have sent multiple times on purpose
+		 */
+		if (is_from_same_orig)
 			continue;
 
 		/* this entry seems to match: same crc, not too old,
@@ -1643,7 +1663,14 @@ 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);
+
+	/* known originator */
+	if (orig)
+		ether_addr_copy(entry->orig, orig);
+	/* anonymous originator */
+	else
+		eth_zero_addr(entry->orig);
+
 	bat_priv->bla.bcast_duplist_curr = curr;
 
 out:
@@ -1652,6 +1679,48 @@ 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)
+{
+	return batadv_bla_check_duplist(bat_priv, skb, (u8 *)skb->data, NULL);
+}
+
+/**
+ * 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.
@@ -1866,6 +1935,14 @@ bool batadv_bla_rx(struct batadv_priv *bat_priv, struct sk_buff *skb,
 			    packet_type == BATADV_UNICAST)
 				goto handled;
 
+	/* potential duplicates from foreign BLA backbone gateways via
+	 * multicast-in-unicast packets
+	 */
+	if (is_multicast_ether_addr(ethhdr->h_dest) &&
+	    packet_type == BATADV_UNICAST &&
+	    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 v5 3/3] batman-adv: mcast: fix duplicate mcast packets from BLA backbone to mesh

[Simon Wunderlich]

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

On Monday, September 14, 2020 9:53:47 PM CEST Linus Lüssing wrote:
> 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 | 107 +++++++++++++++++++++----
>  1 file changed, 92 insertions(+), 15 deletions(-)
> 
> diff --git a/net/batman-adv/bridge_loop_avoidance.c
> b/net/batman-adv/bridge_loop_avoidance.c index 3d2a66f2..e4f66dd8 100644
> --- a/net/batman-adv/bridge_loop_avoidance.c
> +++ b/net/batman-adv/bridge_loop_avoidance.c
> @@ -1580,13 +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
> + * @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
> + * @orig: originator mac address, NULL if unknown
>   *
> - * 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.
> + * 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
> @@ -1595,23 +1598,23 @@ 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);
> 
>  	for (i = 0; i < BATADV_DUPLIST_SIZE; i++) {
> +		bool is_from_same_orig = false;
> +
>  		curr = (bat_priv->bla.bcast_duplist_curr + i);
>  		curr %= BATADV_DUPLIST_SIZE;
>  		entry = &bat_priv->bla.bcast_duplist[curr];
> @@ -1626,7 +1629,24 @@ 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))
> +		/* are the originators both known and not anonymous? */
> +		if (orig && !is_zero_ether_addr(orig) &&
> +		    !is_zero_ether_addr(entry->orig)) {
> +			/* if known, check if the new frame came from
> +			 * the same originator
> +			 */
> +			if (batadv_compare_eth(entry->orig, orig))
> +				is_from_same_orig = true;
> +		}
> +
> +		/* we are safe to take identical frames from the
> +		 * same orig, if known, as multiplications in
> +		 * the mesh are detected via the (orig, seqno) pair;
> +		 * so we can be a bit more liberal here and allow
> +		 * identical frames from the same orig which the source
> +		 * host might have sent multiple times on purpose
> +		 */
> +		if (is_from_same_orig)
>  			continue;
> 

I think this "is_from_same_orig" variable is redundant, it's not used again so 
you could continue directly above.

The rest looks good.

Cheers,
      Simon


>  		/* this entry seems to match: same crc, not too old,
> @@ -1643,7 +1663,14 @@ 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);
> +
> +	/* known originator */
> +	if (orig)
> +		ether_addr_copy(entry->orig, orig);
> +	/* anonymous originator */
> +	else
> +		eth_zero_addr(entry->orig);
> +
>  	bat_priv->bla.bcast_duplist_curr = curr;
> 
>  out:
> @@ -1652,6 +1679,48 @@ 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)
> +{
> +	return batadv_bla_check_duplist(bat_priv, skb, (u8 *)skb->data, NULL);
> +}
> +
> +/**
> + * 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.
> @@ -1866,6 +1935,14 @@ bool batadv_bla_rx(struct batadv_priv *bat_priv,
> struct sk_buff *skb, packet_type == BATADV_UNICAST)
>  				goto handled;
> 
> +	/* potential duplicates from foreign BLA backbone gateways via
> +	 * multicast-in-unicast packets
> +	 */
> +	if (is_multicast_ether_addr(ethhdr->h_dest) &&
> +	    packet_type == BATADV_UNICAST &&
> +	    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);


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