Skip to contentSkip to conversation

Bridge VLAN memory leak

DISCUSSION Vlad Buslov 2 participants 5 messages Jul 4, 2022 – Jul 5, 2022

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

Vlad Buslov

author
#0 lore raw

Hi Ido,

While implementing QinQ offload support in mlx5 I encountered a memory leak[0] in the bridge implementation which seems to be related to the new BR_VLFLAG_ADDED_BY_SWITCHDEV flag that you have recently added.

To reproduce the issue netdevice must support bridge VLAN offload, so I can't provide a simple script that uses veth or anything like that. Instead, I'll describe the issue step-by-step:

1. Create a bridge, add offload-capable netdevs to it and assign some VLAN to them. __vlan_vid_add() function will set the BR_VLFLAG_ADDED_BY_SWITCHDEV flag since br_switchdev_port_vlan_add() should return 0 if dev can offload VLANs and will also skip call to vlan_vid_add() in such case:

        /* Try switchdev op first. In case it is not supported, fallback to
         * 8021q add.
         */
        err = br_switchdev_port_vlan_add(dev, v->vid, flags, false, extack);
        if (err == -EOPNOTSUPP)
                return vlan_vid_add(dev, br->vlan_proto, v->vid);
        v->priv_flags |= BR_VLFLAG_ADDED_BY_SWITCHDEV;

2. Enable filtering and set VLAN protocol to 802.1ad. That will trigger the following code in __br_vlan_set_proto() that re-creates existing VLANs with vlan_vid_add() function call whether they have the BR_VLFLAG_ADDED_BY_SWITCHDEV flag set or not:

         /* Add VLANs for the new proto to the device filter. */
         list_for_each_entry(p, &br->port_list, list) {
                 vg = nbp_vlan_group(p);
                 list_for_each_entry(vlan, &vg->vlan_list, vlist) {
                         err = vlan_vid_add(p->dev, proto, vlan->vid);
                         if (err)
                                 goto err_filt;
                 }
         }

3. Now delete the bridge. That will delete all existing VLANs via __vlan_vid_del() function, which skips calling vlan_vid_del() (that is necessary to clean up after vlan_vid_add()) if VLAN has BR_VLFLAG_ADDED_BY_SWITCHDEV flag set:

         /* Try switchdev op first. In case it is not supported, fallback to
          * 8021q del.
          */
         err = br_switchdev_port_vlan_del(dev, v->vid);
         if (!(v->priv_flags & BR_VLFLAG_ADDED_BY_SWITCHDEV))
                 vlan_vid_del(dev, br->vlan_proto, v->vid);

The issue doesn't reproduce for me anymore if I just clear the BR_VLFLAG_ADDED_BY_SWITCHDEV flag when re-creating VLANs on step 2. However, I'm not sure whether it is the right approach in this case. WDYT?

[0]:

unreferenced object 0xffff8881f6771200 (size 256):
comm "ip", pid 446855, jiffies 4298238841 (age 55.240s) hex dump (first 32 bytes):

    00 00 7f 0e 83 88 ff ff 00 00 00 00 00 00 00 00  ................
    00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
  backtrace:
    [<00000000012819ac>] vlan_vid_add+0x437/0x750
    [<00000000f2281fad>] __br_vlan_set_proto+0x289/0x920
    [<000000000632b56f>] br_changelink+0x3d6/0x13f0
    [<0000000089d25f04>] __rtnl_newlink+0x8ae/0x14c0
    [<00000000f6276baf>] rtnl_newlink+0x5f/0x90
    [<00000000746dc902>] rtnetlink_rcv_msg+0x336/0xa00
    [<000000001c2241c0>] netlink_rcv_skb+0x11d/0x340
    [<0000000010588814>] netlink_unicast+0x438/0x710
    [<00000000e1a4cd5c>] netlink_sendmsg+0x788/0xc40
    [<00000000e8992d4e>] sock_sendmsg+0xb0/0xe0
    [<00000000621b8f91>] ____sys_sendmsg+0x4ff/0x6d0
    [<000000000ea26996>] ___sys_sendmsg+0x12e/0x1b0
    [<00000000684f7e25>] __sys_sendmsg+0xab/0x130
    [<000000004538b104>] do_syscall_64+0x3d/0x90
    [<0000000091ed9678>] entry_SYSCALL_64_after_hwframe+0x46/0xb0

Regards,
Vlad

#1 lore raw
Quoted 5 lines · Vlad Buslov

Hi Ido,

While implementing QinQ offload support in mlx5 I encountered a memory leak[0] in the bridge implementation which seems to be related to the new BR_VLFLAG_ADDED_BY_SWITCHDEV flag that you have recently added.

