[maint,v2,3/4] batman-adv: mcast: fix duplicate mcast packets in BLA backbone from mesh
Message ID | 20200904182803.8428-4-linus.luessing@c0d3.blue |
---|---|
State | Superseded, archived |
Delegated to: | Simon Wunderlich |
Headers |
Return-Path: <b.a.t.m.a.n-bounces@lists.open-mesh.org> X-Original-To: patchwork@open-mesh.org Delivered-To: patchwork@open-mesh.org Received: from diktynna.open-mesh.org (localhost [IPv6:::1]) by diktynna.open-mesh.org (Postfix) with ESMTP id 350378079A; Fri, 4 Sep 2020 20:28:16 +0200 (CEST) Received: from mail.aperture-lab.de (mail.aperture-lab.de [138.201.29.205]) by diktynna.open-mesh.org (Postfix) with ESMTPS id A3B3D803F5 for <b.a.t.m.a.n@lists.open-mesh.org>; Fri, 4 Sep 2020 20:28:08 +0200 (CEST) From: =?utf-8?q?Linus_L=C3=BCssing?= <linus.luessing@c0d3.blue> DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=c0d3.blue; s=2018; t=1599244088; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=N0suu9SmFCnuF08sj9iU30UuqseVIIpflbWICWb+qBE=; b=KqVJuQD5PYkcvlz1ZfkUS3gA8BOqcircH2VxySOkbK6gvlZPTh7dhAB6ZrzzjNEGUOkJ5K xibI/Y4T37T2EyKz23LpapvzZ7b003L1RDGNhHVz221PqLCZSikK6opI2iOWxZFZk+wPbl XI31JWcyn0ChK3fn6kW/fnPEJXujaiBsU9kWD0c8O7SeT89CXCufn8mmf6olUSz8ZCXeZv V5CpZWVdUR3kGFnbaGKar8WWNmVOun1wzXFkkh7R8o29R+OHDnsU0wkeQ5+Q/I/DYH4u7j TESEmQ/NEvWTACY8VLmJwL647frd0o+hy6t4P+bJAfC5rJ+bKoMTQEQzI6CYqQ== To: b.a.t.m.a.n@lists.open-mesh.org Cc: =?utf-8?q?Linus_L=C3=BCssing?= <linus.luessing@c0d3.blue> Subject: [PATCH maint v2 3/4] batman-adv: mcast: fix duplicate mcast packets in BLA backbone from mesh Date: Fri, 4 Sep 2020 20:28:02 +0200 Message-Id: <20200904182803.8428-4-linus.luessing@c0d3.blue> In-Reply-To: <20200904182803.8428-1-linus.luessing@c0d3.blue> References: <20200904182803.8428-1-linus.luessing@c0d3.blue> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Authentication-Results: ORIGINATING; auth=pass smtp.auth=linus.luessing@c0d3.blue smtp.mailfrom=linus.luessing@c0d3.blue ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=open-mesh.org; s=20121; t=1599244088; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=N0suu9SmFCnuF08sj9iU30UuqseVIIpflbWICWb+qBE=; b=D6e1+o9ysdgp+7G41P4s9W7t8zDKCO7eWo5fbP+BhGf1VasSxKI/9RCO4nmgyS5+3lNhPH /ZrvaMrhxq/rgaRKssZH9NlzR3w13jHFtt1m4ifvUof/tgcyXYwRreQqPu6mWjE2aHCUH/ doIGcSPoU56wGBKomGQ8LnrKEBI1vwE= ARC-Seal: i=1; s=20121; d=open-mesh.org; t=1599244088; a=rsa-sha256; cv=none; b=NynJ4q3wv3qGzk09Nhmpbn/Ah/7RdYV7n0XAhrzG0S8geFcmbv5CPZz2YsxArwmlwj6tpZ wQW6QOvcwcUe4LKPh5bSJI5heSk8+x8Vq119rFZY9xbos+0P1j/hj91KTydFlltCDi17Sq avglO5P4kDrJbfeuxnELqUX2Zu7AZlA= ARC-Authentication-Results: i=1; diktynna.open-mesh.org; dkim=none (invalid DKIM record) header.d=c0d3.blue header.s=2018 header.b=KqVJuQD5; spf=none (diktynna.open-mesh.org: domain of linus.luessing@c0d3.blue has no SPF policy when checking 138.201.29.205) smtp.mailfrom=linus.luessing@c0d3.blue Content-Transfer-Encoding: quoted-printable Message-ID-Hash: QEB34OGOG3GECYXRTBFNO3T7JMQ3ZDW4 X-Message-ID-Hash: QEB34OGOG3GECYXRTBFNO3T7JMQ3ZDW4 X-MailFrom: linus.luessing@c0d3.blue X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; header-match-b.a.t.m.a.n.lists.open-mesh.org-0; header-match-b.a.t.m.a.n.lists.open-mesh.org-1; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; suspicious-header X-Mailman-Version: 3.2.1 Precedence: list Reply-To: The list for a Better Approach To Mobile Ad-hoc Networking <b.a.t.m.a.n@lists.open-mesh.org> List-Id: The list for a Better Approach To Mobile Ad-hoc Networking <b.a.t.m.a.n.lists.open-mesh.org> Archived-At: <https://lists.open-mesh.org/mailman3/hyperkitty/list/b.a.t.m.a.n@lists.open-mesh.org/message/QEB34OGOG3GECYXRTBFNO3T7JMQ3ZDW4/> List-Archive: <https://lists.open-mesh.org/mailman3/hyperkitty/list/b.a.t.m.a.n@lists.open-mesh.org/> List-Help: <mailto:b.a.t.m.a.n-request@lists.open-mesh.org?subject=help> List-Post: <mailto:b.a.t.m.a.n@lists.open-mesh.org> List-Subscribe: <mailto:b.a.t.m.a.n-join@lists.open-mesh.org> List-Unsubscribe: <mailto:b.a.t.m.a.n-leave@lists.open-mesh.org> |
Series |
batman-adv: mcast: TT/BLA fixes
|
|
Commit Message
Linus Lüssing
Sept. 4, 2020, 6:28 p.m. UTC
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(-)
Comments
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
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