[PATCH net 0/3] bridge: Fix undesirable behavior related to vlan

[PATCH net 0/3] bridge: Fix undesirable behavior related to vlan

[Toshiaki Makita]

There seem to be some problems around vlan code.

- Unexpectedly filter vlan by NIC.
- Not correctly decrement refcount of vlan_vid_info.
- Memory leak when deleting a bridge with vlan_info allocated.

This is a patch series to fix these issues.

Toshiaki Makita (3):
  bridge: Use vlan_vid_[add/del] instead of direct
    ndo_vlan_rx_[add/kill]_vid calls
  bridge: Call vlan_vid_del for all vids at nbp_vlan_flush
  bridge: Fix memory leak when deleting bridge with vlan filtering
    enabled

 net/bridge/br_if.c   |  1 +
 net/bridge/br_vlan.c | 24 ++++++++++--------------
 2 files changed, 11 insertions(+), 14 deletions(-)

-- 
1.8.1.2

[PATCH net 1/3] bridge: Use vlan_vid_[add/del] instead of direct ndo_vlan_rx_[add/kill]_vid calls

[Toshiaki Makita]

We should use wrapper functions vlan_vid_[add/del] instead of
ndo_vlan_rx_[add/kill]_vid. Otherwise, we might be not able to communicate
using vlan interface in a certain situation.

Example of problematic case:
  vconfig add eth0 10
  brctl addif br0 eth0
  bridge vlan add dev eth0 vid 10
  bridge vlan del dev eth0 vid 10
  brctl delif br0 eth0
In this case, we cannot communicate via eth0.10 because vlan 10 is
filtered by NIC that has the vlan filtering feature.

Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
---
 net/bridge/br_vlan.c | 20 ++++++--------------
 1 file changed, 6 insertions(+), 14 deletions(-)

diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
index 53f0990..57074be 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c
@@ -34,7 +34,6 @@ static void __vlan_add_flags(struct net_port_vlans *v, u16 vid, u16 flags)
 
 static int __vlan_add(struct net_port_vlans *v, u16 vid, u16 flags)
 {
-	const struct net_device_ops *ops;
 	struct net_bridge_port *p = NULL;
 	struct net_bridge *br;
 	struct net_device *dev;
@@ -53,17 +52,15 @@ static int __vlan_add(struct net_port_vlans *v, u16 vid, u16 flags)
 		br = v->parent.br;
 		dev = br->dev;
 	}
-	ops = dev->netdev_ops;
 
-	if (p && (dev->features & NETIF_F_HW_VLAN_CTAG_FILTER)) {
+	if (p) {
 		/* Add VLAN to the device filter if it is supported.
 		 * Stricly speaking, this is not necessary now, since
 		 * devices are made promiscuous by the bridge, but if
 		 * that ever changes this code will allow tagged
 		 * traffic to enter the bridge.
 		 */
-		err = ops->ndo_vlan_rx_add_vid(dev, htons(ETH_P_8021Q),
-					       vid);
+		err = vlan_vid_add(dev, htons(ETH_P_8021Q), vid);
 		if (err)
 			return err;
 	}
@@ -82,8 +79,8 @@ static int __vlan_add(struct net_port_vlans *v, u16 vid, u16 flags)
 	return 0;
 
 out_filt:
-	if (p && (dev->features & NETIF_F_HW_VLAN_CTAG_FILTER))
-		ops->ndo_vlan_rx_kill_vid(dev, htons(ETH_P_8021Q), vid);
+	if (p)
+		vlan_vid_del(dev, htons(ETH_P_8021Q), vid);
 	return err;
 }
 
@@ -95,13 +92,8 @@ static int __vlan_del(struct net_port_vlans *v, u16 vid)
 	__vlan_delete_pvid(v, vid);
 	clear_bit(vid, v->untagged_bitmap);
 
-	if (v->port_idx) {
-		struct net_device *dev = v->parent.port->dev;
-		const struct net_device_ops *ops = dev->netdev_ops;
-
-		if (dev->features & NETIF_F_HW_VLAN_CTAG_FILTER)
-			ops->ndo_vlan_rx_kill_vid(dev, htons(ETH_P_8021Q), vid);
-	}
+	if (v->port_idx)
+		vlan_vid_del(v->parent.port->dev, htons(ETH_P_8021Q), vid);
 
 	clear_bit(vid, v->vlan_bitmap);
 	v->num_vlans--;
-- 
1.8.1.2

[PATCH net 2/3] bridge: Call vlan_vid_del for all vids at nbp_vlan_flush

[Toshiaki Makita]

We should call vlan_vid_del for all vids at nbp_vlan_flush to prevent
vid_info->refcount from being leaked when detaching a bridge port.

Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
---
Without patch 1/3, refcount leak will occurs on slave devices if the
bridge port is a bonding device.

 net/bridge/br_vlan.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
