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

[Simon Wunderlich <sw@simonwunderlich.de>]

[-- 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 --]