| Message ID | 20200904182803.8428-4-linus.luessing@c0d3.blue |
|---|---|
| State | Superseded, archived |
| Delegated to: | Simon Wunderlich |
| Headers | show |
| Series | batman-adv: mcast: TT/BLA fixes | expand |
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
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(-)