[B.A.T.M.A.N.] [PATCH 0/2] Bridge loop avoidance startup fixes

[B.A.T.M.A.N.] [PATCH 0/2] Bridge loop avoidance startup fixes

[Simon Wunderlich]

From: Simon Wunderlich <simon@open-mesh.com>

When changing configuration on top of batman-adv, for example in firmwares,
there are two cases which appeared to be problematic and causing
initial temporary loops when starting batman-adv.

 * when activating batman-adv with vlans and bridges, it may take
   longer than 30 seconds to start up the bridges. If the grace
   period is over by that time, the BLA grace period may already
   be over. Therefore increase the grace period to 60 seconds.
 * When changing the network configuration, e.g. adding or removing
   wired interface in a bridge, it may be useful to re-start the
   grace period. Therefore the bridge loop avoidance state is dropped
   when BLA is turned off, so turning off/on bla can be used
   to restart the grace period.

These two fixes addressed the problems we had in our setups.

The patches are based on top of the current master (since its technically
more a feature than a fix), plus the kerneldoc fix sent last week.

Cheers,
     Simon

Simon Wunderlich (2):
  batman-adv: purge bridge loop avoidance when its disabled
  batman-adv: increase BLA wait periods to 6

 net/batman-adv/bridge_loop_avoidance.c | 20 ++++++++++++++++++++
 net/batman-adv/bridge_loop_avoidance.h |  1 +
 net/batman-adv/main.h                  |  2 +-
 net/batman-adv/sysfs.c                 |  4 +++-
 4 files changed, 25 insertions(+), 2 deletions(-)

-- 
2.6.2

[B.A.T.M.A.N.] [PATCH 1/2] batman-adv: purge bridge loop avoidance when its disabled

[Simon Wunderlich]

From: Simon Wunderlich <simon@open-mesh.com>

When bridge loop avoidance is disabled through sysfs, the internal
datastructures are not disabled, but only BLA operations are disabled.
To be sure that they are removed, purge the data immediately. That is
especially useful if a firmwares network state is changed, and the BLA
wait periods should restart on the new network.

Signed-off-by: Simon Wunderlich <simon@open-mesh.com>
---
 net/batman-adv/bridge_loop_avoidance.c | 20 ++++++++++++++++++++
 net/batman-adv/bridge_loop_avoidance.h |  1 +
 net/batman-adv/sysfs.c                 |  4 +++-
 3 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/net/batman-adv/bridge_loop_avoidance.c b/net/batman-adv/bridge_loop_avoidance.c
