* [PATCH net] net: fix memory leak in register_netdevice() on error path
@ 2020-11-26 13:23 Yang Yingliang
2020-11-29 13:56 ` Toshiaki Makita
2020-11-30 4:39 ` Stephen Hemminger
0 siblings, 2 replies; 5+ messages in thread
From: Yang Yingliang @ 2020-11-26 13:23 UTC (permalink / raw)
To: netdev; +Cc: davem, kuba, toshiaki.makita1, rkovhaev, yangyingliang
I got a memleak report when doing fault-inject test:
unreferenced object 0xffff88810ace9000 (size 1024):
comm "ip", pid 4622, jiffies 4295457037 (age 43.378s)
hex dump (first 32 bytes):
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 00 00 00 00 00 00 ................
backtrace:
[<00000000008abe41>] __kmalloc+0x10f/0x210
[<000000005d3533a6>] veth_dev_init+0x140/0x310
[<0000000088353c64>] register_netdevice+0x496/0x7a0
[<000000001324d322>] veth_newlink+0x40b/0x960
[<00000000d0799866>] __rtnl_newlink+0xd8c/0x1360
[<00000000d616040a>] rtnl_newlink+0x6b/0xa0
[<00000000e0a1600d>] rtnetlink_rcv_msg+0x3cc/0x9e0
[<000000009eeff98b>] netlink_rcv_skb+0x130/0x3a0
[<00000000500f8be1>] netlink_unicast+0x4da/0x700
[<00000000666c03b3>] netlink_sendmsg+0x7fe/0xcb0
[<0000000073b28103>] sock_sendmsg+0x143/0x180
[<00000000ad746a30>] ____sys_sendmsg+0x677/0x810
[<0000000087dd98e5>] ___sys_sendmsg+0x105/0x180
[<00000000028dd365>] __sys_sendmsg+0xf0/0x1c0
[<00000000a6bfbae6>] do_syscall_64+0x33/0x40
[<00000000e00521b4>] entry_SYSCALL_64_after_hwframe+0x44/0xa9
It seems ifb and loopback may also hit the leak, so I try to fix this in
register_netdevice().
In common case, priv_destructor() will be called in netdev_run_todo()
after calling ndo_uninit() in rollback_registered(), on other error
path in register_netdevice(), ndo_uninit() and priv_destructor() are
called before register_netdevice() return, but in this case,
priv_destructor() will never be called, then it causes memory leak,
so we should call priv_destructor() here.
Reported-by: Hulk Robot <hulkci@huawei.com>
Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
---
net/core/dev.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/net/core/dev.c b/net/core/dev.c
index 82dc6b48e45f..907204395b64 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -10000,6 +10000,17 @@ int register_netdevice(struct net_device *dev)
ret = notifier_to_errno(ret);
if (ret) {
rollback_registered(dev);
+ /*
+ * In common case, priv_destructor() will be
+ * called in netdev_run_todo() after calling
+ * ndo_uninit() in rollback_registered().
+ * But in this case, priv_destructor() will
+ * never be called, then it causes memory
+ * leak, so we should call priv_destructor()
+ * here.
+ */
+ if (dev->priv_destructor)
+ dev->priv_destructor(dev);
rcu_barrier();
dev->reg_state = NETREG_UNREGISTERED;
--
2.25.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH net] net: fix memory leak in register_netdevice() on error path
2020-11-26 13:23 [PATCH net] net: fix memory leak in register_netdevice() on error path Yang Yingliang
@ 2020-11-29 13:56 ` Toshiaki Makita
2020-11-30 11:13 ` Yang Yingliang
2020-11-30 4:39 ` Stephen Hemminger
1 sibling, 1 reply; 5+ messages in thread
From: Toshiaki Makita @ 2020-11-29 13:56 UTC (permalink / raw)
To: Yang Yingliang; +Cc: davem, kuba, rkovhaev, Netdev
On 2020/11/26 22:23, Yang Yingliang wrote:
...
> Reported-by: Hulk Robot <hulkci@huawei.com>
> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
> ---
> net/core/dev.c | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 82dc6b48e45f..907204395b64 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -10000,6 +10000,17 @@ int register_netdevice(struct net_device *dev)
> ret = notifier_to_errno(ret);
> if (ret) {
> rollback_registered(dev);
> + /*
> + * In common case, priv_destructor() will be
As per netdev-faq, the comment style should be
/* foobar blah blah blah
* another line of text
*/
rather than
/*
* foobar blah blah blah
* another line of text
*/
> + * called in netdev_run_todo() after calling
> + * ndo_uninit() in rollback_registered().
> + * But in this case, priv_destructor() will
> + * never be called, then it causes memory
> + * leak, so we should call priv_destructor()
> + * here.
> + */
> + if (dev->priv_destructor)
> + dev->priv_destructor(dev);
To be in line with netdev_run_todo(), I think priv_destructor() should be
called after "dev->reg_state = NETREG_UNREGISTERED".
Toshiaki Makita
> rcu_barrier();
>
> dev->reg_state = NETREG_UNREGISTERED;
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net] net: fix memory leak in register_netdevice() on error path
2020-11-26 13:23 [PATCH net] net: fix memory leak in register_netdevice() on error path Yang Yingliang
2020-11-29 13:56 ` Toshiaki Makita
@ 2020-11-30 4:39 ` Stephen Hemminger
2020-11-30 11:12 ` Yang Yingliang
1 sibling, 1 reply; 5+ messages in thread
From: Stephen Hemminger @ 2020-11-30 4:39 UTC (permalink / raw)
To: Yang Yingliang; +Cc: netdev, davem, kuba, toshiaki.makita1, rkovhaev
On Thu, 26 Nov 2020 21:23:12 +0800
Yang Yingliang <yangyingliang@huawei.com> wrote:
> I got a memleak report when doing fault-inject test:
>
> unreferenced object 0xffff88810ace9000 (size 1024):
> comm "ip", pid 4622, jiffies 4295457037 (age 43.378s)
> hex dump (first 32 bytes):
> 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 00 00 00 00 00 00 ................
> backtrace:
> [<00000000008abe41>] __kmalloc+0x10f/0x210
> [<000000005d3533a6>] veth_dev_init+0x140/0x310
> [<0000000088353c64>] register_netdevice+0x496/0x7a0
> [<000000001324d322>] veth_newlink+0x40b/0x960
> [<00000000d0799866>] __rtnl_newlink+0xd8c/0x1360
> [<00000000d616040a>] rtnl_newlink+0x6b/0xa0
> [<00000000e0a1600d>] rtnetlink_rcv_msg+0x3cc/0x9e0
> [<000000009eeff98b>] netlink_rcv_skb+0x130/0x3a0
> [<00000000500f8be1>] netlink_unicast+0x4da/0x700
> [<00000000666c03b3>] netlink_sendmsg+0x7fe/0xcb0
> [<0000000073b28103>] sock_sendmsg+0x143/0x180
> [<00000000ad746a30>] ____sys_sendmsg+0x677/0x810
> [<0000000087dd98e5>] ___sys_sendmsg+0x105/0x180
> [<00000000028dd365>] __sys_sendmsg+0xf0/0x1c0
> [<00000000a6bfbae6>] do_syscall_64+0x33/0x40
> [<00000000e00521b4>] entry_SYSCALL_64_after_hwframe+0x44/0xa9
>
> It seems ifb and loopback may also hit the leak, so I try to fix this in
> register_netdevice().
>
> In common case, priv_destructor() will be called in netdev_run_todo()
> after calling ndo_uninit() in rollback_registered(), on other error
> path in register_netdevice(), ndo_uninit() and priv_destructor() are
> called before register_netdevice() return, but in this case,
> priv_destructor() will never be called, then it causes memory leak,
> so we should call priv_destructor() here.
>
> Reported-by: Hulk Robot <hulkci@huawei.com>
> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
> ---
> net/core/dev.c | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 82dc6b48e45f..907204395b64 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -10000,6 +10000,17 @@ int register_netdevice(struct net_device *dev)
> ret = notifier_to_errno(ret);
> if (ret) {
> rollback_registered(dev);
> + /*
> + * In common case, priv_destructor() will be
> + * called in netdev_run_todo() after calling
> + * ndo_uninit() in rollback_registered().
> + * But in this case, priv_destructor() will
> + * never be called, then it causes memory
> + * leak, so we should call priv_destructor()
> + * here.
> + */
> + if (dev->priv_destructor)
> + dev->priv_destructor(dev);
Are you sure this is safe?
Several devices have destructors that call free_netdev.
Up until now a common pattern for those devices was to call
free_netdev on error. After this change it would lead to double free.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net] net: fix memory leak in register_netdevice() on error path
2020-11-30 4:39 ` Stephen Hemminger
@ 2020-11-30 11:12 ` Yang Yingliang
0 siblings, 0 replies; 5+ messages in thread
From: Yang Yingliang @ 2020-11-30 11:12 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev, davem, kuba, toshiaki.makita1, rkovhaev
On 2020/11/30 12:39, Stephen Hemminger wrote:
> On Thu, 26 Nov 2020 21:23:12 +0800
> Yang Yingliang <yangyingliang@huawei.com> wrote:
>
>> I got a memleak report when doing fault-inject test:
>>
>> unreferenced object 0xffff88810ace9000 (size 1024):
>> comm "ip", pid 4622, jiffies 4295457037 (age 43.378s)
>> hex dump (first 32 bytes):
>> 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 00 00 00 00 00 00 ................
>> backtrace:
>> [<00000000008abe41>] __kmalloc+0x10f/0x210
>> [<000000005d3533a6>] veth_dev_init+0x140/0x310
>> [<0000000088353c64>] register_netdevice+0x496/0x7a0
>> [<000000001324d322>] veth_newlink+0x40b/0x960
>> [<00000000d0799866>] __rtnl_newlink+0xd8c/0x1360
>> [<00000000d616040a>] rtnl_newlink+0x6b/0xa0
>> [<00000000e0a1600d>] rtnetlink_rcv_msg+0x3cc/0x9e0
>> [<000000009eeff98b>] netlink_rcv_skb+0x130/0x3a0
>> [<00000000500f8be1>] netlink_unicast+0x4da/0x700
>> [<00000000666c03b3>] netlink_sendmsg+0x7fe/0xcb0
>> [<0000000073b28103>] sock_sendmsg+0x143/0x180
>> [<00000000ad746a30>] ____sys_sendmsg+0x677/0x810
>> [<0000000087dd98e5>] ___sys_sendmsg+0x105/0x180
>> [<00000000028dd365>] __sys_sendmsg+0xf0/0x1c0
>> [<00000000a6bfbae6>] do_syscall_64+0x33/0x40
>> [<00000000e00521b4>] entry_SYSCALL_64_after_hwframe+0x44/0xa9
>>
>> It seems ifb and loopback may also hit the leak, so I try to fix this in
>> register_netdevice().
>>
>> In common case, priv_destructor() will be called in netdev_run_todo()
>> after calling ndo_uninit() in rollback_registered(), on other error
>> path in register_netdevice(), ndo_uninit() and priv_destructor() are
>> called before register_netdevice() return, but in this case,
>> priv_destructor() will never be called, then it causes memory leak,
>> so we should call priv_destructor() here.
>>
>> Reported-by: Hulk Robot <hulkci@huawei.com>
>> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
>> ---
>> net/core/dev.c | 11 +++++++++++
>> 1 file changed, 11 insertions(+)
>>
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index 82dc6b48e45f..907204395b64 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -10000,6 +10000,17 @@ int register_netdevice(struct net_device *dev)
>> ret = notifier_to_errno(ret);
>> if (ret) {
>> rollback_registered(dev);
>> + /*
>> + * In common case, priv_destructor() will be
>> + * called in netdev_run_todo() after calling
>> + * ndo_uninit() in rollback_registered().
>> + * But in this case, priv_destructor() will
>> + * never be called, then it causes memory
>> + * leak, so we should call priv_destructor()
>> + * here.
>> + */
>> + if (dev->priv_destructor)
>> + dev->priv_destructor(dev);
> Are you sure this is safe?
> Several devices have destructors that call free_netdev.
> Up until now a common pattern for those devices was to call
> free_netdev on error. After this change it would lead to double free.
After commit cf124db566e6 ("net: Fix inconsistent teardown and release
of private netdev state."),
free_netdev() is not be called in priv_destructor().
>
> .
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net] net: fix memory leak in register_netdevice() on error path
2020-11-29 13:56 ` Toshiaki Makita
@ 2020-11-30 11:13 ` Yang Yingliang
0 siblings, 0 replies; 5+ messages in thread
From: Yang Yingliang @ 2020-11-30 11:13 UTC (permalink / raw)
To: Toshiaki Makita; +Cc: davem, kuba, rkovhaev, Netdev
On 2020/11/29 21:56, Toshiaki Makita wrote:
> On 2020/11/26 22:23, Yang Yingliang wrote:
> ...
>> Reported-by: Hulk Robot <hulkci@huawei.com>
>> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
>> ---
>> net/core/dev.c | 11 +++++++++++
>> 1 file changed, 11 insertions(+)
>>
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index 82dc6b48e45f..907204395b64 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -10000,6 +10000,17 @@ int register_netdevice(struct net_device *dev)
>> ret = notifier_to_errno(ret);
>> if (ret) {
>> rollback_registered(dev);
>> + /*
>> + * In common case, priv_destructor() will be
>
> As per netdev-faq, the comment style should be
>
> /* foobar blah blah blah
> * another line of text
> */
>
> rather than
>
> /*
> * foobar blah blah blah
> * another line of text
> */
>
>> + * called in netdev_run_todo() after calling
>> + * ndo_uninit() in rollback_registered().
>> + * But in this case, priv_destructor() will
>> + * never be called, then it causes memory
>> + * leak, so we should call priv_destructor()
>> + * here.
>> + */
>> + if (dev->priv_destructor)
>> + dev->priv_destructor(dev);
>
> To be in line with netdev_run_todo(), I think priv_destructor() should be
> called after "dev->reg_state = NETREG_UNREGISTERED".
OK, I will send a v2 later.
>
> Toshiaki Makita
>
>> rcu_barrier();
>> dev->reg_state = NETREG_UNREGISTERED;
>>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-11-30 11:14 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-26 13:23 [PATCH net] net: fix memory leak in register_netdevice() on error path Yang Yingliang
2020-11-29 13:56 ` Toshiaki Makita
2020-11-30 11:13 ` Yang Yingliang
2020-11-30 4:39 ` Stephen Hemminger
2020-11-30 11:12 ` Yang Yingliang
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.