[B.A.T.M.A.N.] [PATCHv3 1/2] batman-adv: free an hard-interface before adding it

[B.A.T.M.A.N.] [PATCHv3 1/2] batman-adv: free an hard-interface before adding it

[Antonio Quartulli]

When adding a new hard interface (e.h. wlan0) to a soft interface (e.g. bat0)
and the former is already enslaved in another virtual interface (e.g. a software
bridge) batman-adv has to free the it first and then continue with the
adding mechanism.

In this way the behaviour becomes consistent with what "ip link set master"
does. At the moment batman-adv enslaves the hard interface without checking for
the master device, possibly causing strange behaviours which are never wanted by
the users.

Reported-by: Marek Lindner <lindner_marek@yahoo.de>
Signed-off-by: Antonio Quartulli <ordex@autistici.org>
---
v2:
 - added compat code for netdev_master_upper_dev_get()
v3:
 - use #define instead of a static inline function in compat code

 compat.h         |  5 +++++
 hard-interface.c | 14 ++++++++------
 2 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/compat.h b/compat.h
index 425b3d9..885e551 100644
--- a/compat.h
+++ b/compat.h
@@ -214,6 +214,11 @@ static int __batadv_interface_set_mac_addr(x, y)
 
 #define netdev_master_upper_dev_link netdev_set_master
 #define netdev_upper_dev_unlink(slave, master) netdev_set_master(slave, NULL)
+#define netdev_master_upper_dev_get(dev) \
+({\
+	ASSERT_RTNL();\
+	dev->master;\
+})
 
 #endif /* < KERNEL_VERSION(3, 9, 0) */
 
