[1/2] batman-adv: purge bridge loop avoidance when its disabled
Commit Message
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(-)
Comments
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
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
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
@@ -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
*
@@ -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);
@@ -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,