FTR, added here:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=279737939a8194f02fa352ab4476a1b241f44ef4

Quoted 39 lines

To reproduce the issue netdevice must support bridge VLAN offload, so I can't provide a simple script that uses veth or anything like that. Instead, I'll describe the issue step-by-step:

1. Create a bridge, add offload-capable netdevs to it and assign some VLAN to them. __vlan_vid_add() function will set the BR_VLFLAG_ADDED_BY_SWITCHDEV flag since br_switchdev_port_vlan_add() should return 0 if dev can offload VLANs and will also skip call to vlan_vid_add() in such case:

        /* Try switchdev op first. In case it is not supported, fallback to
         * 8021q add.
         */
        err = br_switchdev_port_vlan_add(dev, v->vid, flags, false, extack);
        if (err == -EOPNOTSUPP)
                return vlan_vid_add(dev, br->vlan_proto, v->vid);
        v->priv_flags |= BR_VLFLAG_ADDED_BY_SWITCHDEV;

2. Enable filtering and set VLAN protocol to 802.1ad. That will trigger the following code in __br_vlan_set_proto() that re-creates existing VLANs with vlan_vid_add() function call whether they have the BR_VLFLAG_ADDED_BY_SWITCHDEV flag set or not:

         /* Add VLANs for the new proto to the device filter. */
         list_for_each_entry(p, &br->port_list, list) {
                 vg = nbp_vlan_group(p);
                 list_for_each_entry(vlan, &vg->vlan_list, vlist) {
                         err = vlan_vid_add(p->dev, proto, vlan->vid);
                         if (err)
                                 goto err_filt;
                 }
         }

3. Now delete the bridge. That will delete all existing VLANs via __vlan_vid_del() function, which skips calling vlan_vid_del() (that is necessary to clean up after vlan_vid_add()) if VLAN has BR_VLFLAG_ADDED_BY_SWITCHDEV flag set:

Quoted 6 lines
         /* Try switchdev op first. In case it is not supported, fallback to
          * 8021q del.
          */
         err = br_switchdev_port_vlan_del(dev, v->vid);
         if (!(v->priv_flags & BR_VLFLAG_ADDED_BY_SWITCHDEV))
                 vlan_vid_del(dev, br->vlan_proto, v->vid);

Looking at the code before the change, I'm pretty sure you will be able to reproduce the leak prior to above mentioned commit:

```
-	err = br_switchdev_port_vlan_del(dev, vid);
-	if (err == -EOPNOTSUPP) {
-		vlan_vid_del(dev, br->vlan_proto, vid);
-		return 0;
-	}
-	return err;
```
Quoted 6 lines

The issue doesn't reproduce for me anymore if I just clear the BR_VLFLAG_ADDED_BY_SWITCHDEV flag when re-creating VLANs on step 2. However, I'm not sure whether it is the right approach in this case. WDYT?

As a switchdev driver you already know about the new VLAN protocol via 'SWITCHDEV_ATTR_ID_BRIDGE_VLAN_PROTOCOL' so do we really need the VLANs to be programmed again? The VLAN protocol is not communicated in 'SWITCHDEV_OBJ_ID_PORT_VLAN' anyway.

Can you try the below (compile tested only)?

```
diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
index 6e53dc991409..9ffd40b8270c 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c
@@ -959,6 +959,8 @@ int __br_vlan_set_proto(struct net_bridge *br, __be16 proto,
 	list_for_each_entry(p, &br->port_list, list) {
 		vg = nbp_vlan_group(p);
 		list_for_each_entry(vlan, &vg->vlan_list, vlist) {
+			if (vlan->priv_flags & BR_VLFLAG_ADDED_BY_SWITCHDEV)
+				continue;
 			err = vlan_vid_add(p->dev, proto, vlan->vid);
 			if (err)
 				goto err_filt;
@@ -973,8 +975,11 @@ int __br_vlan_set_proto(struct net_bridge *br, __be16 proto,
 	/* Delete VLANs for the old proto from the device filter. */
 	list_for_each_entry(p, &br->port_list, list) {
 		vg = nbp_vlan_group(p);
-		list_for_each_entry(vlan, &vg->vlan_list, vlist)
+		list_for_each_entry(vlan, &vg->vlan_list, vlist) {
+			if (vlan->priv_flags & BR_VLFLAG_ADDED_BY_SWITCHDEV)
+				continue;
 			vlan_vid_del(p->dev, oldproto, vlan->vid);
+		}
 	}
 
 	return 0;
@@ -983,13 +988,19 @@ int __br_vlan_set_proto(struct net_bridge *br, __be16 proto,
 	attr.u.vlan_protocol = ntohs(oldproto);
 	switchdev_port_attr_set(br->dev, &attr, NULL);
 
-	list_for_each_entry_continue_reverse(vlan, &vg->vlan_list, vlist)
+	list_for_each_entry_continue_reverse(vlan, &vg->vlan_list, vlist) {
+		if (vlan->priv_flags & BR_VLFLAG_ADDED_BY_SWITCHDEV)
+			continue;
 		vlan_vid_del(p->dev, proto, vlan->vid);
+	}
 
 	list_for_each_entry_continue_reverse(p, &br->port_list, list) {
 		vg = nbp_vlan_group(p);
-		list_for_each_entry(vlan, &vg->vlan_list, vlist)
+		list_for_each_entry(vlan, &vg->vlan_list, vlist) {
+			if (vlan->priv_flags & BR_VLFLAG_ADDED_BY_SWITCHDEV)
+				continue;
 			vlan_vid_del(p->dev, proto, vlan->vid);
+		}
 	}
 
 	return err;
```
Quoted 28 lines

