* Bridge VLAN memory leak
@ 2022-07-04 15:47 Vlad Buslov
2022-07-05 13:55 ` Ido Schimmel
0 siblings, 1 reply; 5+ messages in thread
From: Vlad Buslov @ 2022-07-04 15:47 UTC (permalink / raw)
To: Ido Schimmel; +Cc: netdev, Maor Dickman, Roopa Prabhu, razor
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Bridge VLAN memory leak
2022-07-04 15:47 Bridge VLAN memory leak Vlad Buslov
@ 2022-07-05 13:55 ` Ido Schimmel
2022-07-05 14:46 ` Vlad Buslov
0 siblings, 1 reply; 5+ messages in thread
From: Ido Schimmel @ 2022-07-05 13:55 UTC (permalink / raw)
To: Vlad Buslov; +Cc: netdev, Maor Dickman, Roopa Prabhu, razor
On Mon, Jul 04, 2022 at 06:47:29PM +0300, Vlad Buslov wrote:
> 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
>
> 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);
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;
```
>
>
> 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;
```
>
> [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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: Bridge VLAN memory leak
2022-07-05 13:55 ` Ido Schimmel
@ 2022-07-05 14:46 ` Vlad Buslov
2022-07-05 15:42 ` Ido Schimmel
0 siblings, 1 reply; 5+ messages in thread
From: Vlad Buslov @ 2022-07-05 14:46 UTC (permalink / raw)
To: Ido Schimmel; +Cc: netdev, Maor Dickman, Roopa Prabhu, razor
On Tue 05 Jul 2022 at 16:55, Ido Schimmel <idosch@nvidia.com> wrote:
> On Mon, Jul 04, 2022 at 06:47:29PM +0300, Vlad Buslov wrote:
>> 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
>
>>
>> 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);
>
> 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;
> ```
>
>>
>>
>> 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.
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.
>
> Can you try the below (compile tested only)?
With the patch applied memleak no longer reproduces.
>
> ```
> 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;
> ```
>
>>
>> [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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Bridge VLAN memory leak
2022-07-05 14:46 ` Vlad Buslov
@ 2022-07-05 15:42 ` Ido Schimmel
2022-07-05 16:25 ` Vlad Buslov
0 siblings, 1 reply; 5+ messages in thread
From: Ido Schimmel @ 2022-07-05 15:42 UTC (permalink / raw)
To: Vlad Buslov; +Cc: netdev, Maor Dickman, Roopa Prabhu, razor
On Tue, Jul 05, 2022 at 05:46:49PM +0300, Vlad Buslov wrote:
> On Tue 05 Jul 2022 at 16:55, Ido Schimmel <idosch@nvidia.com> wrote:
> > On Mon, Jul 04, 2022 at 06:47:29PM +0300, Vlad Buslov wrote:
> >> 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
> >
> >>
> >> 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);
> >
> > 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;
> > ```
> >
> >>
> >>
> >> 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.
>
> 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?
>
> >
> > Can you try the below (compile tested only)?
>
> With the patch applied memleak no longer reproduces.
>
> >
> > ```
> > 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;
> > ```
> >
> >>
> >> [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
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Bridge VLAN memory leak
2022-07-05 15:42 ` Ido Schimmel
@ 2022-07-05 16:25 ` Vlad Buslov
0 siblings, 0 replies; 5+ messages in thread
From: Vlad Buslov @ 2022-07-05 16:25 UTC (permalink / raw)
To: Ido Schimmel; +Cc: netdev, Maor Dickman, Roopa Prabhu, razor
On Tue 05 Jul 2022 at 18:42, Ido Schimmel <idosch@nvidia.com> wrote:
> On Tue, Jul 05, 2022 at 05:46:49PM +0300, Vlad Buslov wrote:
>> On Tue 05 Jul 2022 at 16:55, Ido Schimmel <idosch@nvidia.com> wrote:
>> > On Mon, Jul 04, 2022 at 06:47:29PM +0300, Vlad Buslov wrote:
>> >> 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
>> >
>> >>
>> >> 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);
>> >
>> > 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;
>> > ```
>> >
>> >>
>> >>
>> >> 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.
>>
>> 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.
>
>>
>> >
>> > Can you try the below (compile tested only)?
>>
>> With the patch applied memleak no longer reproduces.
>>
>> >
>> > ```
>> > 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;
>> > ```
>> >
>> >>
>> >> [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
>>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-07-05 16:40 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-07-04 15:47 Bridge VLAN memory leak Vlad Buslov
2022-07-05 13:55 ` Ido Schimmel
2022-07-05 14:46 ` Vlad Buslov
2022-07-05 15:42 ` Ido Schimmel
2022-07-05 16:25 ` Vlad Buslov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).