[B.A.T.M.A.N.] Regression in VLAN sysfs patches

[B.A.T.M.A.N.] Regression in VLAN sysfs patches

[Simon Wunderlich]

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

Hello,

it seems there is an regression in the VLAN sysfs patches. I've tracked
the issue a little bit, it seems to be somewhere between:

6f27447 Antonio Quartulli  batman-adv: fix vlan compat code (broken)

... (few more patches i can't test because vlan compat is broken)

cc14598 Linus Lüssing  batman-adv: (style) fix for switched vid-ifiindex parameter order (works)

The regression is probably in some of Antonios VLAN/sysfs patches.

It can be triggered by calling:

ifconfig bat0 down ; ifconfig bat0 up

Which will lead to a WARNING [1]. This can be reproduced every time
I call it. Note that I've not explicitly configured a VLAN on bat0,
this is automatically created/added. I'm using a fairly old OpenWRT
revision (29637) on kernel 2.6.39.4 here in virtual machines.

Cheers,
	Simon

[1]

[   39.317111] 8021q: adding VLAN 0 to HW filter on device bat0
[   39.322208] ------------[ cut here ]------------
[   39.324213] WARNING: at fs/sysfs/dir.c:455 sysfs_add_one+0x8b/0xb0()
[   39.325378] sysfs: cannot create duplicate filename '/devices/virtual/net/bat0/mesh/vlan0'
[   39.326955] Modules linked in: virtio_net batman_adv ebt_snat ebt_dnat ebt_arpreply ebt_ip ebt_arp ebt_redirect ebt_mark ebt_vlan ebt_stp ebt_pkttype ebt_mark_m ebt_limit ebt_among ebt_802_3 ebtable_nat ebtable_filter ebtable_broute nf_nat_irc nf_conntrack_irc nf_nat_ftp nf_conntrack_ftp xt_string xt_layer7 ipt_MASQUERADE iptable_nat nf_nat xt_CT xt_conntrack xt_NOTRACK iptable_raw xt_state nf_conntrack_ipv4 nf_defrag_ipv4 nf_conntrack ipt_REJECT xt_TCPMSS ipt_LOG xt_comment xt_multiport xt_mac xt_limit iptable_mangle iptable_filter ts_fsm ts_bm ts_kmp libcrc32c ipv6 virtio_rng crc32c crypto_hash crypto_algapi
[   39.340649] Pid: 1166, comm: ifconfig Not tainted 2.6.39.4 #180
[   39.342140] Call Trace:
[   39.342686]  [<c10d557b>] ? sysfs_add_one+0x8b/0xb0
[   39.343899]  [<c10284af>] warn_slowpath_common+0x5f/0x80
[   39.345480]  [<c10d557b>] ? sysfs_add_one+0x8b/0xb0
[   39.347153]  [<c102854e>] warn_slowpath_fmt+0x2e/0x30
[   39.348504]  [<c10d557b>] sysfs_add_one+0x8b/0xb0
[   39.349761]  [<c10d55fb>] create_dir+0x5b/0x90
[   39.351197]  [<c10d56ee>] sysfs_create_dir+0x8e/0xa0
[   39.352558]  [<c11386c5>] kobject_add_internal+0xc5/0x1d0
[   39.353945]  [<c1138ad5>] kobject_add+0x75/0x90
[   39.355177]  [<c1138c86>] kobject_create_and_add+0x36/0x70
[   39.356550]  [<c4b8293e>] batadv_sysfs_add_vlan+0x62/0x216 [batman_adv]
[   39.358314]  [<c108ce09>] ? __kmalloc+0x119/0x130
[   39.359554]  [<c4b8051d>] batadv_softif_create_vlan+0xb6/0xa25 [batman_adv]
[   39.361571]  [<c4b806b9>] batadv_softif_create_vlan+0x252/0xa25 [batman_adv]
[   39.363355]  [<c4b80672>] batadv_softif_create_vlan+0x20b/0xa25 [batman_adv]
[   39.365227]  [<c127f925>] vlan_device_event+0xc5/0x510
[   39.366578]  [<c126f39c>] ? packet_notifier+0x19c/0x1b0
[   39.368010]  [<c126f200>] ? packet_dev_mc+0xd0/0xd0
[   39.369239]  [<c10425a0>] notifier_call_chain+0x30/0x60
[   39.370600]  [<c104266a>] raw_notifier_call_chain+0x1a/0x20
[   39.372089]  [<c120d04e>] call_netdevice_notifiers+0x4e/0x60
[   39.373468]  [<c121b855>] ? rtmsg_ifinfo+0xb5/0xe0
[   39.374713]  [<c121055e>] __dev_notify_flags+0x2e/0x70
[   39.376240]  [<c12105e3>] dev_change_flags+0x43/0x60
[   39.377803]  [<c1258665>] devinet_ioctl+0x285/0x670
[   39.378985]  [<c1210ec5>] ? dev_ioctl+0x665/0x6c0
[   39.379871]  [<c1210be5>] ? dev_ioctl+0x385/0x6c0
[   39.380797]  [<c12597f2>] inet_ioctl+0x72/0xa0
[   39.381689]  [<c11fea88>] sock_ioctl+0x218/0x250
[   39.382603]  [<c11fe870>] ? sock_fasync+0x80/0x80
[   39.383550]  [<c109e09e>] do_vfs_ioctl+0x4ee/0x530
[   39.384488]  [<c108df28>] ? fd_install+0x48/0x60
[   39.385606]  [<c11ff738>] ? sys_socket+0x48/0x70
[   39.386665]  [<c120054a>] ? sys_socketcall+0x6a/0x270
[   39.387666]  [<c109e119>] sys_ioctl+0x39/0x60
[   39.388541]  [<c128d325>] syscall_call+0x7/0xb
[   39.389417] ---[ end trace c59b34f09c69e69f ]---
[   39.390323] kobject_add_internal failed for vlan0 with -EEXIST, don't try to register things with the same name in the same directory.
[   39.392757] Pid: 1166, comm: ifconfig Tainted: G        W   2.6.39.4 #180
[   39.394234] Call Trace:
[   39.395176]  [<c128a7fc>] ? printk+0x18/0x1c
[   39.396080]  [<c11387a8>] kobject_add_internal+0x1a8/0x1d0
[   39.397123]  [<c1138ad5>] kobject_add+0x75/0x90
[   39.398037]  [<c1138c86>] kobject_create_and_add+0x36/0x70
[   39.399241]  [<c4b8293e>] batadv_sysfs_add_vlan+0x62/0x216 [batman_adv]
[   39.400455]  [<c108ce09>] ? __kmalloc+0x119/0x130
[   39.401382]  [<c4b8051d>] batadv_softif_create_vlan+0xb6/0xa25 [batman_adv]
[   39.402634]  [<c4b806b9>] batadv_softif_create_vlan+0x252/0xa25 [batman_adv]
[   39.404345]  [<c4b80672>] batadv_softif_create_vlan+0x20b/0xa25 [batman_adv]
[   39.405675]  [<c127f925>] vlan_device_event+0xc5/0x510
[   39.406671]  [<c126f39c>] ? packet_notifier+0x19c/0x1b0
[   39.407821]  [<c126f200>] ? packet_dev_mc+0xd0/0xd0
[   39.408836]  [<c10425a0>] notifier_call_chain+0x30/0x60
[   39.410132]  [<c104266a>] raw_notifier_call_chain+0x1a/0x20
[   39.411492]  [<c120d04e>] call_netdevice_notifiers+0x4e/0x60
[   39.412554]  [<c121b855>] ? rtmsg_ifinfo+0xb5/0xe0
[   39.413485]  [<c121055e>] __dev_notify_flags+0x2e/0x70
[   39.414473]  [<c12105e3>] dev_change_flags+0x43/0x60
[   39.415752]  [<c1258665>] devinet_ioctl+0x285/0x670
[   39.416716]  [<c1210ec5>] ? dev_ioctl+0x665/0x6c0
[   39.417651]  [<c1210be5>] ? dev_ioctl+0x385/0x6c0
[   39.418691]  [<c12597f2>] inet_ioctl+0x72/0xa0
[   39.419606]  [<c11fea88>] sock_ioctl+0x218/0x250
[   39.420642]  [<c11fe870>] ? sock_fasync+0x80/0x80
[   39.421544]  [<c109e09e>] do_vfs_ioctl+0x4ee/0x530
[   39.422456]  [<c108df28>] ? fd_install+0x48/0x60
[   39.423348]  [<c11ff738>] ? sys_socket+0x48/0x70
[   39.424641]  [<c120054a>] ? sys_socketcall+0x6a/0x270
[   39.426055]  [<c109e119>] sys_ioctl+0x39/0x60
[   39.426982]  [<c128d325>] syscall_call+0x7/0xb
[   39.427944] kobject_create_and_add: kobject_add error: -17
[   39.428999] batman_adv: bat0: Can't add sysfs directory: bat0/vlan0


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

[B.A.T.M.A.N.] [PATCH] batman-adv: print some info for vlan debugging

[Antonio Quartulli]

This patch will print some more information when doing
ifconfig bat0 up & down

please report the output.

Signed-off-by: Antonio Quartulli <ordex@autistici.org>
---
 soft-interface.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/soft-interface.c b/soft-interface.c
index d018c49..7876476 100644
--- a/soft-interface.c
+++ b/soft-interface.c
@@ -525,6 +525,9 @@ static int batadv_interface_add_vid(struct net_device *dev, __be16 proto,
 
 	vid |= BATADV_VLAN_HAS_TAG;
 
+	printk("batadv_add_vid(): creating vlan %s %d\n", dev->name,
+	       BATADV_PRINT_VID(vid));
+
 	return batadv_softif_create_vlan(bat_priv, vid);
 }
 
@@ -551,9 +554,14 @@ static int batadv_interface_kill_vid(struct net_device *dev, __be16 proto,
 	if (proto != htons(ETH_P_8021Q))
 		return -EINVAL;
 
+	printk("batadv_kill_vid(): %s %d\n", dev->name,
+		BATADV_PRINT_VID(vid | BATADV_VLAN_HAS_TAG));
+
 	vlan = batadv_softif_vlan_get(bat_priv, vid | BATADV_VLAN_HAS_TAG);
-	if (!vlan)
+	if (!vlan) {
+		printk("batadv_kill_vid(): couldn't find vlan to kill\n");
 		return -ENOENT;
+	}
 
 	batadv_softif_destroy_vlan(bat_priv, vlan);
 
-- 
1.8.1.5

[B.A.T.M.A.N.] [PATCH] batman-adv: check if a softif_vlan already exists

[Antonio Quartulli]

From: Antonio Quartulli <antonio@open-mesh.com>

Before creating a new softif_vlan it is better to check if
that does already exist.
If so batman-adv should refuse to create a new structure
otherwise this would lead to an inconsistent state.

Normally this is not a problem because the operating system
will prevent from creating the same vlan twice, but some
ancient kernels exhibited an improper behaviour that led to
a bug.

Introduced by 952cebb57518ec18dfdebfcb2b85539f58f20779
("batman-adv: add per VLAN interface attribute framework")

Reported-by: Simon Wunderlich <simon.wunderlich@s2003.tu-chemnitz.de>
Signed-off-by: Antonio Quartulli <antonio@open-mesh.com>
---
 soft-interface.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/soft-interface.c b/soft-interface.c
index d018c49..2459967 100644
--- a/soft-interface.c
+++ b/soft-interface.c
@@ -450,6 +450,10 @@ int batadv_softif_create_vlan(struct batadv_priv *bat_priv, unsigned short vid)
 	struct batadv_softif_vlan *vlan;
 	int err;
 
+	vlan = batadv_softif_vlan_get(bat_priv, vid);
+	if (vlan)
+		return -EEXIST;
+
 	vlan = kzalloc(sizeof(*vlan), GFP_ATOMIC);
 	if (!vlan)
 		return -ENOMEM;
-- 
1.8.1.5

[B.A.T.M.A.N.] [PATCHv2] batman-adv: check if a softif_vlan already exists

[Antonio Quartulli]

From: Antonio Quartulli <antonio@open-mesh.com>

Before creating a new softif_vlan it is better to check if
that does already exist.
If so batman-adv should refuse to create a new structure
otherwise this would lead to an inconsistent state.

Normally this is not a problem because the operating system
will prevent from creating the same vlan twice, but some
ancient kernels exhibited an improper behaviour that led to
a bug.

Introduced by 952cebb57518ec18dfdebfcb2b85539f58f20779
("batman-adv: add per VLAN interface attribute framework")

Reported-by: Simon Wunderlich <simon.wunderlich@s2003.tu-chemnitz.de>
Signed-off-by: Antonio Quartulli <antonio@open-mesh.com>
---

however I don't know if returning -EEXIST will make ifconfig happy..it may be
that we simply have to return 0 and silently ignore the problem..
let's see :)

cheers,

changes from v1:
- fixed refcounting

 soft-interface.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/soft-interface.c b/soft-interface.c
index d018c49..bafc811 100644
--- a/soft-interface.c
+++ b/soft-interface.c
@@ -450,6 +450,12 @@ int batadv_softif_create_vlan(struct batadv_priv *bat_priv, unsigned short vid)
 	struct batadv_softif_vlan *vlan;
 	int err;
 
+	vlan = batadv_softif_vlan_get(bat_priv, vid);
+	if (vlan) {
+		batadv_softif_vlan_free_ref(vlan);
+		return -EEXIST;
+	}
+
 	vlan = kzalloc(sizeof(*vlan), GFP_ATOMIC);
 	if (!vlan)
 		return -ENOMEM;
-- 
1.8.1.5

Re: [B.A.T.M.A.N.] [PATCHv2] batman-adv: check if a softif_vlan already exists

[Simon Wunderlich]

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

Hey Antonio,

Thanks, that solves the problem on my ancient kernel! :)

I don't see a problem with -EEXIST (at least there is no error
whatsoever), so I think we can leave it at that.