[0]:

unreferenced object 0xffff8881f6771200 (size 256):
comm "ip", pid 446855, jiffies 4298238841 (age 55.240s) hex dump (first 32 bytes):

    00 00 7f 0e 83 88 ff ff 00 00 00 00 00 00 00 00  ................
    00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
  backtrace:
    [<00000000012819ac>] vlan_vid_add+0x437/0x750
    [<00000000f2281fad>] __br_vlan_set_proto+0x289/0x920
    [<000000000632b56f>] br_changelink+0x3d6/0x13f0
    [<0000000089d25f04>] __rtnl_newlink+0x8ae/0x14c0
    [<00000000f6276baf>] rtnl_newlink+0x5f/0x90
    [<00000000746dc902>] rtnetlink_rcv_msg+0x336/0xa00
    [<000000001c2241c0>] netlink_rcv_skb+0x11d/0x340
    [<0000000010588814>] netlink_unicast+0x438/0x710
    [<00000000e1a4cd5c>] netlink_sendmsg+0x788/0xc40
    [<00000000e8992d4e>] sock_sendmsg+0xb0/0xe0
    [<00000000621b8f91>] ____sys_sendmsg+0x4ff/0x6d0
    [<000000000ea26996>] ___sys_sendmsg+0x12e/0x1b0
    [<00000000684f7e25>] __sys_sendmsg+0xab/0x130
    [<000000004538b104>] do_syscall_64+0x3d/0x90
    [<0000000091ed9678>] entry_SYSCALL_64_after_hwframe+0x46/0xb0

Regards,
Vlad

author
#2 lore raw
Quoted 75 lines · Ido Schimmel
Quoted 5 lines · Vlad Buslov

Hi Ido,

While implementing QinQ offload support in mlx5 I encountered a memory leak[0] in the bridge implementation which seems to be related to the new BR_VLFLAG_ADDED_BY_SWITCHDEV flag that you have recently added.

FTR, added here:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=279737939a8194f02fa352ab4476a1b241f44ef4

Quoted 39 lines

To reproduce the issue netdevice must support bridge VLAN offload, so I can't provide a simple script that uses veth or anything like that. Instead, I'll describe the issue step-by-step:

1. Create a bridge, add offload-capable netdevs to it and assign some VLAN to them. __vlan_vid_add() function will set the BR_VLFLAG_ADDED_BY_SWITCHDEV flag since br_switchdev_port_vlan_add() should return 0 if dev can offload VLANs and will also skip call to vlan_vid_add() in such case:

        /* Try switchdev op first. In case it is not supported, fallback to
         * 8021q add.
         */
        err = br_switchdev_port_vlan_add(dev, v->vid, flags, false, extack);
        if (err == -EOPNOTSUPP)
                return vlan_vid_add(dev, br->vlan_proto, v->vid);
        v->priv_flags |= BR_VLFLAG_ADDED_BY_SWITCHDEV;

2. Enable filtering and set VLAN protocol to 802.1ad. That will trigger the following code in __br_vlan_set_proto() that re-creates existing VLANs with vlan_vid_add() function call whether they have the BR_VLFLAG_ADDED_BY_SWITCHDEV flag set or not:

         /* Add VLANs for the new proto to the device filter. */
         list_for_each_entry(p, &br->port_list, list) {
                 vg = nbp_vlan_group(p);
                 list_for_each_entry(vlan, &vg->vlan_list, vlist) {
                         err = vlan_vid_add(p->dev, proto, vlan->vid);
                         if (err)
                                 goto err_filt;
                 }
         }

