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