* Re: [PATCH net] rtnetlink: invoke 'cb->done' destructor before 'cb->args' reset
2018-10-31 6:42 [PATCH net] rtnetlink: invoke 'cb->done' destructor before 'cb->args' reset Alexey Kodanev
@ 2018-10-31 6:42 ` Alexey Kodanev
2018-10-31 16:55 ` David Ahern
1 sibling, 0 replies; 7+ messages in thread
From: Alexey Kodanev @ 2018-10-31 6:42 UTC (permalink / raw)
To: netdev; +Cc: David Ahern, David Miller
On 31.10.2018 09:42, Alexey Kodanev wrote:
> cb->args[2] can store the pointer to the struct fib6_walker,
> allocated in inet6_dump_fib(). On the next loop iteration in
> rtnl_dump_all(), 'memset(&cb, 0, sizeof(cb->args))' can reset
> that pointer, leaking the memory [1].
>
On the second thought we could as well save the state of fib6_walker
in inet6_dump_fib() with cb->data. That should fix the leak too.
Is it sounds reasonable?
Thanks,
Alexey
> Fix it by calling cb->done, if it is set, before filling 'cb->args'
> with zeros.
>
> Looks like the recent changes in rtnl_dump_all() contributed to
> the appearance of this kmemleak [1], commit c63586dc9b3e ("net:
> rtnl_dump_all needs to propagate error from dumpit function")
> breaks the loop only on an error now.
>
> [1]:
> unreferenced object 0xffff88001322a200 (size 96):
> comm "sshd", pid 1484, jiffies 4296032768 (age 1432.542s)
> hex dump (first 32 bytes):
> 00 01 00 00 00 00 ad de 00 02 00 00 00 00 ad de ................
> 18 09 41 36 00 88 ff ff 18 09 41 36 00 88 ff ff ..A6......A6....
> backtrace:
> [<0000000095846b39>] kmem_cache_alloc_trace+0x151/0x220
> [<000000007d12709f>] inet6_dump_fib+0x68d/0x940
> [<000000002775a316>] rtnl_dump_all+0x1d9/0x2d0
> [<00000000d7cd302b>] netlink_dump+0x945/0x11a0
> [<000000002f43485f>] __netlink_dump_start+0x55d/0x800
> [<00000000f76bbeec>] rtnetlink_rcv_msg+0x4fa/0xa00
> [<000000009b5761f3>] netlink_rcv_skb+0x29c/0x420
> [<0000000087a1dae1>] rtnetlink_rcv+0x15/0x20
> [<00000000691b703b>] netlink_unicast+0x4e3/0x6c0
> [<00000000b5be0204>] netlink_sendmsg+0x7f2/0xba0
> [<0000000096d2aa60>] sock_sendmsg+0xba/0xf0
> [<000000008c1b786f>] __sys_sendto+0x1e4/0x330
> [<0000000019587b3f>] __x64_sys_sendto+0xe1/0x1a0
> [<00000000071f4d56>] do_syscall_64+0x9f/0x300
> [<000000002737577f>] entry_SYSCALL_64_after_hwframe+0x44/0xa9
> [<0000000057587684>] 0xffffffffffffffff
>
> Fixes: 1b43af5480c3 ("[IPV6]: Increase number of possible routing tables to 2^32")
> Signed-off-by: Alexey Kodanev <alexey.kodanev@oracle.com>
> ---
> net/core/rtnetlink.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index f679c7a..314c683 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -3362,6 +3362,8 @@ static int rtnl_dump_all(struct sk_buff *skb, struct netlink_callback *cb)
> continue;
>
> if (idx > s_idx) {
> + if (cb->done)
> + cb->done(cb);
> memset(&cb->args[0], 0, sizeof(cb->args));
> cb->prev_seq = 0;
> cb->seq = 0;
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH net] rtnetlink: invoke 'cb->done' destructor before 'cb->args' reset
@ 2018-10-31 6:42 Alexey Kodanev
2018-10-31 6:42 ` Alexey Kodanev
2018-10-31 16:55 ` David Ahern
0 siblings, 2 replies; 7+ messages in thread
From: Alexey Kodanev @ 2018-10-31 6:42 UTC (permalink / raw)
To: netdev; +Cc: David Ahern, David Miller, Alexey Kodanev
cb->args[2] can store the pointer to the struct fib6_walker,
allocated in inet6_dump_fib(). On the next loop iteration in
rtnl_dump_all(), 'memset(&cb, 0, sizeof(cb->args))' can reset
that pointer, leaking the memory [1].
Fix it by calling cb->done, if it is set, before filling 'cb->args'
with zeros.
Looks like the recent changes in rtnl_dump_all() contributed to
the appearance of this kmemleak [1], commit c63586dc9b3e ("net:
rtnl_dump_all needs to propagate error from dumpit function")
breaks the loop only on an error now.
[1]:
unreferenced object 0xffff88001322a200 (size 96):
comm "sshd", pid 1484, jiffies 4296032768 (age 1432.542s)
hex dump (first 32 bytes):
00 01 00 00 00 00 ad de 00 02 00 00 00 00 ad de ................
18 09 41 36 00 88 ff ff 18 09 41 36 00 88 ff ff ..A6......A6....
backtrace:
[<0000000095846b39>] kmem_cache_alloc_trace+0x151/0x220
[<000000007d12709f>] inet6_dump_fib+0x68d/0x940
[<000000002775a316>] rtnl_dump_all+0x1d9/0x2d0
[<00000000d7cd302b>] netlink_dump+0x945/0x11a0
[<000000002f43485f>] __netlink_dump_start+0x55d/0x800
[<00000000f76bbeec>] rtnetlink_rcv_msg+0x4fa/0xa00
[<000000009b5761f3>] netlink_rcv_skb+0x29c/0x420
[<0000000087a1dae1>] rtnetlink_rcv+0x15/0x20
[<00000000691b703b>] netlink_unicast+0x4e3/0x6c0
[<00000000b5be0204>] netlink_sendmsg+0x7f2/0xba0
[<0000000096d2aa60>] sock_sendmsg+0xba/0xf0
[<000000008c1b786f>] __sys_sendto+0x1e4/0x330
[<0000000019587b3f>] __x64_sys_sendto+0xe1/0x1a0
[<00000000071f4d56>] do_syscall_64+0x9f/0x300
[<000000002737577f>] entry_SYSCALL_64_after_hwframe+0x44/0xa9
[<0000000057587684>] 0xffffffffffffffff
Fixes: 1b43af5480c3 ("[IPV6]: Increase number of possible routing tables to 2^32")
Signed-off-by: Alexey Kodanev <alexey.kodanev@oracle.com>
---
net/core/rtnetlink.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index f679c7a..314c683 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -3362,6 +3362,8 @@ static int rtnl_dump_all(struct sk_buff *skb, struct netlink_callback *cb)
continue;
if (idx > s_idx) {
+ if (cb->done)
+ cb->done(cb);
memset(&cb->args[0], 0, sizeof(cb->args));
cb->prev_seq = 0;
cb->seq = 0;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH net] rtnetlink: invoke 'cb->done' destructor before 'cb->args' reset
2018-10-31 6:42 [PATCH net] rtnetlink: invoke 'cb->done' destructor before 'cb->args' reset Alexey Kodanev
2018-10-31 6:42 ` Alexey Kodanev
@ 2018-10-31 16:55 ` David Ahern
2018-10-31 17:35 ` David Ahern
1 sibling, 1 reply; 7+ messages in thread
From: David Ahern @ 2018-10-31 16:55 UTC (permalink / raw)
To: Alexey Kodanev, netdev; +Cc: David Miller
On 10/31/18 12:42 AM, Alexey Kodanev wrote:
> cb->args[2] can store the pointer to the struct fib6_walker,
> allocated in inet6_dump_fib(). On the next loop iteration in
> rtnl_dump_all(), 'memset(&cb, 0, sizeof(cb->args))' can reset
> that pointer, leaking the memory [1].
>
> Fix it by calling cb->done, if it is set, before filling 'cb->args'
> with zeros.
>
> Looks like the recent changes in rtnl_dump_all() contributed to
> the appearance of this kmemleak [1], commit c63586dc9b3e ("net:
> rtnl_dump_all needs to propagate error from dumpit function")
> breaks the loop only on an error now.
>
...
It is more efficient to keep going.
I think the simplest fix for 4.20 is to break the loop if ret is non-0 -
restore the previous behavior. For net-next I think the done callback is
not needed for ipv6; I think there is a simpler way to do it.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net] rtnetlink: invoke 'cb->done' destructor before 'cb->args' reset
2018-10-31 16:55 ` David Ahern
@ 2018-10-31 17:35 ` David Ahern
2018-11-01 13:11 ` Alexey Kodanev
0 siblings, 1 reply; 7+ messages in thread
From: David Ahern @ 2018-10-31 17:35 UTC (permalink / raw)
To: Alexey Kodanev, netdev; +Cc: David Miller
On 10/31/18 10:55 AM, David Ahern wrote:
> I think the simplest fix for 4.20 is to break the loop if ret is non-0 -
> restore the previous behavior.
that is the only recourse. It has to bail if ret is non-0. Do you want
to send a patch with that fix?
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net] rtnetlink: invoke 'cb->done' destructor before 'cb->args' reset
2018-10-31 17:35 ` David Ahern
@ 2018-11-01 13:11 ` Alexey Kodanev
2018-11-01 13:42 ` Alexey Kodanev
0 siblings, 1 reply; 7+ messages in thread
From: Alexey Kodanev @ 2018-11-01 13:11 UTC (permalink / raw)
To: David Ahern, netdev; +Cc: David Miller
On 10/31/2018 08:35 PM, David Ahern wrote:
> On 10/31/18 10:55 AM, David Ahern wrote:
>> I think the simplest fix for 4.20 is to break the loop if ret is non-0 -
>> restore the previous behavior.
>
> that is the only recourse. It has to bail if ret is non-0. Do you want
> to send a patch with that fix?
>
I see, and inet6_dump_fib() cleanups fib6_walker if ret is zero. Will send the fix.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net] rtnetlink: invoke 'cb->done' destructor before 'cb->args' reset
2018-11-01 13:11 ` Alexey Kodanev
@ 2018-11-01 13:42 ` Alexey Kodanev
2018-11-01 21:51 ` David Ahern
0 siblings, 1 reply; 7+ messages in thread
From: Alexey Kodanev @ 2018-11-01 13:42 UTC (permalink / raw)
To: David Ahern, netdev; +Cc: David Miller
On 11/01/2018 04:11 PM, Alexey Kodanev wrote:
> On 10/31/2018 08:35 PM, David Ahern wrote:
>> On 10/31/18 10:55 AM, David Ahern wrote:
>>> I think the simplest fix for 4.20 is to break the loop if ret is non-0 -
>>> restore the previous behavior.
>>
>> that is the only recourse. It has to bail if ret is non-0. Do you want
>> to send a patch with that fix?
>>
>
> I see, and inet6_dump_fib() cleanups fib6_walker if ret is zero. Will send the fix.
Can it happen that inet6_dump_fib() returns skb->len (0) in the below cases?
* if (arg.filter.flags & RTM_F_CLONED)
return skb->len;
...
w = (void *)cb->args[2];
if (!w) {
...
w = kzalloc(...)
...
* if (arg.filter.table_id) {
...
if (!tb) {
if (arg.filter.dump_all_families)
return skb->len;
Would it be safer to add "res = skb->len; goto out;" instead of "return skb->len;"
so that it can call fib6_dump_end() for "res <= 0"? Or use cb->data instead of
cb->args?
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net] rtnetlink: invoke 'cb->done' destructor before 'cb->args' reset
2018-11-01 13:42 ` Alexey Kodanev
@ 2018-11-01 21:51 ` David Ahern
0 siblings, 0 replies; 7+ messages in thread
From: David Ahern @ 2018-11-01 21:51 UTC (permalink / raw)
To: Alexey Kodanev, netdev; +Cc: David Miller
On 11/1/18 7:42 AM, Alexey Kodanev wrote:
> On 11/01/2018 04:11 PM, Alexey Kodanev wrote:
>> On 10/31/2018 08:35 PM, David Ahern wrote:
>>> On 10/31/18 10:55 AM, David Ahern wrote:
>>>> I think the simplest fix for 4.20 is to break the loop if ret is non-0 -
>>>> restore the previous behavior.
>>>
>>> that is the only recourse. It has to bail if ret is non-0. Do you want
>>> to send a patch with that fix?
>>>
>>
>> I see, and inet6_dump_fib() cleanups fib6_walker if ret is zero. Will send the fix.
>
> Can it happen that inet6_dump_fib() returns skb->len (0) in the below cases?
>
> * if (arg.filter.flags & RTM_F_CLONED)
> return skb->len;
>
> ...
>
> w = (void *)cb->args[2];
> if (!w) {
> ...
> w = kzalloc(...)
> ...
>
> * if (arg.filter.table_id) {
> ...
> if (!tb) {
> if (arg.filter.dump_all_families)
> return skb->len;
>
>
> Would it be safer to add "res = skb->len; goto out;" instead of "return skb->len;"
> so that it can call fib6_dump_end() for "res <= 0"? Or use cb->data instead of
> cb->args?
>
Since res is initialized to 0, both of those can just be 'goto out;'
The break in dump_all is still needed though.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-11-02 6:56 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-31 6:42 [PATCH net] rtnetlink: invoke 'cb->done' destructor before 'cb->args' reset Alexey Kodanev
2018-10-31 6:42 ` Alexey Kodanev
2018-10-31 16:55 ` David Ahern
2018-10-31 17:35 ` David Ahern
2018-11-01 13:11 ` Alexey Kodanev
2018-11-01 13:42 ` Alexey Kodanev
2018-11-01 21:51 ` David Ahern
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.