index e068ad9..d9b034a 100644
--- a/net/batman-adv/bridge_loop_avoidance.c
+++ b/net/batman-adv/bridge_loop_avoidance.c
@@ -1247,6 +1247,26 @@ void batadv_bla_update_orig_address(struct batadv_priv *bat_priv,
 }
 
 /**
+ * batadv_bla_status_update - purge bla interfaces if necessary
+ * @net_dev: the soft interface net device
+ */
+void batadv_bla_status_update(struct net_device *net_dev)
+{
+	struct batadv_priv *bat_priv = netdev_priv(net_dev);
+	struct batadv_hard_iface *primary_if;
+
+	primary_if = batadv_primary_if_get_selected(bat_priv);
+	if (!primary_if)
+		return;
+
+	/* this function already purges everything when bla is disabled,
+	 * so just call that one.
+	 */
+	batadv_bla_update_orig_address(bat_priv, primary_if, primary_if);
+	batadv_hardif_free_ref(primary_if);
+}
+
+/**
  * batadv_bla_periodic_work - performs periodic bla work
  * @work: kernel work struct
  *
diff --git a/net/batman-adv/bridge_loop_avoidance.h b/net/batman-adv/bridge_loop_avoidance.h
index 025152b..db1aeb5 100644
--- a/net/batman-adv/bridge_loop_avoidance.h
+++ b/net/batman-adv/bridge_loop_avoidance.h
@@ -42,6 +42,7 @@ int batadv_bla_check_bcast_duplist(struct batadv_priv *bat_priv,
 void batadv_bla_update_orig_address(struct batadv_priv *bat_priv,
 				    struct batadv_hard_iface *primary_if,
 				    struct batadv_hard_iface *oldif);
+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);
 
diff --git a/net/batman-adv/sysfs.c b/net/batman-adv/sysfs.c
index 2bcc7f6..9569494 100644
--- a/net/batman-adv/sysfs.c
+++ b/net/batman-adv/sysfs.c
@@ -40,6 +40,7 @@
 #include "distributed-arp-table.h"
 #include "gateway_client.h"
 #include "gateway_common.h"
+#include "bridge_loop_avoidance.h"
 #include "hard-interface.h"
 #include "network-coding.h"
 #include "packet.h"
@@ -549,7 +550,8 @@ static ssize_t batadv_store_isolation_mark(struct kobject *kobj,
 BATADV_ATTR_SIF_BOOL(aggregated_ogms, S_IRUGO | S_IWUSR, NULL);
 BATADV_ATTR_SIF_BOOL(bonding, S_IRUGO | S_IWUSR, NULL);
 #ifdef CONFIG_BATMAN_ADV_BLA
-BATADV_ATTR_SIF_BOOL(bridge_loop_avoidance, S_IRUGO | S_IWUSR, NULL);
+BATADV_ATTR_SIF_BOOL(bridge_loop_avoidance, S_IRUGO | S_IWUSR,
+		     batadv_bla_status_update);
 #endif
 #ifdef CONFIG_BATMAN_ADV_DAT
 BATADV_ATTR_SIF_BOOL(distributed_arp_table, S_IRUGO | S_IWUSR,
-- 
2.6.2

Re: [B.A.T.M.A.N.] [PATCH 1/2] batman-adv: purge bridge loop avoidance when its disabled

[Marek Lindner]

[-- Attachment #1: Type: text/plain, Size: 906 bytes --]

On Monday, November 09, 2015 16:20:52 Simon Wunderlich wrote:
> -BATADV_ATTR_SIF_BOOL(bridge_loop_avoidance, S_IRUGO | S_IWUSR, NULL);
> +BATADV_ATTR_SIF_BOOL(bridge_loop_avoidance, S_IRUGO | S_IWUSR,
> +                    batadv_bla_status_update);
>  #endif

Are we sure this is correct ? The post function is called whether or not there 
actually was a change in the setting. The check in __batadv_store_bool_attr() 
is this:

ret = batadv_store_bool_attr(buff, count, net_dev, attr->name,
                                     attr_store);
if (post_func && ret)
            post_func(net_dev);

Let's ignore for now that ret should be changed to check for '> 0' to avoid 
calling post_func() in case of an error. The return value is always non-
negative unless the input is broken. You could enable BLA while it already is 
enabled which would reset all claim tables. Is that intended ?

Cheers,
Marek

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [B.A.T.M.A.N.] [PATCH 1/2] batman-adv: purge bridge loop avoidance when its disabled

[Simon Wunderlich]

[-- Attachment #1: Type: text/plain, Size: 1929 bytes --]

On Tuesday 17 November 2015 16:01:27 Marek Lindner wrote:
> On Monday, November 09, 2015 16:20:52 Simon Wunderlich wrote:
> > -BATADV_ATTR_SIF_BOOL(bridge_loop_avoidance, S_IRUGO | S_IWUSR, NULL);
> > +BATADV_ATTR_SIF_BOOL(bridge_loop_avoidance, S_IRUGO | S_IWUSR,
> > +                    batadv_bla_status_update);
> > 
> >  #endif
> 
> Are we sure this is correct ? The post function is called whether or not
> there actually was a change in the setting. The check in
> __batadv_store_bool_attr() is this:
> 
> ret = batadv_store_bool_attr(buff, count, net_dev, attr->name,
>                                      attr_store);
> if (post_func && ret)
>             post_func(net_dev);
> 
> Let's ignore for now that ret should be changed to check for '> 0' to avoid
> calling post_func() in case of an error. The return value is always non-
> negative unless the input is broken. You could enable BLA while it already
> is enabled which would reset all claim tables. Is that intended ?

Its not intended, although my initial thought was that it didn't hurt too much 
- the backbone gateway and claim tables would be dropped and the interface 
would go into the "protected" state again, not allowing broadcasts for 30 (or 
60 seconds, if the second patch is applied).

However, since you brought up this point, I think we should really change the 
behaviour of batadv_store_bool_attr() and friends, only calling post_func if 
there really was a change. I've checked the other functions which use that, 
and there shouldn't be any problem with that as far as I see - they do all 
some changes which depend on actual changes of the respective parameter. The 
other update functions are:

 * batadv_dat_status_update
 * batadv_update_min_mtu
 * batadv_post_gw_reselect
 * batadv_nc_status_update

If you agree, I'd send another patch to change the behaviour as proposed.

Cheers,
    Simon

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [B.A.T.M.A.N.] [PATCH 1/2] batman-adv: purge bridge loop avoidance when its disabled

[Marek Lindner]

[-- Attachment #1: Type: text/plain, Size: 780 bytes --]

On Monday, November 09, 2015 16:20:52 Simon Wunderlich wrote:
> From: Simon Wunderlich <simon@open-mesh.com>
> 
> When bridge loop avoidance is disabled through sysfs, the internal
> datastructures are not disabled, but only BLA operations are disabled.
> To be sure that they are removed, purge the data immediately. That is
> especially useful if a firmwares network state is changed, and the BLA
> wait periods should restart on the new network.
> 
> Signed-off-by: Simon Wunderlich <simon@open-mesh.com>
> ---
>  net/batman-adv/bridge_loop_avoidance.c | 20 ++++++++++++++++++++
>  net/batman-adv/bridge_loop_avoidance.h |  1 +
>  net/batman-adv/sysfs.c                 |  4 +++-
>  3 files changed, 24 insertions(+), 1 deletion(-)

Applied in revision 07ed3c3.

Thanks,
Marek

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[B.A.T.M.A.N.] [PATCH 2/2] batman-adv: increase BLA wait periods to 6

[Simon Wunderlich]

From: Simon Wunderlich <simon@open-mesh.com>

If networks take a long time to come up, e.g. due to lossy links, then
the bridge loop avoidance wait time to suppress broadcasts may not wait
long enough and detect a backbone before the mesh is brought up.
Increasing the wait period further to 60 seconds makes this scenario
less likely.

Signed-off-by: Simon Wunderlich <simon@open-mesh.com>
---
 net/batman-adv/main.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/batman-adv/main.h b/net/batman-adv/main.h
index 79e2052..dcca44b 100644
--- a/net/batman-adv/main.h
+++ b/net/batman-adv/main.h
@@ -109,7 +109,7 @@
 #define BATADV_MAX_AGGREGATION_MS 100
 
 #define BATADV_BLA_PERIOD_LENGTH	10000	/* 10 seconds */