Tested-by: Simon Wunderlich <simon.wunderlich@s2003.tu-chemnitz.de>

Just one thing ...

On Thu, Aug 15, 2013 at 11:05:55PM +0200, Antonio Quartulli wrote:
> From: Antonio Quartulli <antonio@open-mesh.com>
> 
> Before creating a new softif_vlan it is better to check if
> that does already exist.
> If so batman-adv should refuse to create a new structure
> otherwise this would lead to an inconsistent state.
> 
> Normally this is not a problem because the operating system
> will prevent from creating the same vlan twice, but some
> ancient kernels exhibited an improper behaviour that led to
> a bug.

You might want to skip that when sending upstream? They might not
care about older kernels. Duno. :)

Thanks!
	Simon

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

Re: [B.A.T.M.A.N.] [PATCHv2] batman-adv: check if a softif_vlan already exists

[Antonio Quartulli]

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

On Mon, Aug 19, 2013 at 10:20:28PM +0200, Simon Wunderlich wrote:
> Hey Antonio,
> 
> Thanks, that solves the problem on my ancient kernel! :)
> 
> I don't see a problem with -EEXIST (at least there is no error
> whatsoever), so I think we can leave it at that.
> 
> Tested-by: Simon Wunderlich <simon.wunderlich@s2003.tu-chemnitz.de>
> 
> Just one thing ...
> 
> On Thu, Aug 15, 2013 at 11:05:55PM +0200, Antonio Quartulli wrote:
> > From: Antonio Quartulli <antonio@open-mesh.com>
> > 
> > Before creating a new softif_vlan it is better to check if
> > that does already exist.
> > If so batman-adv should refuse to create a new structure
> > otherwise this would lead to an inconsistent state.
> > 
> > Normally this is not a problem because the operating system
> > will prevent from creating the same vlan twice, but some
> > ancient kernels exhibited an improper behaviour that led to
> > a bug.
> 
> You might want to skip that when sending upstream? They might not
> care about older kernels. Duno. :)

