[1/4] batman-adv: Prevent mutliple ARP replies sent by gateways in bla setups with dat enabled
Commit Message
From 2b90abdf53e9ab09d9acfd141c7225de1ae16719 Mon Sep 17 00:00:00 2001
From: Andreas Pape <apape@phoenixcontact.com>
Date: Fri, 12 Feb 2016 10:05:57 +0100
Subject: [PATCH 1/4] batman-adv: Prevent mutliple ARP replies sent by
gateways in bla setups with dat enabled
This patch shall make 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 | 58
++++++++++++++++++++++++++++++++
net/batman-adv/bridge_loop_avoidance.h | 6 +++
net/batman-adv/distributed-arp-table.c | 14 ++++++++
3 files changed, 78 insertions(+), 0 deletions(-)
+ 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);
--
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
On Friday 12 February 2016 14:51:32 Andreas Pape wrote:
> From 2b90abdf53e9ab09d9acfd141c7225de1ae16719 Mon Sep 17 00:00:00 2001
> From: Andreas Pape <apape@phoenixcontact.com>
> Date: Fri, 12 Feb 2016 10:05:57 +0100
> Subject: [PATCH 1/4] batman-adv: Prevent mutliple ARP replies sent by
> gateways in bla setups with dat enabled
>
This patch is corrupt:
Applying: batman-adv: Prevent mutliple ARP replies sent by gateways in bla setups with dat enabled
fatal: corrupt patch at line 32
Patch failed at 0001 batman-adv: Prevent mutliple ARP replies sent by gateways in bla setups with dat enabled
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Please read https://www.kernel.org/doc/Documentation/email-clients.txt
and https://www.kernel.org/doc/Documentation/SubmittingPatches
Kind regards,
Sven
On Friday 12 February 2016 18:24:50 Andreas Pape wrote:
> Am I right that the problem comes from the e-mail client I use? Or is
> there an issue with the patch as such (i.e. how I created it with git - I
> tried to follow the description on open-mesh.org)?
It looks like your client in corrupting the patch. The first link should
explain how to configure some clients. Maybe you can follow the steps to
configure your client.
> I conclude from the
> error message in your mail that it's more likely that I have become a
> victim of my e-mail client.... If this is the case, I will follow the
> advice given in one of the links you refered to and send the patches to
> myself first and try out if they can be applied first before I make a
> second attempt to send them to the mailing list. Unfortunately I'm not
> allowed to use another e-mail client, but I think I can squeeze some
> settings for pure text e-mails (like automatic line breaks).
Usually the easiest way is to use git-send-email. But this would be a
different email client.
And let me guess, you have to use your phoenixcontact.com mail account to
contribute to projects... If nothing else works then maybe you can send it
just as attachments. It is a little bit ugly to comment on attachments but
better than no patches at all. (I hope it is ok for the maintainers)
> Sorry for spamming the mailing list with nonsense....
No problem. I've send more nonsense to the list than anyone else :)
> Just for my curiosity: are patches automatically applied to a repository
> if I send them to the mailing list? If this is the case, I didn't know. I
> would have been more cautious otherwise.
No, the patches are not applied anywhere automatically. Marek will go through
the patches and apply them manually after they were discussed on the
mailinglist.
Some projects use a tool called patchwork that gathers patches but even then
it wouldn't be applied in a git repo automatically.
Kind regards,
Sven
On Friday 12 February 2016 14:51:32 Andreas Pape wrote:
> From 2b90abdf53e9ab09d9acfd141c7225de1ae16719 Mon Sep 17 00:00:00 2001
> From: Andreas Pape <apape@phoenixcontact.com>
> Date: Fri, 12 Feb 2016 10:05:57 +0100
> Subject: [PATCH 1/4] batman-adv: Prevent mutliple ARP replies sent by
> gateways in bla setups with dat enabled
>
> This patch shall make 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.
This patch looks good in general. I can not apply it though, please check the
links that Sven posted how to set up your mail client to send patches. Also,
the commit message seems to have too long lines. Usually your git client
should limit those to ~72 characters per line (I'm not sure about the actual
limit)
>
> Signed-off-by: Andreas Pape <apape@phoenixcontact.com>
> ---
> net/batman-adv/bridge_loop_avoidance.c | 58
> ++++++++++++++++++++++++++++++++
> net/batman-adv/bridge_loop_avoidance.h | 6 +++
> net/batman-adv/distributed-arp-table.c | 14 ++++++++
> 3 files changed, 78 insertions(+), 0 deletions(-)
>
> diff --git a/net/batman-adv/bridge_loop_avoidance.c
> b/net/batman-adv/bridge_loop_avoidance.c
> index 0a6c8b8..c70363d 100644
> --- a/net/batman-adv/bridge_loop_avoidance.c
> +++ b/net/batman-adv/bridge_loop_avoidance.c
> @@ -1906,3 +1906,61 @@ out:
> batadv_hardif_put(primary_if);
> return 0;
> }
> +
> +/**
> + * batadv_check_local_claim
You should put a short description here, like
batadv_check_local_claim - check if the address has been claimed by the local
backbone
> + * @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
> + *
> + * batadv_check_local_claim:
Please remove the repetition of the function name
> + * addr is checked if this address is claimed by the local device itself.
> + * If the address is not claimed at all, claim it.
> + * returns true if bla is disabled or the mac is claimed by the device
> + * returns false if the device addr is already claimed by another gateway
> + */
Should put Return: and then describe the return values. Please check the other
functions for reference.
kerneldoc is parsed automatically and must therefore be in the right format.
> +bool batadv_bla_check_local_claim(struct batadv_priv *bat_priv, uint8_t
> *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)) {
You can save an intendation by doing a return here immediately
> +
> + primary_if = batadv_primary_if_get_selected(bat_priv);
> + if (!primary_if)
> + return ret;
I'd prefer a goto here. If we have other stuff to clean up when we change this
function later, we may forget that this is not done because we used return
here.
> +
> + /* 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;
> + }
braces not needed
> + } else {
> + /* If there is no claim, claim the device */
> + batadv_dbg(BATADV_DBG_BLA, bat_priv, "No claim
> found for %pM. Claim mac for us.\n",
> + search_claim.addr);
Maybe put something in the debug code where this was called from? This looks
like a very generic claim message.
> +
> + batadv_handle_claim(bat_priv,
> + primary_if,
> + primary_if->net_dev->dev_addr, addr,
> + vid);
I wonder if we should rename the function somehow, since actively claiming
goes beyond "just checking". Maybe handle_local_claim.?
Also, primary_if can go on the line above I guess.
Cheers,
Simon
On Monday 15 February 2016 09:50:18 Simon Wunderlich wrote:
> On Friday 12 February 2016 14:51:32 Andreas Pape wrote:
> > From 2b90abdf53e9ab09d9acfd141c7225de1ae16719 Mon Sep 17 00:00:00 2001
> > From: Andreas Pape <apape@phoenixcontact.com>
> > Date: Fri, 12 Feb 2016 10:05:57 +0100
> > Subject: [PATCH 1/4] batman-adv: Prevent mutliple ARP replies sent by
> > gateways in bla setups with dat enabled
> >
> > This patch shall make 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.
>
> This patch looks good in general. I can not apply it though, please check the
> links that Sven posted how to set up your mail client to send patches. Also,
> the commit message seems to have too long lines. Usually your git client
> should limit those to ~72 characters per line (I'm not sure about the actual
> limit)
See SubmittingPatches:
- The body of the explanation, line wrapped at 75 columns, which will
be copied to the permanent changelog to describe this patch.
This should also have been displayed when you've checked the patch with
Linux's
./scripts/checkpatch.pl --strict 000*.patch
Kind regards,
Sven
Hi Simon
was this the only patch which cannot be applied? What about the others I
sent? I am still working on the e-mail client issue ....
Kind regards,
Andreas
Simon Wunderlich <sw@simonwunderlich.de> schrieb am 15.02.2016 09:50:18:
> Von: Simon Wunderlich <sw@simonwunderlich.de>
> An: b.a.t.m.a.n@lists.open-mesh.org
> Kopie: Andreas Pape <APape@phoenixcontact.com>
> Datum: 15.02.2016 09:50
> Betreff: Re: [B.A.T.M.A.N.] [PATCH 1/4] batman-adv: Prevent mutliple
> ARP replies sent by gateways in bla setups with dat enabled
>
> On Friday 12 February 2016 14:51:32 Andreas Pape wrote:
> > From 2b90abdf53e9ab09d9acfd141c7225de1ae16719 Mon Sep 17 00:00:00 2001
> > From: Andreas Pape <apape@phoenixcontact.com>
> > Date: Fri, 12 Feb 2016 10:05:57 +0100
> > Subject: [PATCH 1/4] batman-adv: Prevent mutliple ARP replies sent by
> > gateways in bla setups with dat enabled
> >
> > This patch shall make 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.
>
> This patch looks good in general. I can not apply it though, please
check the
> links that Sven posted how to set up your mail client to send patches.
Also,
> the commit message seems to have too long lines. Usually your git client
> should limit those to ~72 characters per line (I'm not sure about the
actual
> limit)
>
> >
> > Signed-off-by: Andreas Pape <apape@phoenixcontact.com>
> > ---
> > net/batman-adv/bridge_loop_avoidance.c | 58
> > ++++++++++++++++++++++++++++++++
> > net/batman-adv/bridge_loop_avoidance.h | 6 +++
> > net/batman-adv/distributed-arp-table.c | 14 ++++++++
> > 3 files changed, 78 insertions(+), 0 deletions(-)
> >
> > diff --git a/net/batman-adv/bridge_loop_avoidance.c
> > b/net/batman-adv/bridge_loop_avoidance.c
> > index 0a6c8b8..c70363d 100644
> > --- a/net/batman-adv/bridge_loop_avoidance.c
> > +++ b/net/batman-adv/bridge_loop_avoidance.c
> > @@ -1906,3 +1906,61 @@ out:
> > batadv_hardif_put(primary_if);
> > return 0;
> > }
> > +
> > +/**
> > + * batadv_check_local_claim
>
> You should put a short description here, like
>
> batadv_check_local_claim - check if the address has been claimed by the
local
> backbone
>
> > + * @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
> > + *
> > + * batadv_check_local_claim:
>
> Please remove the repetition of the function name
>
> > + * addr is checked if this address is claimed by the local device
itself.
> > + * If the address is not claimed at all, claim it.
> > + * returns true if bla is disabled or the mac is claimed by the
device
> > + * returns false if the device addr is already claimed by another
gateway
> > + */
>
> Should put Return: and then describe the return values. Please checkthe
other
> functions for reference.
>
> kerneldoc is parsed automatically and must therefore be in the right
format.
>
> > +bool batadv_bla_check_local_claim(struct batadv_priv *bat_priv,
uint8_t
> > *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)) {
>
> You can save an intendation by doing a return here immediately
>
> > +
> > + primary_if = batadv_primary_if_get_selected(bat_priv);
> > + if (!primary_if)
> > + return ret;
>
> I'd prefer a goto here. If we have other stuff to clean up when we
> change this
> function later, we may forget that this is not done because we used
return
> here.
>
> > +
> > + /* 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;
> > + }
>
> braces not needed
>
> > + } else {
> > + /* If there is no claim, claim the device */
> > + batadv_dbg(BATADV_DBG_BLA, bat_priv, "No claim
> > found for %pM. Claim mac for us.\n",
> > + search_claim.addr);
>
> Maybe put something in the debug code where this was called from? This
looks
> like a very generic claim message.
>
> > +
> > + batadv_handle_claim(bat_priv,
> > + primary_if,
> > + primary_if->net_dev->dev_addr, addr,
> > + vid);
>
> I wonder if we should rename the function somehow, since actively
claiming
> goes beyond "just checking". Maybe handle_local_claim.?
>
> Also, primary_if can go on the line above I guess.
>
> Cheers,
> Simon[Anhang "signature.asc" gelöscht von Andreas Pape/Phoenix
Contact]
..................................................................
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
On Monday 15 February 2016 10:39:58 Andreas Pape wrote:
> Hi Simon
>
> was this the only patch which cannot be applied? What about the others I
> sent? I am still working on the e-mail client issue ....
All patches were destroyed by your email client.
Kind regards,
Sven
b/net/batman-adv/bridge_loop_avoidance.c
@@ -1906,3 +1906,61 @@ out:
batadv_hardif_put(primary_if);
return 0;
}
+
+/**
+ * batadv_check_local_claim
+ * @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
+ *
+ * batadv_check_local_claim:
+ * addr is checked if this address is claimed by the local device itself.
+ * If the address is not claimed at all, claim it.
+ * returns true if bla is disabled or the mac is claimed by the device
+ * returns false if the device addr is already claimed by another gateway
+ */
+bool batadv_bla_check_local_claim(struct batadv_priv *bat_priv, uint8_t
*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)) {
+
+ 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;
+ }
+ } else {
+ /* If there is no claim, claim the device */
+ batadv_dbg(BATADV_DBG_BLA, bat_priv, "No claim
found for %pM. Claim mac for us.\n",
+ search_claim.addr);
+
+ batadv_handle_claim(bat_priv,
+ primary_if,
+ primary_if->net_dev->dev_addr, addr,
+ vid);
+ }
+ }
+
+ if (claim)
+ batadv_claim_put(claim);
+ if (primary_if)
+ batadv_hardif_put(primary_if);
+ return ret;
+}
b/net/batman-adv/bridge_loop_avoidance.h
@@ -46,6 +46,7 @@ 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);
+bool batadv_bla_check_local_claim(struct batadv_priv *bat_priv, u8 *addr,
unsigned short vid);
#define BATADV_BLA_CRC_INIT 0
#else /* ifdef CONFIG_BATMAN_ADV_BLA */
@@ -111,6 +112,11 @@ static inline void batadv_bla_free(struct batadv_priv
*bat_priv)
{
}
+bool batadv_bla_check_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_ */
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"
static void batadv_dat_purge(struct work_struct *work);
@@ -1000,6 +1001,19 @@ bool batadv_dat_snoop_outgoing_arp_request(struct
batadv_priv *bat_priv,
goto out;
}
+ /* If BLA is enabled, only send ARP REPLYs 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_check_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
into common backbone !",
+ dat_entry->mac_addr);