3. Now delete the bridge. That will delete all existing VLANs via __vlan_vid_del() function, which skips calling vlan_vid_del() (that is necessary to clean up after vlan_vid_add()) if VLAN has BR_VLFLAG_ADDED_BY_SWITCHDEV flag set:

Quoted 6 lines
         /* Try switchdev op first. In case it is not supported, fallback to
          * 8021q del.
          */
         err = br_switchdev_port_vlan_del(dev, v->vid);
         if (!(v->priv_flags & BR_VLFLAG_ADDED_BY_SWITCHDEV))
                 vlan_vid_del(dev, br->vlan_proto, v->vid);

Looking at the code before the change, I'm pretty sure you will be able to reproduce the leak prior to above mentioned commit:

```

(quoted diff) +0 -6
-	err = br_switchdev_port_vlan_del(dev, vid);
-	if (err == -EOPNOTSUPP) {
-		vlan_vid_del(dev, br->vlan_proto, vid);
-		return 0;
-	}
-	return err;

```

Quoted 6 lines

The issue doesn't reproduce for me anymore if I just clear the BR_VLFLAG_ADDED_BY_SWITCHDEV flag when re-creating VLANs on step 2. However, I'm not sure whether it is the right approach in this case. WDYT?

Quoted 4 lines

As a switchdev driver you already know about the new VLAN protocol via 'SWITCHDEV_ATTR_ID_BRIDGE_VLAN_PROTOCOL' so do we really need the VLANs to be programmed again? The VLAN protocol is not communicated in 'SWITCHDEV_OBJ_ID_PORT_VLAN' anyway.

In my WIP implementation of 802.1ad offload I just re-create all existing VLANs in hardware with new protocol upon receiving SWITCHDEV_ATTR_ID_BRIDGE_VLAN_PROTOCOL notification.

Quoted 2 lines

Can you try the below (compile tested only)?

With the patch applied memleak no longer reproduces.

Quoted 80 lines

```

net/bridge/br_vlan.c +14 -3
index 6e53dc991409..9ffd40b8270c 100644
@@ -959,6 +959,8 @@ int __br_vlan_set_proto(struct net_bridge *br, __be16 proto,
 	list_for_each_entry(p, &br->port_list, list) {
 		vg = nbp_vlan_group(p);
 		list_for_each_entry(vlan, &vg->vlan_list, vlist) {
+			if (vlan->priv_flags & BR_VLFLAG_ADDED_BY_SWITCHDEV)
+				continue;
 			err = vlan_vid_add(p->dev, proto, vlan->vid);
 			if (err)
 				goto err_filt;

@@ -973,8 +975,11 @@ int __br_vlan_set_proto(struct net_bridge *br, __be16 proto,
 	/* Delete VLANs for the old proto from the device filter. */
 	list_for_each_entry(p, &br->port_list, list) {
 		vg = nbp_vlan_group(p);
-		list_for_each_entry(vlan, &vg->vlan_list, vlist)
+		list_for_each_entry(vlan, &vg->vlan_list, vlist) {
+			if (vlan->priv_flags & BR_VLFLAG_ADDED_BY_SWITCHDEV)
+				continue;
 			vlan_vid_del(p->dev, oldproto, vlan->vid);
+		}
 	}
 
 	return 0;

@@ -983,13 +988,19 @@ int __br_vlan_set_proto(struct net_bridge *br, __be16 proto,
 	attr.u.vlan_protocol = ntohs(oldproto);
 	switchdev_port_attr_set(br->dev, &attr, NULL);
 
-	list_for_each_entry_continue_reverse(vlan, &vg->vlan_list, vlist)
+	list_for_each_entry_continue_reverse(vlan, &vg->vlan_list, vlist) {
+		if (vlan->priv_flags & BR_VLFLAG_ADDED_BY_SWITCHDEV)
+			continue;
 		vlan_vid_del(p->dev, proto, vlan->vid);
+	}
 
 	list_for_each_entry_continue_reverse(p, &br->port_list, list) {
 		vg = nbp_vlan_group(p);
-		list_for_each_entry(vlan, &vg->vlan_list, vlist)
+		list_for_each_entry(vlan, &vg->vlan_list, vlist) {
+			if (vlan->priv_flags & BR_VLFLAG_ADDED_BY_SWITCHDEV)
+				continue;
 			vlan_vid_del(p->dev, proto, vlan->vid);
+		}
 	}
 
 	return err;

```

Quoted 28 lines

[0]:

unreferenced object 0xffff8881f6771200 (size 256):
comm "ip", pid 446855, jiffies 4298238841 (age 55.240s) hex dump (first 32 bytes):

    00 00 7f 0e 83 88 ff ff 00 00 00 00 00 00 00 00  ................
    00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
  backtrace:
    [<00000000012819ac>] vlan_vid_add+0x437/0x750
    [<00000000f2281fad>] __br_vlan_set_proto+0x289/0x920
    [<000000000632b56f>] br_changelink+0x3d6/0x13f0
    [<0000000089d25f04>] __rtnl_newlink+0x8ae/0x14c0
    [<00000000f6276baf>] rtnl_newlink+0x5f/0x90
    [<00000000746dc902>] rtnetlink_rcv_msg+0x336/0xa00
    [<000000001c2241c0>] netlink_rcv_skb+0x11d/0x340
    [<0000000010588814>] netlink_unicast+0x438/0x710
    [<00000000e1a4cd5c>] netlink_sendmsg+0x788/0xc40
    [<00000000e8992d4e>] sock_sendmsg+0xb0/0xe0
    [<00000000621b8f91>] ____sys_sendmsg+0x4ff/0x6d0
    [<000000000ea26996>] ___sys_sendmsg+0x12e/0x1b0
    [<00000000684f7e25>] __sys_sendmsg+0xab/0x130
    [<000000004538b104>] do_syscall_64+0x3d/0x90
    [<0000000091ed9678>] entry_SYSCALL_64_after_hwframe+0x46/0xb0

Regards,
Vlad

#3 lore raw
Quoted 76 lines · Vlad Buslov
Quoted 75 lines · Ido Schimmel
Quoted 5 lines · Vlad Buslov

Hi Ido,

While implementing QinQ offload support in mlx5 I encountered a memory leak[0] in the bridge implementation which seems to be related to the new BR_VLFLAG_ADDED_BY_SWITCHDEV flag that you have recently added.

FTR, added here:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=279737939a8194f02fa352ab4476a1b241f44ef4

Quoted 39 lines

To reproduce the issue netdevice must support bridge VLAN offload, so I can't provide a simple script that uses veth or anything like that. Instead, I'll describe the issue step-by-step:

1. Create a bridge, add offload-capable netdevs to it and assign some VLAN to them. __vlan_vid_add() function will set the BR_VLFLAG_ADDED_BY_SWITCHDEV flag since br_switchdev_port_vlan_add() should return 0 if dev can offload VLANs and will also skip call to vlan_vid_add() in such case:

        /* Try switchdev op first. In case it is not supported, fallback to
         * 8021q add.
         */
        err = br_switchdev_port_vlan_add(dev, v->vid, flags, false, extack);
        if (err == -EOPNOTSUPP)
                return vlan_vid_add(dev, br->vlan_proto, v->vid);
        v->priv_flags |= BR_VLFLAG_ADDED_BY_SWITCHDEV;

2. Enable filtering and set VLAN protocol to 802.1ad. That will trigger the following code in __br_vlan_set_proto() that re-creates existing VLANs with vlan_vid_add() function call whether they have the BR_VLFLAG_ADDED_BY_SWITCHDEV flag set or not:

         /* Add VLANs for the new proto to the device filter. */
         list_for_each_entry(p, &br->port_list, list) {
                 vg = nbp_vlan_group(p);
                 list_for_each_entry(vlan, &vg->vlan_list, vlist) {
                         err = vlan_vid_add(p->dev, proto, vlan->vid);
                         if (err)
                                 goto err_filt;
                 }
         }

3. Now delete the bridge. That will delete all existing VLANs via __vlan_vid_del() function, which skips calling vlan_vid_del() (that is necessary to clean up after vlan_vid_add()) if VLAN has BR_VLFLAG_ADDED_BY_SWITCHDEV flag set:

Quoted 6 lines
         /* Try switchdev op first. In case it is not supported, fallback to
          * 8021q del.
          */
         err = br_switchdev_port_vlan_del(dev, v->vid);
         if (!(v->priv_flags & BR_VLFLAG_ADDED_BY_SWITCHDEV))
                 vlan_vid_del(dev, br->vlan_proto, v->vid);

Looking at the code before the change, I'm pretty sure you will be able to reproduce the leak prior to above mentioned commit:

```

(quoted diff) +0 -6
-	err = br_switchdev_port_vlan_del(dev, vid);
-	if (err == -EOPNOTSUPP) {
-		vlan_vid_del(dev, br->vlan_proto, vid);
-		return 0;
-	}
-	return err;

```

Quoted 6 lines

The issue doesn't reproduce for me anymore if I just clear the BR_VLFLAG_ADDED_BY_SWITCHDEV flag when re-creating VLANs on step 2. However, I'm not sure whether it is the right approach in this case. WDYT?