diff --git a/hard-interface.c b/hard-interface.c
index c08e39e..fd99e42 100644
--- a/hard-interface.c
+++ b/hard-interface.c
@@ -311,7 +311,7 @@ int batadv_hardif_enable_interface(struct batadv_hard_iface *hard_iface,
 				   const char *iface_name)
 {
 	struct batadv_priv *bat_priv;
-	struct net_device *soft_iface;
+	struct net_device *soft_iface, *master;
 	__be16 ethertype = __constant_htons(ETH_P_BATMAN);
 	int ret;
 
@@ -321,11 +321,6 @@ int batadv_hardif_enable_interface(struct batadv_hard_iface *hard_iface,
 	if (!atomic_inc_not_zero(&hard_iface->refcount))
 		goto out;
 
-	/* hard-interface is part of a bridge */
-	if (hard_iface->net_dev->priv_flags & IFF_BRIDGE_PORT)
-		pr_err("You are about to enable batman-adv on '%s' which already is part of a bridge. Unless you know exactly what you are doing this is probably wrong and won't work the way you think it would.\n",
-		       hard_iface->net_dev->name);
-
 	soft_iface = dev_get_by_name(&init_net, iface_name);
 
 	if (!soft_iface) {
@@ -347,6 +342,13 @@ int batadv_hardif_enable_interface(struct batadv_hard_iface *hard_iface,
 		goto err_dev;
 	}
 
+	/* check if the interface is enslaved in another virtual one and
+	 * in that case unlink it first
+	 */
+	master = netdev_master_upper_dev_get(hard_iface->net_dev);
+	if (master)
+		netdev_upper_dev_unlink(hard_iface->net_dev, master);
+
 	hard_iface->soft_iface = soft_iface;
 	bat_priv = netdev_priv(hard_iface->soft_iface);
 
-- 
1.8.1.2

[B.A.T.M.A.N.] [PATCHv3 2/2] batman-adv: remove interfacing enslaving loop private check

[Antonio Quartulli]

Now that the new dev_upper API is used, the private check for enslaving loops is
not needed anymore as this is performed by the new API itself.

Reported-by: Marek Lindner <lindner_marek@yahoo.de>
Signed-off-by: Antonio Quartulli <ordex@autistici.org>
---
 hard-interface.c | 43 -------------------------------------------
 1 file changed, 43 deletions(-)

diff --git a/hard-interface.c b/hard-interface.c
index fd99e42..b96f819 100644
--- a/hard-interface.c
+++ b/hard-interface.c
@@ -60,45 +60,6 @@ out:
 	return hard_iface;
 }
 
-/**
- * batadv_is_on_batman_iface - check if a device is a batman iface descendant
- * @net_dev: the device to check
- *
- * If the user creates any virtual device on top of a batman-adv interface, it
- * is important to prevent this new interface to be used to create a new mesh
- * network (this behaviour would lead to a batman-over-batman configuration).
- * This function recursively checks all the fathers of the device passed as
- * argument looking for a batman-adv soft interface.
- *
- * Returns true if the device is descendant of a batman-adv mesh interface (or
- * if it is a batman-adv interface itself), false otherwise
- */
-static bool batadv_is_on_batman_iface(const struct net_device *net_dev)
-{
-	struct net_device *parent_dev;
-	bool ret;
-
-	/* check if this is a batman-adv mesh interface */
-	if (batadv_softif_is_valid(net_dev))
-		return true;
-
-	/* no more parents..stop recursion */
-	if (net_dev->iflink == net_dev->ifindex)
-		return false;
-
-	/* recurse over the parent device */
-	parent_dev = dev_get_by_index(&init_net, net_dev->iflink);
-	/* if we got a NULL parent_dev there is something broken.. */
-	if (WARN(!parent_dev, "Cannot find parent device"))
-		return false;
-
-	ret = batadv_is_on_batman_iface(parent_dev);
-
-	if (parent_dev)
-		dev_put(parent_dev);
-	return ret;
-}
-
 static int batadv_is_valid_iface(const struct net_device *net_dev)
 {
 	if (net_dev->flags & IFF_LOOPBACK)
@@ -110,10 +71,6 @@ static int batadv_is_valid_iface(const struct net_device *net_dev)
 	if (net_dev->addr_len != ETH_ALEN)
 		return 0;
 
-	/* no batman over batman */
-	if (batadv_is_on_batman_iface(net_dev))
-		return 0;
-
 	return 1;
 }
 
-- 
1.8.1.2

Re: [B.A.T.M.A.N.] [PATCHv3 2/2] batman-adv: remove interfacing enslaving loop private check

[Marek Lindner]

On Saturday, February 16, 2013 21:42:40 Antonio Quartulli wrote:
> Now that the new dev_upper API is used, the private check for enslaving
> loops is not needed anymore as this is performed by the new API itself.

Are you sure ? Isn't the following supposed to fail:

$ batctl if add eth0
$ vconfig add bat0 5
$ batctl if add bat0.5

$ batctl if
  eth0: active
  bat0.5: active

Or what is the expected behavior ?

Cheers,
Marek

Re: [B.A.T.M.A.N.] [PATCHv3 2/2] batman-adv: remove interfacing enslaving loop private check

[Marek Lindner]

On Wednesday, February 27, 2013 18:09:48 Marek Lindner wrote:
> On Saturday, February 16, 2013 21:42:40 Antonio Quartulli wrote:
> > Now that the new dev_upper API is used, the private check for enslaving
> > loops is not needed anymore as this is performed by the new API itself.
> 
> Are you sure ? Isn't the following supposed to fail:
> 
> $ batctl if add eth0
> $ vconfig add bat0 5
> $ batctl if add bat0.5
> 
> $ batctl if
>   eth0: active
>   bat0.5: active

Without your patch this is what happens:

$ batctl if add eth0
$ vconfig add bat0 5
$ batctl if add bat0.5
  Error - interface type not supported by batman-adv: bat0.5

Cheers,
Marek

Re: [B.A.T.M.A.N.] [PATCHv3 2/2] batman-adv: remove interfacing enslaving loop private check

[Antonio Quartulli]

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

On Wed, Feb 27, 2013 at 06:21:18PM +0800, Marek Lindner wrote:
> On Wednesday, February 27, 2013 18:09:48 Marek Lindner wrote:
> > On Saturday, February 16, 2013 21:42:40 Antonio Quartulli wrote:
> > > Now that the new dev_upper API is used, the private check for enslaving
> > > loops is not needed anymore as this is performed by the new API itself.
> > 
> > Are you sure ? Isn't the following supposed to fail:
> > 
> > $ batctl if add eth0
> > $ vconfig add bat0 5
> > $ batctl if add bat0.5
> > 
> > $ batctl if
> >   eth0: active
> >   bat0.5: active
> 
> Without your patch this is what happens:
> 
> $ batctl if add eth0
> $ vconfig add bat0 5
> $ batctl if add bat0.5
>   Error - interface type not supported by batman-adv: bat0.5


Are you testing this with < linux-3.9/net-next ?
We need some compat stuff as if we want to remove this check and keep the
behaviour on old kernels.

Cheers,

/me does not think to old kernels :(

-- 
Antonio Quartulli

..each of us alone is worth nothing..
Ernesto "Che" Guevara

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [B.A.T.M.A.N.] [PATCHv3 2/2] batman-adv: remove interfacing enslaving loop private check

[Marek Lindner]

On Wednesday, February 27, 2013 18:27:51 Antonio Quartulli wrote:
> On Wed, Feb 27, 2013 at 06:21:18PM +0800, Marek Lindner wrote:
> > On Wednesday, February 27, 2013 18:09:48 Marek Lindner wrote:
> > > On Saturday, February 16, 2013 21:42:40 Antonio Quartulli wrote:
> > > > Now that the new dev_upper API is used, the private check for
> > > > enslaving loops is not needed anymore as this is performed by the
> > > > new API itself.
> > > 
> > > Are you sure ? Isn't the following supposed to fail:
> > > 
> > > $ batctl if add eth0
> > > $ vconfig add bat0 5
> > > $ batctl if add bat0.5
> > > 
> > > $ batctl if
> > > 
> > >   eth0: active
> > >   bat0.5: active
> > 
> > Without your patch this is what happens:
> > 
> > $ batctl if add eth0
> > $ vconfig add bat0 5
> > $ batctl if add bat0.5
> > 
> >   Error - interface type not supported by batman-adv: bat0.5
> 
> Are you testing this with < linux-3.9/net-next ?
> We need some compat stuff as if we want to remove this check and keep the
> behaviour on old kernels.

I am not running net-next. This is on linux 3.3.8.

Cheers,
Marek

Re: [B.A.T.M.A.N.] [PATCHv3 1/2] batman-adv: free an hard-interface before adding it

[Marek Lindner]

On Saturday, February 16, 2013 21:42:39 Antonio Quartulli wrote:
> When adding a new hard interface (e.h. wlan0) to a soft interface (e.g.
> bat0) and the former is already enslaved in another virtual interface
> (e.g. a software bridge) batman-adv has to free the it first and then
> continue with the adding mechanism.
> 
> In this way the behaviour becomes consistent with what "ip link set master"
> does. At the moment batman-adv enslaves the hard interface without checking
> for the master device, possibly causing strange behaviours which are never
> wanted by the users.
> 
> Reported-by: Marek Lindner <lindner_marek@yahoo.de>
> Signed-off-by: Antonio Quartulli <ordex@autistici.org>
> ---
> v2:
>  - added compat code for netdev_master_upper_dev_get()
> v3:
>  - use #define instead of a static inline function in compat code
> 
>  compat.h         |  5 +++++
>  hard-interface.c | 14 ++++++++------
>  2 files changed, 13 insertions(+), 6 deletions(-)

Applied in revision 5e57e3b.

Thanks,
Marek