[v4,1/6] batman-adv: prevent multiple ARP replies sent by gateways if dat enbled
Commit Message
If dat is enabled it must be made sure that only the backbone gw which has
claimed the remote destination for the ARP request answers the ARP request
directly if the MAC address is known due to the local dat table. This
prevents multiple ARP replies in a common backbone if more than one
gateway already knows the remote mac searched for in the ARP request.
Signed-off-by: Andreas Pape <apape@phoenixcontact.com>
---
net/batman-adv/bridge_loop_avoidance.c | 49 ++++++++++++++++++++++++++++++++
net/batman-adv/bridge_loop_avoidance.h | 11 +++++++
net/batman-adv/distributed-arp-table.c | 15 ++++++++++
3 files changed, 75 insertions(+), 0 deletions(-)
--
1.7.0.4
..................................................................
PHOENIX CONTACT ELECTRONICS GmbH
Sitz der Gesellschaft / registered office of the company: 31812 Bad Pyrmont
USt-Id-Nr.: DE811742156
Amtsgericht Hannover HRB 100528 / district court Hannover HRB 100528
Geschäftsführer / Executive Board: Roland Bent, Dr. Martin Heubeck
Comments
> --- a/net/batman-adv/distributed-arp-table.c
> +++ b/net/batman-adv/distributed-arp-table.c
> @@ -48,6 +48,7 @@
> #include "originator.h"
> #include "send.h"
> #include "translation-table.h"
> +#include "bridge_loop_avoidance.h"
Please try to keep the includes in alphabetical order like this:
+#include "bridge_loop_avoidance.h"
#include "hard-interface.h"
#include "hash.h"
#include "originator.h"
#include "send.h"
#include "translation-table.h"
Maybe Marek can also do this when he wants to apply it (in case
Simon/Marek/... don't find anything else problematic in the patchset)
Kind regards,
Sven
On Thursday 12 May 2016 08:31:32 Andreas Pape wrote:
> @@ -1003,6 +1004,20 @@ bool batadv_dat_snoop_outgoing_arp_request(struct
> batadv_priv *bat_priv, goto out;
> }
>
> + /* If BLA is enabled, only send ARP replies if we have claimed
> + * the destination for the ARP request or if no one else of
> + * the backbone gws belonging to our backbone has claimed the
> + * destination.
> + */
> + if (!batadv_bla_handle_local_claim(bat_priv,
> + dat_entry->mac_addr, vid)) {
> + batadv_dbg(BATADV_DBG_DAT, bat_priv,
> + "Device %pM claimed by another backbone gw. Don't send ARP
reply!",
> + dat_entry->mac_addr);
> + ret = true;
> + goto out;
> + }
> +
There is still my question/comment pending for this from your PATCHv2. I'll
quote:
I would agree to Antonio that sending a claim is not necessary, and might
actually lead to bad choices of the outgoing backbone gateway. After all, at
this point any "random" backbone gateway may answer to this request, just
because it has the DAT entry, but maybe may actually not be able to reach the
client via the mesh.
I think checking if a local claim exists is a good idea, but sending a claim
is not a good idea - this might introduce effects which will be even more
complicated to debug.
Cheers,
Simon
Hi Simon,
Simon Wunderlich <sw@simonwunderlich.de> schrieb am 18.05.2016 14:32:09:
> Von: Simon Wunderlich <sw@simonwunderlich.de>
> An: Andreas Pape <APape@phoenixcontact.com>
> Datum: 18.05.2016 14:32
> Betreff: Re: Antwort: Re: [B.A.T.M.A.N.] [PATCH v4 1/6] batman-adv:
> prevent multiple ARP replies sent by gateways if dat enbled
>
> Hi Andreas,
>
> On Wednesday 18 May 2016 09:50:21 Andreas Pape wrote:
> > > Von: Simon Wunderlich <sw@simonwunderlich.de>
> > > An: b.a.t.m.a.n@lists.open-mesh.org
> > > Kopie: Andreas Pape <apape@phoenixcontact.com>
> > > Datum: 13.05.2016 19:37
> > > Betreff: Re: [B.A.T.M.A.N.] [PATCH v4 1/6] batman-adv: prevent
> > > multiple ARP replies sent by gateways if dat enbled
> > >
> > > On Thursday 12 May 2016 08:31:32 Andreas Pape wrote:
> > > > @@ -1003,6 +1004,20 @@ bool
> >
> > batadv_dat_snoop_outgoing_arp_request(struct
> >
> > > > batadv_priv *bat_priv, goto out;
> > > >
> > > > }
> > > >
> > > > + /* If BLA is enabled, only send ARP replies if we have
claimed
> > > > + * the destination for the ARP request or if no one else of
> > > > + * the backbone gws belonging to our backbone has claimed
the
> > > > + * destination.
> > > > + */
> > > > + if (!batadv_bla_handle_local_claim(bat_priv,
> > > > + dat_entry->mac_addr, vid)) {
> > > > + batadv_dbg(BATADV_DBG_DAT, bat_priv,
> > > > + "Device %pM claimed by another backbone gw. Don't
send
> >
> > ARP
> >
> > > reply!",
> > >
> > > > + dat_entry->mac_addr);
> > > > + ret = true;
> > > > + goto out;
> > > > + }
> > > > +
> > >
> > > There is still my question/comment pending for this from your
PATCHv2.
> >
> > I'll
> >
> > > quote:
> > >
> > > I would agree to Antonio that sending a claim is not necessary, and
> >
> > might
> >
> > > actually lead to bad choices of the outgoing backbone gateway. After
> >
> > all, at
> >
> > > this point any "random" backbone gateway may answer to this request,
> >
> > just
> >
> > > because it has the DAT entry, but maybe may actually not be able to
> >
> > reach the
> >
> > > client via the mesh.
>
>
> looks like your reply didn't go to the mailing list, not sure if
> this was your
> intention. Anyway ...
>
That wasn't my intention. It was just my fault;-). Sorry, if you receive
this
twice. I once again forgot to disable html support in my e-mail client...
> >
> > I removed the automatic claim in function
batadv_bla_handle_local_claim
> > but
> > obviously missed to change the comment here accordingly:-( I haven't
had
> > much
> > time recently to work on this and did not spent enough time to work
> > through
> > the new version of the patchset in a way I should have done....
>
> Ah right, I missed that point, and was merely checking my old comments
...
> sorry, I should have checked better too.
>
> >
> > Perhaps it might be a also a good idea to rename the function to
something
> > which cannot be mixed easily with the function batadv_handle_claim.
What
> > about
> > the name batadv_bla_check_claim?
>
> Yes, definitely! We should rename it. check_claim sounds good to me.
>
If there are no other objections I will change the name as discussed in my
next attempt.
> Thanks a lot,
> Simon
Best regards,
Andreas
..................................................................
PHOENIX CONTACT ELECTRONICS GmbH
Sitz der Gesellschaft / registered office of the company: 31812 Bad Pyrmont
USt-Id-Nr.: DE811742156
Amtsgericht Hannover HRB 100528 / district court Hannover HRB 100528
Geschäftsführer / Executive Board: Roland Bent, Dr. Martin Heubeck
@@ -2045,3 +2045,52 @@ out:
batadv_hardif_put(primary_if);
return 0;
}
+
+#ifdef CONFIG_BATMAN_ADV_DAT
+/**
+ * batadv_bla_handle_local_claim - check if address is claimed
+ *
+ * @bat_priv: the bat priv with all the soft interface information
+ * @addr: mac address of which the claim status is checked
+ * @vid: the VLAN ID
+ *
+ * addr is checked if this address is claimed by the local device itself.
+ *
+ * Return: true if bla is disabled or the mac is claimed by the device,
+ * false if the device addr is already claimed by another gateway
+ */
+bool batadv_bla_handle_local_claim(struct batadv_priv *bat_priv,
+ u8 *addr, unsigned short vid)
+{
+ struct batadv_bla_claim search_claim;
+ struct batadv_bla_claim *claim = NULL;
+ struct batadv_hard_iface *primary_if = NULL;
+ bool ret = true;
+
+ if (!atomic_read(&bat_priv->bridge_loop_avoidance))
+ return ret;
+
+ primary_if = batadv_primary_if_get_selected(bat_priv);
+ if (!primary_if)
+ return ret;
+
+ /* First look if the mac address is claimed */
+ ether_addr_copy(search_claim.addr, addr);
+ search_claim.vid = vid;
+
+ claim = batadv_claim_hash_find(bat_priv, &search_claim);
+
+ /* If there is a claim and we are not owner of the claim,
+ * return false.
+ */
+ if (claim) {
+ if (!batadv_compare_eth(claim->backbone_gw->orig,
+ primary_if->net_dev->dev_addr))
+ ret = false;
+ batadv_claim_put(claim);
+ }
+
+ batadv_hardif_put(primary_if);
+ return ret;
+}
+#endif
@@ -47,6 +47,10 @@ void batadv_bla_update_orig_address(struct batadv_priv *bat_priv,
void batadv_bla_status_update(struct net_device *net_dev);
int batadv_bla_init(struct batadv_priv *bat_priv);
void batadv_bla_free(struct batadv_priv *bat_priv);
+#ifdef CONFIG_BATMAN_ADV_DAT
+bool batadv_bla_handle_local_claim(struct batadv_priv *bat_priv, u8 *addr,
+ unsigned short vid);
+#endif
#define BATADV_BLA_CRC_INIT 0
#else /* ifdef CONFIG_BATMAN_ADV_BLA */
@@ -112,6 +116,13 @@ static inline void batadv_bla_free(struct batadv_priv *bat_priv)
{
}
+static inline
+bool batadv_bla_handle_local_claim(struct batadv_priv *bat_priv, u8 *addr,
+ unsigned short vid)
+{
+ return true;
+}
+
#endif /* ifdef CONFIG_BATMAN_ADV_BLA */
#endif /* ifndef _NET_BATMAN_ADV_BLA_H_ */
@@ -48,6 +48,7 @@
#include "originator.h"
#include "send.h"
#include "translation-table.h"
+#include "bridge_loop_avoidance.h"
static void batadv_dat_purge(struct work_struct *work);
@@ -1003,6 +1004,20 @@ bool batadv_dat_snoop_outgoing_arp_request(struct batadv_priv *bat_priv,
goto out;
}
+ /* If BLA is enabled, only send ARP replies if we have claimed
+ * the destination for the ARP request or if no one else of
+ * the backbone gws belonging to our backbone has claimed the
+ * destination.
+ */
+ if (!batadv_bla_handle_local_claim(bat_priv,
+ dat_entry->mac_addr, vid)) {
+ batadv_dbg(BATADV_DBG_DAT, bat_priv,
+ "Device %pM claimed by another backbone gw. Don't send ARP reply!",
+ dat_entry->mac_addr);
+ ret = true;
+ goto out;
+ }
+
skb_new = arp_create(ARPOP_REPLY, ETH_P_ARP, ip_src,
bat_priv->soft_iface, ip_dst, hw_src,
dat_entry->mac_addr, hw_src);