This patch is only for us, because it is going to be squashed with a previous
one before going to David. So I'd leave the commit message as it is.

Cheers,

> 
> Thanks!
> 	Simon



-- 
Antonio Quartulli

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

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

Re: [B.A.T.M.A.N.] [PATCHv2] batman-adv: check if a softif_vlan already exists

[Marek Lindner]

On Tuesday, August 20, 2013 14:39:06 Antonio Quartulli wrote:
> > Tested-by: Simon Wunderlich <simon.wunderlich@s2003.tu-chemnitz.de>
> >
> > 
> >
> > Just one thing ...
> >
> > 
> >
> > On Thu, Aug 15, 2013 at 11:05:55PM +0200, Antonio Quartulli wrote:
> > > From: Antonio Quartulli <antonio@open-mesh.com>
> > >
> > > 
> > >
> > > Before creating a new softif_vlan it is better to check if
> > > that does already exist.
> > > If so batman-adv should refuse to create a new structure
> > > otherwise this would lead to an inconsistent state.
> > >
> > > 
> > >
> > > Normally this is not a problem because the operating system
> > > will prevent from creating the same vlan twice, but some
> > > ancient kernels exhibited an improper behaviour that led to
> > > a bug.
> >
> > 
> >
> > You might want to skip that when sending upstream? They might not
> > care about older kernels. Duno. :)
> 
> This patch is only for us, because it is going to be squashed with a
> previous one before going to David. So I'd leave the commit message as it
> is.

Applied in revision 00f2151.

Thanks,
Marek