Quoted 8 lines
Quoted 4 lines

As a switchdev driver you already know about the new VLAN protocol via 'SWITCHDEV_ATTR_ID_BRIDGE_VLAN_PROTOCOL' so do we really need the VLANs to be programmed again? The VLAN protocol is not communicated in 'SWITCHDEV_OBJ_ID_PORT_VLAN' anyway.

In my WIP implementation of 802.1ad offload I just re-create all existing VLANs in hardware with new protocol upon receiving SWITCHDEV_ATTR_ID_BRIDGE_VLAN_PROTOCOL notification.

Would it be easier for you if you got the VLAN protocol in 'SWITCHDEV_OBJ_ID_PORT_VLAN' and __br_vlan_set_proto() would invoke __vlan_vid_add() and __vlan_vid_del() instead of calling the 8021q driver directly?

Quoted 87 lines
Quoted 3 lines

Can you try the below (compile tested only)?

With the patch applied memleak no longer reproduces.

Quoted 80 lines

```

net/bridge/br_vlan.c +14 -3
index 6e53dc991409..9ffd40b8270c 100644
@@ -959,6 +959,8 @@ int __br_vlan_set_proto(struct net_bridge *br, __be16 proto,
 	list_for_each_entry(p, &br->port_list, list) {
 		vg = nbp_vlan_group(p);
 		list_for_each_entry(vlan, &vg->vlan_list, vlist) {
+			if (vlan->priv_flags & BR_VLFLAG_ADDED_BY_SWITCHDEV)
+				continue;
 			err = vlan_vid_add(p->dev, proto, vlan->vid);
 			if (err)
 				goto err_filt;

@@ -973,8 +975,11 @@ int __br_vlan_set_proto(struct net_bridge *br, __be16 proto,
 	/* Delete VLANs for the old proto from the device filter. */
 	list_for_each_entry(p, &br->port_list, list) {
 		vg = nbp_vlan_group(p);
-		list_for_each_entry(vlan, &vg->vlan_list, vlist)
+		list_for_each_entry(vlan, &vg->vlan_list, vlist) {
+			if (vlan->priv_flags & BR_VLFLAG_ADDED_BY_SWITCHDEV)
+				continue;
 			vlan_vid_del(p->dev, oldproto, vlan->vid);
+		}
 	}
 
 	return 0;

@@ -983,13 +988,19 @@ int __br_vlan_set_proto(struct net_bridge *br, __be16 proto,
 	attr.u.vlan_protocol = ntohs(oldproto);
 	switchdev_port_attr_set(br->dev, &attr, NULL);
 
-	list_for_each_entry_continue_reverse(vlan, &vg->vlan_list, vlist)
+	list_for_each_entry_continue_reverse(vlan, &vg->vlan_list, vlist) {
+		if (vlan->priv_flags & BR_VLFLAG_ADDED_BY_SWITCHDEV)
+			continue;
 		vlan_vid_del(p->dev, proto, vlan->vid);
+	}
 
 	list_for_each_entry_continue_reverse(p, &br->port_list, list) {
 		vg = nbp_vlan_group(p);
-		list_for_each_entry(vlan, &vg->vlan_list, vlist)
+		list_for_each_entry(vlan, &vg->vlan_list, vlist) {
+			if (vlan->priv_flags & BR_VLFLAG_ADDED_BY_SWITCHDEV)
+				continue;
 			vlan_vid_del(p->dev, proto, vlan->vid);
+		}
 	}
 
 	return err;

```

Quoted 28 lines

[0]:

unreferenced object 0xffff8881f6771200 (size 256):
comm "ip", pid 446855, jiffies 4298238841 (age 55.240s) hex dump (first 32 bytes):

    00 00 7f 0e 83 88 ff ff 00 00 00 00 00 00 00 00  ................
    00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
  backtrace:
    [<00000000012819ac>] vlan_vid_add+0x437/0x750
    [<00000000f2281fad>] __br_vlan_set_proto+0x289/0x920
    [<000000000632b56f>] br_changelink+0x3d6/0x13f0
    [<0000000089d25f04>] __rtnl_newlink+0x8ae/0x14c0
    [<00000000f6276baf>] rtnl_newlink+0x5f/0x90
    [<00000000746dc902>] rtnetlink_rcv_msg+0x336/0xa00
    [<000000001c2241c0>] netlink_rcv_skb+0x11d/0x340
    [<0000000010588814>] netlink_unicast+0x438/0x710
    [<00000000e1a4cd5c>] netlink_sendmsg+0x788/0xc40
    [<00000000e8992d4e>] sock_sendmsg+0xb0/0xe0
    [<00000000621b8f91>] ____sys_sendmsg+0x4ff/0x6d0
    [<000000000ea26996>] ___sys_sendmsg+0x12e/0x1b0
    [<00000000684f7e25>] __sys_sendmsg+0xab/0x130
    [<000000004538b104>] do_syscall_64+0x3d/0x90
    [<0000000091ed9678>] entry_SYSCALL_64_after_hwframe+0x46/0xb0

