| Message ID | 20220708151153.1813012-1-edumazet@google.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 72a0b329114b1caa8e69dfa7cdad1dd3c69b8602 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] vlan: fix memory leak in vlan_newlink() | expand |
On Fri, Jul 8, 2022 at 11:11 AM Eric Dumazet <edumazet@google.com> wrote: > > Blamed commit added back a bug I fixed in commit 9bbd917e0bec > ("vlan: fix memory leak in vlan_dev_set_egress_priority") > > If a memory allocation fails in vlan_changelink() after other allocations > succeeded, we need to call vlan_dev_free_egress_priority() > to free all allocated memory because after a failed ->newlink() > we do not call any methods like ndo_uninit() or dev->priv_destructor(). > > In following example, if the allocation for last element 2000:2001 fails, > we need to free eight prior allocations: > > ip link add link dummy0 dummy0.100 type vlan id 100 \ > egress-qos-map 1:2 2:3 3:4 4:5 5:6 6:7 7:8 8:9 2000:2001 > > syzbot report was: > > BUG: memory leak > unreferenced object 0xffff888117bd1060 (size 32): > comm "syz-executor408", pid 3759, jiffies 4294956555 (age 34.090s) > hex dump (first 32 bytes): > 09 00 00 00 00 a0 00 00 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: > [<ffffffff83fc60ad>] kmalloc include/linux/slab.h:600 [inline] > [<ffffffff83fc60ad>] vlan_dev_set_egress_priority+0xed/0x170 net/8021q/vlan_dev.c:193 > [<ffffffff83fc6628>] vlan_changelink+0x178/0x1d0 net/8021q/vlan_netlink.c:128 > [<ffffffff83fc67c8>] vlan_newlink+0x148/0x260 net/8021q/vlan_netlink.c:185 > [<ffffffff838b1278>] rtnl_newlink_create net/core/rtnetlink.c:3363 [inline] > [<ffffffff838b1278>] __rtnl_newlink+0xa58/0xdc0 net/core/rtnetlink.c:3580 > [<ffffffff838b1629>] rtnl_newlink+0x49/0x70 net/core/rtnetlink.c:3593 > [<ffffffff838ac66c>] rtnetlink_rcv_msg+0x21c/0x5c0 net/core/rtnetlink.c:6089 > [<ffffffff839f9c37>] netlink_rcv_skb+0x87/0x1d0 net/netlink/af_netlink.c:2501 > [<ffffffff839f8da7>] netlink_unicast_kernel net/netlink/af_netlink.c:1319 [inline] > [<ffffffff839f8da7>] netlink_unicast+0x397/0x4c0 net/netlink/af_netlink.c:1345 > [<ffffffff839f9266>] netlink_sendmsg+0x396/0x710 net/netlink/af_netlink.c:1921 > [<ffffffff8384dbf6>] sock_sendmsg_nosec net/socket.c:714 [inline] > [<ffffffff8384dbf6>] sock_sendmsg+0x56/0x80 net/socket.c:734 > [<ffffffff8384e15c>] ____sys_sendmsg+0x36c/0x390 net/socket.c:2488 > [<ffffffff838523cb>] ___sys_sendmsg+0x8b/0xd0 net/socket.c:2542 > [<ffffffff838525b8>] __sys_sendmsg net/socket.c:2571 [inline] > [<ffffffff838525b8>] __do_sys_sendmsg net/socket.c:2580 [inline] > [<ffffffff838525b8>] __se_sys_sendmsg net/socket.c:2578 [inline] > [<ffffffff838525b8>] __x64_sys_sendmsg+0x78/0xf0 net/socket.c:2578 > [<ffffffff845ad8d5>] do_syscall_x64 arch/x86/entry/common.c:50 [inline] > [<ffffffff845ad8d5>] do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80 > [<ffffffff8460006a>] entry_SYSCALL_64_after_hwframe+0x46/0xb0 > > Fixes: 37aa50c539bc ("vlan: introduce vlan_dev_free_egress_priority") > Reported-by: syzbot <syzkaller@googlegroups.com> > Signed-off-by: Eric Dumazet <edumazet@google.com> > Cc: Xin Long <lucien.xin@gmail.com> > --- > net/8021q/vlan_netlink.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/net/8021q/vlan_netlink.c b/net/8021q/vlan_netlink.c > index 53b1955b027f89245eb8dd46ede9d7bfd6e553a3..214532173536b790cf032615f73fb3d868d2aae1 100644 > --- a/net/8021q/vlan_netlink.c > +++ b/net/8021q/vlan_netlink.c > @@ -182,10 +182,14 @@ static int vlan_newlink(struct net *src_net, struct net_device *dev, > else if (dev->mtu > max_mtu) > return -EINVAL; > > + /* Note: If this initial vlan_changelink() fails, we need > + * to call vlan_dev_free_egress_priority() to free memory. > + */ > err = vlan_changelink(dev, tb, data, extack); > - if (err) > - return err; > - err = register_vlan_dev(dev, extack); > + > + if (!err) > + err = register_vlan_dev(dev, extack); > + > if (err) > vlan_dev_free_egress_priority(dev); > return err; > -- > 2.37.0.rc0.161.g10f37bed90-goog > Reviewed-by: Xin Long <lucien.xin@gmail.com>
On Fri, Jul 8, 2022 at 11:11 AM Eric Dumazet <edumazet@google.com> wrote: > > Blamed commit added back a bug I fixed in commit 9bbd917e0bec > ("vlan: fix memory leak in vlan_dev_set_egress_priority") > > If a memory allocation fails in vlan_changelink() after other allocations > succeeded, we need to call vlan_dev_free_egress_priority() > to free all allocated memory because after a failed ->newlink() > we do not call any methods like ndo_uninit() or dev->priv_destructor(). > > In following example, if the allocation for last element 2000:2001 fails, > we need to free eight prior allocations: > > ip link add link dummy0 dummy0.100 type vlan id 100 \ > egress-qos-map 1:2 2:3 3:4 4:5 5:6 6:7 7:8 8:9 2000:2001 BTW, it seems that: # ip link change link dummy0 dummy0.100 type vlan id 100 egress-qos-map 8:9 2003:2004 instead of changing qos-map to {8:1 2003:2004}, this cmd can only be able to append the new qos-map "2003:2004". Is this expected? Thanks. > > syzbot report was: > > BUG: memory leak > unreferenced object 0xffff888117bd1060 (size 32): > comm "syz-executor408", pid 3759, jiffies 4294956555 (age 34.090s) > hex dump (first 32 bytes): > 09 00 00 00 00 a0 00 00 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: > [<ffffffff83fc60ad>] kmalloc include/linux/slab.h:600 [inline] > [<ffffffff83fc60ad>] vlan_dev_set_egress_priority+0xed/0x170 net/8021q/vlan_dev.c:193 > [<ffffffff83fc6628>] vlan_changelink+0x178/0x1d0 net/8021q/vlan_netlink.c:128 > [<ffffffff83fc67c8>] vlan_newlink+0x148/0x260 net/8021q/vlan_netlink.c:185 > [<ffffffff838b1278>] rtnl_newlink_create net/core/rtnetlink.c:3363 [inline] > [<ffffffff838b1278>] __rtnl_newlink+0xa58/0xdc0 net/core/rtnetlink.c:3580 > [<ffffffff838b1629>] rtnl_newlink+0x49/0x70 net/core/rtnetlink.c:3593 > [<ffffffff838ac66c>] rtnetlink_rcv_msg+0x21c/0x5c0 net/core/rtnetlink.c:6089 > [<ffffffff839f9c37>] netlink_rcv_skb+0x87/0x1d0 net/netlink/af_netlink.c:2501 > [<ffffffff839f8da7>] netlink_unicast_kernel net/netlink/af_netlink.c:1319 [inline] > [<ffffffff839f8da7>] netlink_unicast+0x397/0x4c0 net/netlink/af_netlink.c:1345 > [<ffffffff839f9266>] netlink_sendmsg+0x396/0x710 net/netlink/af_netlink.c:1921 > [<ffffffff8384dbf6>] sock_sendmsg_nosec net/socket.c:714 [inline] > [<ffffffff8384dbf6>] sock_sendmsg+0x56/0x80 net/socket.c:734 > [<ffffffff8384e15c>] ____sys_sendmsg+0x36c/0x390 net/socket.c:2488 > [<ffffffff838523cb>] ___sys_sendmsg+0x8b/0xd0 net/socket.c:2542 > [<ffffffff838525b8>] __sys_sendmsg net/socket.c:2571 [inline] > [<ffffffff838525b8>] __do_sys_sendmsg net/socket.c:2580 [inline] > [<ffffffff838525b8>] __se_sys_sendmsg net/socket.c:2578 [inline] > [<ffffffff838525b8>] __x64_sys_sendmsg+0x78/0xf0 net/socket.c:2578 > [<ffffffff845ad8d5>] do_syscall_x64 arch/x86/entry/common.c:50 [inline] > [<ffffffff845ad8d5>] do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80 > [<ffffffff8460006a>] entry_SYSCALL_64_after_hwframe+0x46/0xb0 > > Fixes: 37aa50c539bc ("vlan: introduce vlan_dev_free_egress_priority") > Reported-by: syzbot <syzkaller@googlegroups.com> > Signed-off-by: Eric Dumazet <edumazet@google.com> > Cc: Xin Long <lucien.xin@gmail.com> > --- > net/8021q/vlan_netlink.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/net/8021q/vlan_netlink.c b/net/8021q/vlan_netlink.c > index 53b1955b027f89245eb8dd46ede9d7bfd6e553a3..214532173536b790cf032615f73fb3d868d2aae1 100644 > --- a/net/8021q/vlan_netlink.c > +++ b/net/8021q/vlan_netlink.c > @@ -182,10 +182,14 @@ static int vlan_newlink(struct net *src_net, struct net_device *dev, > else if (dev->mtu > max_mtu) > return -EINVAL; > > + /* Note: If this initial vlan_changelink() fails, we need > + * to call vlan_dev_free_egress_priority() to free memory. > + */ > err = vlan_changelink(dev, tb, data, extack); > - if (err) > - return err; > - err = register_vlan_dev(dev, extack); > + > + if (!err) > + err = register_vlan_dev(dev, extack); > + > if (err) > vlan_dev_free_egress_priority(dev); > return err; > -- > 2.37.0.rc0.161.g10f37bed90-goog >
On Sat, Jul 9, 2022 at 3:45 AM Xin Long <lucien.xin@gmail.com> wrote: > > On Fri, Jul 8, 2022 at 11:11 AM Eric Dumazet <edumazet@google.com> wrote: > > > > Blamed commit added back a bug I fixed in commit 9bbd917e0bec > > ("vlan: fix memory leak in vlan_dev_set_egress_priority") > > > > If a memory allocation fails in vlan_changelink() after other allocations > > succeeded, we need to call vlan_dev_free_egress_priority() > > to free all allocated memory because after a failed ->newlink() > > we do not call any methods like ndo_uninit() or dev->priv_destructor(). > > > > In following example, if the allocation for last element 2000:2001 fails, > > we need to free eight prior allocations: > > > > ip link add link dummy0 dummy0.100 type vlan id 100 \ > > egress-qos-map 1:2 2:3 3:4 4:5 5:6 6:7 7:8 8:9 2000:2001 > BTW, it seems that: > > # ip link change link dummy0 dummy0.100 type vlan id 100 > egress-qos-map 8:9 2003:2004 > > instead of changing qos-map to {8:1 2003:2004}, this cmd can only be > able to append the new qos-map "2003:2004". > > Is this expected? > I think this is expected, kernel only supports additions of new items. Otherwise a real RCU protection would be needed. Speaking of (lack of) RCU, it seems we need the following orthogonal fix. diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h index e00c4ee81ff7f82e4343fe45c14d8e5d81d80e95..bbe2c73ca74aeaccb91d2bde71c59c60c53ec515 100644 --- a/include/linux/if_vlan.h +++ b/include/linux/if_vlan.h @@ -202,8 +202,8 @@ vlan_dev_get_egress_qos_mask(struct net_device *dev, u32 skprio) struct vlan_priority_tci_mapping *mp; smp_rmb(); /* coupled with smp_wmb() in vlan_dev_set_egress_priority() */ - - mp = vlan_dev_priv(dev)->egress_priority_map[(skprio & 0xF)]; + /* Paired with WRITE_ONCE() in vlan_dev_set_egress_priority() */ + mp = READ_ONCE(vlan_dev_priv(dev)->egress_priority_map[skprio & 0xF]); while (mp) { if (mp->priority == skprio) { return mp->vlan_qos; /* This should already be shifted diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c index 035812b0461cc4b403f1a80bfdb9cfd9f44e4b45..5cb4d5121a71b012000de14f4e919cd3bf42a44b 100644 --- a/net/8021q/vlan_dev.c +++ b/net/8021q/vlan_dev.c @@ -202,7 +202,8 @@ int vlan_dev_set_egress_priority(const struct net_device *dev, * coupled with smp_rmb() in vlan_dev_get_egress_qos_mask() */ smp_wmb(); - vlan->egress_priority_map[skb_prio & 0xF] = np; + /* Paired with READ_ONCE() in vlan_dev_get_egress_qos_mask() */ + WRITE_ONCE(vlan->egress_priority_map[skb_prio & 0xF], np); if (vlan_qos) vlan->nr_egress_mappings++; return 0;
Hello: This patch was applied to netdev/net.git (master) by David S. Miller <davem@davemloft.net>: On Fri, 8 Jul 2022 15:11:53 +0000 you wrote: > Blamed commit added back a bug I fixed in commit 9bbd917e0bec > ("vlan: fix memory leak in vlan_dev_set_egress_priority") > > If a memory allocation fails in vlan_changelink() after other allocations > succeeded, we need to call vlan_dev_free_egress_priority() > to free all allocated memory because after a failed ->newlink() > we do not call any methods like ndo_uninit() or dev->priv_destructor(). > > [...] Here is the summary with links: - [net] vlan: fix memory leak in vlan_newlink() https://git.kernel.org/netdev/net/c/72a0b329114b You are awesome, thank you!
diff --git a/net/8021q/vlan_netlink.c b/net/8021q/vlan_netlink.c index 53b1955b027f89245eb8dd46ede9d7bfd6e553a3..214532173536b790cf032615f73fb3d868d2aae1 100644 --- a/net/8021q/vlan_netlink.c +++ b/net/8021q/vlan_netlink.c @@ -182,10 +182,14 @@ static int vlan_newlink(struct net *src_net, struct net_device *dev, else if (dev->mtu > max_mtu) return -EINVAL; + /* Note: If this initial vlan_changelink() fails, we need + * to call vlan_dev_free_egress_priority() to free memory. + */ err = vlan_changelink(dev, tb, data, extack); - if (err) - return err; - err = register_vlan_dev(dev, extack); + + if (!err) + err = register_vlan_dev(dev, extack); + if (err) vlan_dev_free_egress_priority(dev); return err;
Blamed commit added back a bug I fixed in commit 9bbd917e0bec ("vlan: fix memory leak in vlan_dev_set_egress_priority") If a memory allocation fails in vlan_changelink() after other allocations succeeded, we need to call vlan_dev_free_egress_priority() to free all allocated memory because after a failed ->newlink() we do not call any methods like ndo_uninit() or dev->priv_destructor(). In following example, if the allocation for last element 2000:2001 fails, we need to free eight prior allocations: ip link add link dummy0 dummy0.100 type vlan id 100 \ egress-qos-map 1:2 2:3 3:4 4:5 5:6 6:7 7:8 8:9 2000:2001 syzbot report was: BUG: memory leak unreferenced object 0xffff888117bd1060 (size 32): comm "syz-executor408", pid 3759, jiffies 4294956555 (age 34.090s) hex dump (first 32 bytes): 09 00 00 00 00 a0 00 00 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: [<ffffffff83fc60ad>] kmalloc include/linux/slab.h:600 [inline] [<ffffffff83fc60ad>] vlan_dev_set_egress_priority+0xed/0x170 net/8021q/vlan_dev.c:193 [<ffffffff83fc6628>] vlan_changelink+0x178/0x1d0 net/8021q/vlan_netlink.c:128 [<ffffffff83fc67c8>] vlan_newlink+0x148/0x260 net/8021q/vlan_netlink.c:185 [<ffffffff838b1278>] rtnl_newlink_create net/core/rtnetlink.c:3363 [inline] [<ffffffff838b1278>] __rtnl_newlink+0xa58/0xdc0 net/core/rtnetlink.c:3580 [<ffffffff838b1629>] rtnl_newlink+0x49/0x70 net/core/rtnetlink.c:3593 [<ffffffff838ac66c>] rtnetlink_rcv_msg+0x21c/0x5c0 net/core/rtnetlink.c:6089 [<ffffffff839f9c37>] netlink_rcv_skb+0x87/0x1d0 net/netlink/af_netlink.c:2501 [<ffffffff839f8da7>] netlink_unicast_kernel net/netlink/af_netlink.c:1319 [inline] [<ffffffff839f8da7>] netlink_unicast+0x397/0x4c0 net/netlink/af_netlink.c:1345 [<ffffffff839f9266>] netlink_sendmsg+0x396/0x710 net/netlink/af_netlink.c:1921 [<ffffffff8384dbf6>] sock_sendmsg_nosec net/socket.c:714 [inline] [<ffffffff8384dbf6>] sock_sendmsg+0x56/0x80 net/socket.c:734 [<ffffffff8384e15c>] ____sys_sendmsg+0x36c/0x390 net/socket.c:2488 [<ffffffff838523cb>] ___sys_sendmsg+0x8b/0xd0 net/socket.c:2542 [<ffffffff838525b8>] __sys_sendmsg net/socket.c:2571 [inline] [<ffffffff838525b8>] __do_sys_sendmsg net/socket.c:2580 [inline] [<ffffffff838525b8>] __se_sys_sendmsg net/socket.c:2578 [inline] [<ffffffff838525b8>] __x64_sys_sendmsg+0x78/0xf0 net/socket.c:2578 [<ffffffff845ad8d5>] do_syscall_x64 arch/x86/entry/common.c:50 [inline] [<ffffffff845ad8d5>] do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80 [<ffffffff8460006a>] entry_SYSCALL_64_after_hwframe+0x46/0xb0 Fixes: 37aa50c539bc ("vlan: introduce vlan_dev_free_egress_priority") Reported-by: syzbot <syzkaller@googlegroups.com> Signed-off-by: Eric Dumazet <edumazet@google.com> Cc: Xin Long <lucien.xin@gmail.com> --- net/8021q/vlan_netlink.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)