index 57074be..af5ebd1 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c
@@ -390,6 +390,7 @@ int nbp_vlan_delete(struct net_bridge_port *port, u16 vid)
 void nbp_vlan_flush(struct net_bridge_port *port)
 {
 	struct net_port_vlans *pv;
+	u16 vid;
 
 	ASSERT_RTNL();
 
@@ -397,6 +398,9 @@ void nbp_vlan_flush(struct net_bridge_port *port)
 	if (!pv)
 		return;
 
+	for_each_set_bit(vid, pv->vlan_bitmap, VLAN_N_VID)
+		vlan_vid_del(port->dev, htons(ETH_P_8021Q), vid);
+
 	__vlan_flush(pv);
 }
 
-- 
1.8.1.2

[PATCH net 3/3] bridge: Fix memory leak when deleting bridge with vlan filtering enabled

[Toshiaki Makita]

We currently don't call br_vlan_flush() when deleting a bridge, which
leads to memory leak if br->vlan_info is allocated.

Steps to reproduce:
  while :
  do
    brctl addbr br0
    bridge vlan add dev br0 vid 10 self
    brctl delbr br0
  done
We can observe the cache size of corresponding slab entry
(as kmalloc-2048 in SLUB) is increased.

kmemleak output:
unreferenced object 0xffff8800b68a7000 (size 2048):
  comm "bridge", pid 2086, jiffies 4295774704 (age 47.656s)
  hex dump (first 32 bytes):
    00 00 00 00 00 00 00 00 00 48 9b 36 00 88 ff ff  .........H.6....
    00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
  backtrace:
    [<ffffffff815eb6ae>] kmemleak_alloc+0x4e/0xb0
    [<ffffffff8116a1ca>] kmem_cache_alloc_trace+0xca/0x220
    [<ffffffffa03eddd6>] br_vlan_add+0x66/0xe0 [bridge]
    [<ffffffffa03e543c>] br_setlink+0x2dc/0x340 [bridge]
    [<ffffffff8150e481>] rtnl_bridge_setlink+0x101/0x200
    [<ffffffff8150d9d9>] rtnetlink_rcv_msg+0x99/0x260
    [<ffffffff81528679>] netlink_rcv_skb+0xa9/0xc0
    [<ffffffff8150d938>] rtnetlink_rcv+0x28/0x30
    [<ffffffff81527ccd>] netlink_unicast+0xdd/0x190
    [<ffffffff8152807f>] netlink_sendmsg+0x2ff/0x740
    [<ffffffff814e8368>] sock_sendmsg+0x88/0xc0
    [<ffffffff814e8ac8>] ___sys_sendmsg.part.14+0x298/0x2b0
    [<ffffffff814e91de>] __sys_sendmsg+0x4e/0x90
    [<ffffffff814e922e>] SyS_sendmsg+0xe/0x10
    [<ffffffff81601669>] system_call_fastpath+0x16/0x1b
    [<ffffffffffffffff>] 0xffffffffffffffff

Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
---
 net/bridge/br_if.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index c41d5fb..6e6194f 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c
@@ -172,6 +172,7 @@ void br_dev_delete(struct net_device *dev, struct list_head *head)
 		del_nbp(p);
 	}
 
+	br_vlan_flush(br);
 	del_timer_sync(&br->gc_timer);
 
 	br_sysfs_delbr(br->dev);
-- 
1.8.1.2

Re: [PATCH net 0/3] bridge: Fix undesirable behavior related to vlan

[Vlad Yasevich]

On 11/13/2013 03:26 AM, Toshiaki Makita wrote:
> There seem to be some problems around vlan code.
>
> - Unexpectedly filter vlan by NIC.
> - Not correctly decrement refcount of vlan_vid_info.
> - Memory leak when deleting a bridge with vlan_info allocated.
>
> This is a patch series to fix these issues.
>
> Toshiaki Makita (3):
>    bridge: Use vlan_vid_[add/del] instead of direct
>      ndo_vlan_rx_[add/kill]_vid calls
>    bridge: Call vlan_vid_del for all vids at nbp_vlan_flush
>    bridge: Fix memory leak when deleting bridge with vlan filtering
>      enabled
>

These all look good to me.  Thanks you.

Acked-by: Vlad Yasevich <vyasevic@redhat.com>

-vlad

>   net/bridge/br_if.c   |  1 +
>   net/bridge/br_vlan.c | 24 ++++++++++--------------
>   2 files changed, 11 insertions(+), 14 deletions(-)
>

Re: [PATCH net 0/3] bridge: Fix undesirable behavior related to vlan

[David Miller]

From: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
Date: Wed, 13 Nov 2013 17:26:11 +0900

> There seem to be some problems around vlan code.
> 
> - Unexpectedly filter vlan by NIC.
> - Not correctly decrement refcount of vlan_vid_info.
> - Memory leak when deleting a bridge with vlan_info allocated.
> 
> This is a patch series to fix these issues.

Series applied, thanks.