-#define BATADV_BLA_BACKBONE_TIMEOUT	(BATADV_BLA_PERIOD_LENGTH * 3)
+#define BATADV_BLA_BACKBONE_TIMEOUT	(BATADV_BLA_PERIOD_LENGTH * 6)
 #define BATADV_BLA_CLAIM_TIMEOUT	(BATADV_BLA_PERIOD_LENGTH * 10)
 #define BATADV_BLA_WAIT_PERIODS		3
 
-- 
2.6.2

Re: [B.A.T.M.A.N.] [PATCH 2/2] batman-adv: increase BLA wait periods to6

[Marek Lindner]

[-- Attachment #1: Type: text/plain, Size: 603 bytes --]

On Monday, November 09, 2015 16:20:53 Simon Wunderlich wrote:
> From: Simon Wunderlich <simon@open-mesh.com>
> 
> If networks take a long time to come up, e.g. due to lossy links, then
> the bridge loop avoidance wait time to suppress broadcasts may not wait
> long enough and detect a backbone before the mesh is brought up.
> Increasing the wait period further to 60 seconds makes this scenario
> less likely.
> 
> Signed-off-by: Simon Wunderlich <simon@open-mesh.com>
> ---
>  net/batman-adv/main.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Applied in revision c57af3a.

Thanks,
Marek

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 473 bytes --]