* [PATCH] netlink: Safer deletion of sk_bind_node
@ 2014-09-01 7:08 Harish, Jenny, K, N
2014-09-02 1:12 ` Eric W. Biederman
2014-09-02 5:03 ` David Miller
0 siblings, 2 replies; 7+ messages in thread
From: Harish, Jenny, K, N @ 2014-09-01 7:08 UTC (permalink / raw)
To: davem
Cc: dborkman, tgraf, ebiederm, darkjames-ws, rgb, eric.dumazet,
stephen, netdev, linux-kernel
From: Harish Jenny K N <harish_kandiga@mentor.com>
Unable to handle kernel NULL pointer dereference at virtual address 00000000
(netlink_release+0x0/0x2a0) from [<8034e78c>] (sock_release+0x28/0xa4)
(sock_release+0x0/0xa4) from [<8034e830>] (sock_close+0x28/0x34)
(sock_close+0x0/0x34) from [<800f3490>] (__fput+0xf0/0x1ec)
(__fput+0x0/0x1ec) from [<800f3634>] (____fput+0x10/0x14)
(____fput+0x0/0x14) from [<80040a64>] (task_work_run+0xb8/0xd8)
(task_work_run+0x0/0xd8) from [<800113a0>] (do_work_pending+0xb0/0xc4)
(do_work_pending+0x0/0xc4) from [<8000d960>] (work_pending+0xc/0x20)
Call flow of the inline and static functions
netlink_release
-----netlink_remove
---------__sk_del_bind_node
--------------__hlist_del
Signed-off-by: Harish Jenny K N <harish_kandiga@mentor.com>
---
net/netlink/af_netlink.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index c416725..21a6b32 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -1103,7 +1103,7 @@ static void netlink_remove(struct sock *sk)
netlink_table_grab();
if (nlk_sk(sk)->subscriptions)
- __sk_del_bind_node(sk);
+ hlist_del_init(&sk->sk_bind_node);
netlink_table_ungrab();
}
@@ -1382,7 +1382,7 @@ netlink_update_subscriptions(struct sock *sk, unsigned int subscriptions)
struct netlink_sock *nlk = nlk_sk(sk);
if (nlk->subscriptions && !subscriptions)
- __sk_del_bind_node(sk);
+ hlist_del_init(&sk->sk_bind_node);
else if (!nlk->subscriptions && subscriptions)
sk_add_bind_node(sk, &nl_table[sk->sk_protocol].mc_list);
nlk->subscriptions = subscriptions;
--
1.7.9.5
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH] netlink: Safer deletion of sk_bind_node
2014-09-01 7:08 [PATCH] netlink: Safer deletion of sk_bind_node Harish, Jenny, K, N
@ 2014-09-02 1:12 ` Eric W. Biederman
2014-09-02 5:03 ` David Miller
1 sibling, 0 replies; 7+ messages in thread
From: Eric W. Biederman @ 2014-09-02 1:12 UTC (permalink / raw)
To: Harish Jenny K N
Cc: davem, dborkman, tgraf, darkjames-ws, rgb, eric.dumazet, stephen,
netdev, linux-kernel
Harish Jenny K N writes:
> From: Harish Jenny K N <harish_kandiga@mentor.com>
>
> Unable to handle kernel NULL pointer dereference at virtual address 00000000
> (netlink_release+0x0/0x2a0) from [<8034e78c>] (sock_release+0x28/0xa4)
> (sock_release+0x0/0xa4) from [<8034e830>] (sock_close+0x28/0x34)
> (sock_close+0x0/0x34) from [<800f3490>] (__fput+0xf0/0x1ec)
> (__fput+0x0/0x1ec) from [<800f3634>] (____fput+0x10/0x14)
> (____fput+0x0/0x14) from [<80040a64>] (task_work_run+0xb8/0xd8)
> (task_work_run+0x0/0xd8) from [<800113a0>] (do_work_pending+0xb0/0xc4)
> (do_work_pending+0x0/0xc4) from [<8000d960>] (work_pending+0xc/0x20)
> Call flow of the inline and static functions
> netlink_release
> -----netlink_remove
> ---------__sk_del_bind_node
> --------------__hlist_del
Is there any reason __sk_del_bind_node should not be changed instead?
If not there should be a description of what makes netlink's use of
__sk_del_bind_node special....
Eric
p.s. Your name was in your from line, but not your email address making
it hard to reply to you.
> Signed-off-by: Harish Jenny K N <harish_kandiga@mentor.com>
> ---
> net/netlink/af_netlink.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
> index c416725..21a6b32 100644
> --- a/net/netlink/af_netlink.c
> +++ b/net/netlink/af_netlink.c
> @@ -1103,7 +1103,7 @@ static void netlink_remove(struct sock *sk)
>
> netlink_table_grab();
> if (nlk_sk(sk)->subscriptions)
> - __sk_del_bind_node(sk);
> + hlist_del_init(&sk->sk_bind_node);
> netlink_table_ungrab();
> }
>
> @@ -1382,7 +1382,7 @@ netlink_update_subscriptions(struct sock *sk, unsigned int subscriptions)
> struct netlink_sock *nlk = nlk_sk(sk);
>
> if (nlk->subscriptions && !subscriptions)
> - __sk_del_bind_node(sk);
> + hlist_del_init(&sk->sk_bind_node);
> else if (!nlk->subscriptions && subscriptions)
> sk_add_bind_node(sk, &nl_table[sk->sk_protocol].mc_list);
> nlk->subscriptions = subscriptions;
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] netlink: Safer deletion of sk_bind_node
2014-09-01 7:08 [PATCH] netlink: Safer deletion of sk_bind_node Harish, Jenny, K, N
2014-09-02 1:12 ` Eric W. Biederman
@ 2014-09-02 5:03 ` David Miller
2014-09-02 8:44 ` [RFC PATCH] " Harish Jenny Kandiga Nagaraj
1 sibling, 1 reply; 7+ messages in thread
From: David Miller @ 2014-09-02 5:03 UTC (permalink / raw)
To: HarishJennyKN
Cc: dborkman, tgraf, ebiederm, darkjames-ws, rgb, eric.dumazet,
stephen, netdev, linux-kernel
From: Harish Jenny K N
Date: Mon, 1 Sep 2014 12:38:29 +0530
Firstly, you really need to fix your outgoing email so that your email
address appears in your From: header properly.
> From: Harish Jenny K N <harish_kandiga@mentor.com>
>
> Unable to handle kernel NULL pointer dereference at virtual address 00000000
> (netlink_release+0x0/0x2a0) from [<8034e78c>] (sock_release+0x28/0xa4)
> (sock_release+0x0/0xa4) from [<8034e830>] (sock_close+0x28/0x34)
> (sock_close+0x0/0x34) from [<800f3490>] (__fput+0xf0/0x1ec)
> (__fput+0x0/0x1ec) from [<800f3634>] (____fput+0x10/0x14)
> (____fput+0x0/0x14) from [<80040a64>] (task_work_run+0xb8/0xd8)
> (task_work_run+0x0/0xd8) from [<800113a0>] (do_work_pending+0xb0/0xc4)
> (do_work_pending+0x0/0xc4) from [<8000d960>] (work_pending+0xc/0x20)
> Call flow of the inline and static functions
> netlink_release
> -----netlink_remove
> ---------__sk_del_bind_node
> --------------__hlist_del
>
> Signed-off-by: Harish Jenny K N <harish_kandiga@mentor.com>
This doesn't tell us anything about how this situation can be
arrived at.
When subscriptions changes, we delete the node with the table lock
held if subscriptions goes to zero. We only try to delete the node
when subscriptions was zero.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC PATCH] netlink: Safer deletion of sk_bind_node
2014-09-02 5:03 ` David Miller
@ 2014-09-02 8:44 ` Harish Jenny Kandiga Nagaraj
2014-09-02 18:52 ` David Miller
0 siblings, 1 reply; 7+ messages in thread
From: Harish Jenny Kandiga Nagaraj @ 2014-09-02 8:44 UTC (permalink / raw)
To: David Miller
Cc: dborkman, tgraf, ebiederm, darkjames-ws, rgb, eric.dumazet,
stephen, netdev, linux-kernel
In one of our random test runs we observed the crash mentioned in the previous mail.
After debugging we found out that the call flow of the inline and static functions were
netlink_release
-----netlink_remove
---------__sk_del_bind_node
--------------__hlist_del
*pprev was NULL in __hlist_del function while deleting &sk->sk_bind_node hlist_node. Hence the patch was given.
In netlink_remove function , first the sk_del_node_init function will be called. This internally calls __sk_del_node_init function. While deleting &sk->sk_node hlist_node using __sk_del_node function there is a NULL check with sk_hashed function.
Why there is no NULL check for *pprev while deleting &sk->sk_bind_node ?
On Tuesday 02 September 2014 10:33 AM, David Miller wrote:
> From: Harish Jenny K N
> Date: Mon, 1 Sep 2014 12:38:29 +0530
>
> Firstly, you really need to fix your outgoing email so that your email
> address appears in your From: header properly.
>
>> From: Harish Jenny K N <harish_kandiga@mentor.com>
>>
>> Unable to handle kernel NULL pointer dereference at virtual address 00000000
>> (netlink_release+0x0/0x2a0) from [<8034e78c>] (sock_release+0x28/0xa4)
>> (sock_release+0x0/0xa4) from [<8034e830>] (sock_close+0x28/0x34)
>> (sock_close+0x0/0x34) from [<800f3490>] (__fput+0xf0/0x1ec)
>> (__fput+0x0/0x1ec) from [<800f3634>] (____fput+0x10/0x14)
>> (____fput+0x0/0x14) from [<80040a64>] (task_work_run+0xb8/0xd8)
>> (task_work_run+0x0/0xd8) from [<800113a0>] (do_work_pending+0xb0/0xc4)
>> (do_work_pending+0x0/0xc4) from [<8000d960>] (work_pending+0xc/0x20)
>> Call flow of the inline and static functions
>> netlink_release
>> -----netlink_remove
>> ---------__sk_del_bind_node
>> --------------__hlist_del
>>
>> Signed-off-by: Harish Jenny K N <harish_kandiga@mentor.com>
> This doesn't tell us anything about how this situation can be
> arrived at.
>
> When subscriptions changes, we delete the node with the table lock
> held if subscriptions goes to zero. We only try to delete the node
> when subscriptions was zero.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC PATCH] netlink: Safer deletion of sk_bind_node
2014-09-02 8:44 ` [RFC PATCH] " Harish Jenny Kandiga Nagaraj
@ 2014-09-02 18:52 ` David Miller
2014-09-03 5:19 ` Harish Jenny Kandiga Nagaraj
0 siblings, 1 reply; 7+ messages in thread
From: David Miller @ 2014-09-02 18:52 UTC (permalink / raw)
To: harish_kandiga
Cc: dborkman, tgraf, ebiederm, darkjames-ws, rgb, eric.dumazet,
stephen, netdev, linux-kernel
From: Harish Jenny Kandiga Nagaraj <harish_kandiga@mentor.com>
Date: Tue, 2 Sep 2014 14:14:38 +0530
> In one of our random test runs we observed the crash mentioned in the previous mail.
>
> After debugging we found out that the call flow of the inline and static functions were
> netlink_release
> -----netlink_remove
> ---------__sk_del_bind_node
> --------------__hlist_del
>
> *pprev was NULL in __hlist_del function while deleting &sk->sk_bind_node hlist_node. Hence the patch was given.
>
> In netlink_remove function , first the sk_del_node_init function will be called. This internally calls __sk_del_node_init function. While deleting &sk->sk_node hlist_node using __sk_del_node function there is a NULL check with sk_hashed function.
>
> Why there is no NULL check for *pprev while deleting &sk->sk_bind_node ?
Because if ->subscriptions is non-zero, it must be on a list, and therefore
pprev must be non-NULL.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC PATCH] netlink: Safer deletion of sk_bind_node
2014-09-02 18:52 ` David Miller
@ 2014-09-03 5:19 ` Harish Jenny Kandiga Nagaraj
2014-09-03 5:28 ` David Miller
0 siblings, 1 reply; 7+ messages in thread
From: Harish Jenny Kandiga Nagaraj @ 2014-09-03 5:19 UTC (permalink / raw)
To: David Miller
Cc: dborkman, tgraf, ebiederm, darkjames-ws, rgb, eric.dumazet,
stephen, netdev, linux-kernel
If that is the case , then subscriptions of netlink_sock should have been updated after netlink_remove or netlink_release.
I don't see that happening.
On Wednesday 03 September 2014 12:22 AM, David Miller wrote:
> From: Harish Jenny Kandiga Nagaraj <harish_kandiga@mentor.com>
> Date: Tue, 2 Sep 2014 14:14:38 +0530
>
>> In one of our random test runs we observed the crash mentioned in the previous mail.
>>
>> After debugging we found out that the call flow of the inline and static functions were
>> netlink_release
>> -----netlink_remove
>> ---------__sk_del_bind_node
>> --------------__hlist_del
>>
>> *pprev was NULL in __hlist_del function while deleting &sk->sk_bind_node hlist_node. Hence the patch was given.
>>
>> In netlink_remove function , first the sk_del_node_init function will be called. This internally calls __sk_del_node_init function. While deleting &sk->sk_node hlist_node using __sk_del_node function there is a NULL check with sk_hashed function.
>>
>> Why there is no NULL check for *pprev while deleting &sk->sk_bind_node ?
> Because if ->subscriptions is non-zero, it must be on a list, and therefore
> pprev must be non-NULL.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC PATCH] netlink: Safer deletion of sk_bind_node
2014-09-03 5:19 ` Harish Jenny Kandiga Nagaraj
@ 2014-09-03 5:28 ` David Miller
0 siblings, 0 replies; 7+ messages in thread
From: David Miller @ 2014-09-03 5:28 UTC (permalink / raw)
To: harish_kandiga
Cc: dborkman, tgraf, ebiederm, darkjames-ws, rgb, eric.dumazet,
stephen, netdev, linux-kernel
From: Harish Jenny Kandiga Nagaraj <harish_kandiga@mentor.com>
Date: Wed, 3 Sep 2014 10:49:59 +0530
> If that is the case , then subscriptions of netlink_sock should have
> been updated after netlink_remove or netlink_release. I don't see
> that happening.
Please do not top-post. First provide the quoted context, then provide
your response afterwards, rather than the other way around.
The point I was trying to make is that all code paths that operate
on this list have to follow this rule, if there is a code path that
is not then that is your bug and that is where the fix belongs.
We're unexpectedly having double deletes occur on this list, I would
prefer if we don't paper over it by allowing it to silently succeed.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2014-09-03 5:28 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-09-01 7:08 [PATCH] netlink: Safer deletion of sk_bind_node Harish, Jenny, K, N
2014-09-02 1:12 ` Eric W. Biederman
2014-09-02 5:03 ` David Miller
2014-09-02 8:44 ` [RFC PATCH] " Harish Jenny Kandiga Nagaraj
2014-09-02 18:52 ` David Miller
2014-09-03 5:19 ` Harish Jenny Kandiga Nagaraj
2014-09-03 5:28 ` David Miller
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).