Regards,
Vlad

author
#4 lore raw
Quoted 82 lines · Ido Schimmel
Quoted 81 lines · Vlad Buslov
Quoted 79 lines · Ido Schimmel
Quoted 5 lines · Vlad Buslov

Hi Ido,

While implementing QinQ offload support in mlx5 I encountered a memory leak[0] in the bridge implementation which seems to be related to the new BR_VLFLAG_ADDED_BY_SWITCHDEV flag that you have recently added.

FTR, added here:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=279737939a8194f02fa352ab4476a1b241f44ef4

Quoted 39 lines

To reproduce the issue netdevice must support bridge VLAN offload, so I can't provide a simple script that uses veth or anything like that. Instead, I'll describe the issue step-by-step:

1. Create a bridge, add offload-capable netdevs to it and assign some VLAN to them. __vlan_vid_add() function will set the BR_VLFLAG_ADDED_BY_SWITCHDEV flag since br_switchdev_port_vlan_add() should return 0 if dev can offload VLANs and will also skip call to vlan_vid_add() in such case:

        /* Try switchdev op first. In case it is not supported, fallback to
         * 8021q add.
         */
        err = br_switchdev_port_vlan_add(dev, v->vid, flags, false, extack);
        if (err == -EOPNOTSUPP)
                return vlan_vid_add(dev, br->vlan_proto, v->vid);
        v->priv_flags |= BR_VLFLAG_ADDED_BY_SWITCHDEV;

2. Enable filtering and set VLAN protocol to 802.1ad. That will trigger the following code in __br_vlan_set_proto() that re-creates existing VLANs with vlan_vid_add() function call whether they have the BR_VLFLAG_ADDED_BY_SWITCHDEV flag set or not:

         /* Add VLANs for the new proto to the device filter. */
         list_for_each_entry(p, &br->port_list, list) {
                 vg = nbp_vlan_group(p);
                 list_for_each_entry(vlan, &vg->vlan_list, vlist) {
                         err = vlan_vid_add(p->dev, proto, vlan->vid);
                         if (err)
                                 goto err_filt;
                 }
         }

3. Now delete the bridge. That will delete all existing VLANs via __vlan_vid_del() function, which skips calling vlan_vid_del() (that is necessary to clean up after vlan_vid_add()) if VLAN has BR_VLFLAG_ADDED_BY_SWITCHDEV flag set:

Quoted 6 lines
         /* Try switchdev op first. In case it is not supported, fallback to
          * 8021q del.
          */
         err = br_switchdev_port_vlan_del(dev, v->vid);
         if (!(v->priv_flags & BR_VLFLAG_ADDED_BY_SWITCHDEV))
                 vlan_vid_del(dev, br->vlan_proto, v->vid);

Looking at the code before the change, I'm pretty sure you will be able to reproduce the leak prior to above mentioned commit:

```

(quoted diff) +0 -6
-	err = br_switchdev_port_vlan_del(dev, vid);
-	if (err == -EOPNOTSUPP) {
-		vlan_vid_del(dev, br->vlan_proto, vid);
-		return 0;
-	}
-	return err;

```

Quoted 6 lines

The issue doesn't reproduce for me anymore if I just clear the BR_VLFLAG_ADDED_BY_SWITCHDEV flag when re-creating VLANs on step 2. However, I'm not sure whether it is the right approach in this case. WDYT?

As a switchdev driver you already know about the new VLAN protocol via 'SWITCHDEV_ATTR_ID_BRIDGE_VLAN_PROTOCOL' so do we really need the VLANs to be programmed again? The VLAN protocol is not communicated in 'SWITCHDEV_OBJ_ID_PORT_VLAN' anyway.

Quoted 8 lines
Quoted 3 lines

In my WIP implementation of 802.1ad offload I just re-create all existing VLANs in hardware with new protocol upon receiving SWITCHDEV_ATTR_ID_BRIDGE_VLAN_PROTOCOL notification.

Would it be easier for you if you got the VLAN protocol in 'SWITCHDEV_OBJ_ID_PORT_VLAN' and __br_vlan_set_proto() would invoke __vlan_vid_add() and __vlan_vid_del() instead of calling the 8021q driver directly?

For me it is easy to iterate and re-create existing VLANs with new protocol inside the driver since I already have all the necessary structures for that. Also, with current architecture I pre-create required flow groups based on current bridge VLAN proto (during new bridge creation or on reception of SWITCHDEV_ATTR_ID_BRIDGE_VLAN_PROTOCOL event), so having protocol inside switchdev_obj_port_vlan will be redundant.

Quoted 88 lines
Quoted 88 lines
Quoted 4 lines

Can you try the below (compile tested only)?

With the patch applied memleak no longer reproduces.

Quoted 80 lines

```

net/bridge/br_vlan.c +14 -3
index 6e53dc991409..9ffd40b8270c 100644
@@ -959,6 +959,8 @@ int __br_vlan_set_proto(struct net_bridge *br, __be16 proto,
 	list_for_each_entry(p, &br->port_list, list) {
 		vg = nbp_vlan_group(p);
 		list_for_each_entry(vlan, &vg->vlan_list, vlist) {
+			if (vlan->priv_flags & BR_VLFLAG_ADDED_BY_SWITCHDEV)
+				continue;
 			err = vlan_vid_add(p->dev, proto, vlan->vid);
 			if (err)
 				goto err_filt;

@@ -973,8 +975,11 @@ int __br_vlan_set_proto(struct net_bridge *br, __be16 proto,
 	/* Delete VLANs for the old proto from the device filter. */
 	list_for_each_entry(p, &br->port_list, list) {
 		vg = nbp_vlan_group(p);
-		list_for_each_entry(vlan, &vg->vlan_list, vlist)
+		list_for_each_entry(vlan, &vg->vlan_list, vlist) {
+			if (vlan->priv_flags & BR_VLFLAG_ADDED_BY_SWITCHDEV)
+				continue;
 			vlan_vid_del(p->dev, oldproto, vlan->vid);
+		}
 	}
 
 	return 0;

@@ -983,13 +988,19 @@ int __br_vlan_set_proto(struct net_bridge *br, __be16 proto,
 	attr.u.vlan_protocol = ntohs(oldproto);
 	switchdev_port_attr_set(br->dev, &attr, NULL);
 
-	list_for_each_entry_continue_reverse(vlan, &vg->vlan_list, vlist)
+	list_for_each_entry_continue_reverse(vlan, &vg->vlan_list, vlist) {
+		if (vlan->priv_flags & BR_VLFLAG_ADDED_BY_SWITCHDEV)
+			continue;
 		vlan_vid_del(p->dev, proto, vlan->vid);
+	}
 
 	list_for_each_entry_continue_reverse(p, &br->port_list, list) {
 		vg = nbp_vlan_group(p);
-		list_for_each_entry(vlan, &vg->vlan_list, vlist)
+		list_for_each_entry(vlan, &vg->vlan_list, vlist) {
+			if (vlan->priv_flags & BR_VLFLAG_ADDED_BY_SWITCHDEV)
+				continue;
 			vlan_vid_del(p->dev, proto, vlan->vid);
+		}
 	}
 
 	return err;

```

Quoted 28 lines

[0]:

unreferenced object 0xffff8881f6771200 (size 256):
comm "ip", pid 446855, jiffies 4298238841 (age 55.240s) hex dump (first 32 bytes):

    00 00 7f 0e 83 88 ff ff 00 00 00 00 00 00 00 00  ................
    00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
  backtrace:
    [<00000000012819ac>] vlan_vid_add+0x437/0x750
    [<00000000f2281fad>] __br_vlan_set_proto+0x289/0x920
    [<000000000632b56f>] br_changelink+0x3d6/0x13f0
    [<0000000089d25f04>] __rtnl_newlink+0x8ae/0x14c0
    [<00000000f6276baf>] rtnl_newlink+0x5f/0x90
    [<00000000746dc902>] rtnetlink_rcv_msg+0x336/0xa00
    [<000000001c2241c0>] netlink_rcv_skb+0x11d/0x340
    [<0000000010588814>] netlink_unicast+0x438/0x710
    [<00000000e1a4cd5c>] netlink_sendmsg+0x788/0xc40
    [<00000000e8992d4e>] sock_sendmsg+0xb0/0xe0
    [<00000000621b8f91>] ____sys_sendmsg+0x4ff/0x6d0
    [<000000000ea26996>] ___sys_sendmsg+0x12e/0x1b0
    [<00000000684f7e25>] __sys_sendmsg+0xab/0x130
    [<000000004538b104>] do_syscall_64+0x3d/0x90
    [<0000000091ed9678>] entry_SYSCALL_64_after_hwframe+0x46/0xb0

Regards,
Vlad