linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] tun: don't zeroize sock->file on detach
@ 2012-08-09 12:50 Stanislav Kinsbursky
  2012-08-09 23:16 ` David Miller
  0 siblings, 1 reply; 5+ messages in thread
From: Stanislav Kinsbursky @ 2012-08-09 12:50 UTC (permalink / raw)
  To: davem; +Cc: dhowells, netdev, rick.jones2, ycheng, linux-kernel

This is a fix for bug, introduced in 3.4 kernel by commit
1ab5ecb90cb6a3df1476e052f76a6e8f6511cb3d, which, among other things, replaced
simple sock_put() by sk_release_kernel(). Below is sequence, which leads to
oops for non-persistent devices:

tun_chr_close()
tun_detach()				<== tun->socket.file = NULL
tun_free_netdev()
sk_release_sock()
sock_release(sock->file == NULL)
iput(SOCK_INODE(sock))			<== dereference on NULL pointer

This patch just removes zeroing of socket's file from __tun_detach().
sock_release() will do this.

Cc: stable@vger.kernel.org
Reported-by: Ruan Zhijie <ruanzhijie@hotmail.com>
Tested-by: Ruan Zhijie <ruanzhijie@hotmail.com>
Acked-by: Al Viro <viro@ZenIV.linux.org.uk>
Acked-by: Eric Dumazet <edumazet@google.com>
Acked-by: Yuchung Cheng <ycheng@google.com>
Signed-off-by: Stanislav Kinsbursky <skinsbursky@parallels.com>
---
 drivers/net/tun.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 987aeef..c1639f3 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -185,7 +185,6 @@ static void __tun_detach(struct tun_struct *tun)
 	netif_tx_lock_bh(tun->dev);
 	netif_carrier_off(tun->dev);
 	tun->tfile = NULL;
-	tun->socket.file = NULL;
 	netif_tx_unlock_bh(tun->dev);
 
 	/* Drop read queue */


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] tun: don't zeroize sock->file on detach
  2012-08-09 12:50 [PATCH] tun: don't zeroize sock->file on detach Stanislav Kinsbursky
@ 2012-08-09 23:16 ` David Miller
  2012-08-21 16:04   ` Stanislav Kinsbursky
  0 siblings, 1 reply; 5+ messages in thread
From: David Miller @ 2012-08-09 23:16 UTC (permalink / raw)
  To: skinsbursky; +Cc: dhowells, netdev, rick.jones2, ycheng, linux-kernel

From: Stanislav Kinsbursky <skinsbursky@parallels.com>
Date: Thu, 09 Aug 2012 16:50:40 +0400

> This is a fix for bug, introduced in 3.4 kernel by commit
> 1ab5ecb90cb6a3df1476e052f76a6e8f6511cb3d, which, among other things, replaced
> simple sock_put() by sk_release_kernel(). Below is sequence, which leads to
> oops for non-persistent devices:
> 
> tun_chr_close()
> tun_detach()				<== tun->socket.file = NULL
> tun_free_netdev()
> sk_release_sock()
> sock_release(sock->file == NULL)
> iput(SOCK_INODE(sock))			<== dereference on NULL pointer
> 
> This patch just removes zeroing of socket's file from __tun_detach().
> sock_release() will do this.
> 
> Cc: stable@vger.kernel.org
> Reported-by: Ruan Zhijie <ruanzhijie@hotmail.com>
> Tested-by: Ruan Zhijie <ruanzhijie@hotmail.com>
> Acked-by: Al Viro <viro@ZenIV.linux.org.uk>
> Acked-by: Eric Dumazet <edumazet@google.com>
> Acked-by: Yuchung Cheng <ycheng@google.com>
> Signed-off-by: Stanislav Kinsbursky <skinsbursky@parallels.com>

Applied, thanks.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] tun: don't zeroize sock->file on detach
  2012-08-09 23:16 ` David Miller
@ 2012-08-21 16:04   ` Stanislav Kinsbursky
  2012-08-21 17:18     ` Neal Cardwell
  0 siblings, 1 reply; 5+ messages in thread
From: Stanislav Kinsbursky @ 2012-08-21 16:04 UTC (permalink / raw)
  To: David Miller; +Cc: dhowells, netdev, rick.jones2, ycheng, linux-kernel, mikulas

10.08.2012 03:16, David Miller пишет:
> From: Stanislav Kinsbursky <skinsbursky@parallels.com>
> Date: Thu, 09 Aug 2012 16:50:40 +0400
>
>> This is a fix for bug, introduced in 3.4 kernel by commit
>> 1ab5ecb90cb6a3df1476e052f76a6e8f6511cb3d, which, among other things, replaced
>> simple sock_put() by sk_release_kernel(). Below is sequence, which leads to
>> oops for non-persistent devices:
>>
>> tun_chr_close()
>> tun_detach()				<== tun->socket.file = NULL
>> tun_free_netdev()
>> sk_release_sock()
>> sock_release(sock->file == NULL)
>> iput(SOCK_INODE(sock))			<== dereference on NULL pointer
>>
>> This patch just removes zeroing of socket's file from __tun_detach().
>> sock_release() will do this.
>>
>> Cc: stable@vger.kernel.org
>> Reported-by: Ruan Zhijie <ruanzhijie@hotmail.com>
>> Tested-by: Ruan Zhijie <ruanzhijie@hotmail.com>
>> Acked-by: Al Viro <viro@ZenIV.linux.org.uk>
>> Acked-by: Eric Dumazet <edumazet@google.com>
>> Acked-by: Yuchung Cheng <ycheng@google.com>
>> Signed-off-by: Stanislav Kinsbursky <skinsbursky@parallels.com>
>
> Applied, thanks.
>

Hi, David.
I found out, that this commit: b09e786bd1dd66418b69348cb110f3a64764626a
was previous attempt to fix the problem.
I believe this commit have to be dropped.


-- 
Best regards,
Stanislav Kinsbursky

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] tun: don't zeroize sock->file on detach
  2012-08-21 16:04   ` Stanislav Kinsbursky
@ 2012-08-21 17:18     ` Neal Cardwell
  2012-08-22  9:14       ` Stanislav Kinsbursky
  0 siblings, 1 reply; 5+ messages in thread
From: Neal Cardwell @ 2012-08-21 17:18 UTC (permalink / raw)
  To: Stanislav Kinsbursky
  Cc: David Miller, dhowells, netdev, rick.jones2, ycheng,
	linux-kernel, mikulas

On Tue, Aug 21, 2012 at 12:04 PM, Stanislav Kinsbursky
<skinsbursky@parallels.com> wrote:
> 10.08.2012 03:16, David Miller пишет:
>
>> From: Stanislav Kinsbursky <skinsbursky@parallels.com>
>> Date: Thu, 09 Aug 2012 16:50:40 +0400
>>
>>> This is a fix for bug, introduced in 3.4 kernel by commit
>>> 1ab5ecb90cb6a3df1476e052f76a6e8f6511cb3d, which, among other things,
>>> replaced
>>> simple sock_put() by sk_release_kernel(). Below is sequence, which leads
>>> to
>>> oops for non-persistent devices:
>>>
>>> tun_chr_close()
>>> tun_detach()                            <== tun->socket.file = NULL
>>> tun_free_netdev()
>>> sk_release_sock()
>>> sock_release(sock->file == NULL)
>>> iput(SOCK_INODE(sock))                  <== dereference on NULL pointer
>>>
>>> This patch just removes zeroing of socket's file from __tun_detach().
>>> sock_release() will do this.
>>>
>>> Cc: stable@vger.kernel.org
>>> Reported-by: Ruan Zhijie <ruanzhijie@hotmail.com>
>>> Tested-by: Ruan Zhijie <ruanzhijie@hotmail.com>
>>> Acked-by: Al Viro <viro@ZenIV.linux.org.uk>
>>> Acked-by: Eric Dumazet <edumazet@google.com>
>>> Acked-by: Yuchung Cheng <ycheng@google.com>
>>> Signed-off-by: Stanislav Kinsbursky <skinsbursky@parallels.com>
>>
>>
>> Applied, thanks.
>>
>
> Hi, David.
> I found out, that this commit: b09e786bd1dd66418b69348cb110f3a64764626a
> was previous attempt to fix the problem.
> I believe this commit have to be dropped.

Have you tried testing with that commit reverted? AFAICT from reading
the code, if you revert b09e786bd1dd66418b69348cb110f3a64764626a then
the sockets_in_use count becomes incorrect, because sock_release()
will be calling this_cpu_sub() for each tun socket teardown when there
was no corresponding this_cpu_add() for the tun socket (because the
tun socket is not allocated with sock_alloc()).

Can you sketch in more detail why that commit should be dropped?

neal

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] tun: don't zeroize sock->file on detach
  2012-08-21 17:18     ` Neal Cardwell
@ 2012-08-22  9:14       ` Stanislav Kinsbursky
  0 siblings, 0 replies; 5+ messages in thread
From: Stanislav Kinsbursky @ 2012-08-22  9:14 UTC (permalink / raw)
  To: Neal Cardwell
  Cc: David Miller, dhowells, netdev, rick.jones2, ycheng,
	linux-kernel, mikulas

21.08.2012 21:18, Neal Cardwell пишет:
> On Tue, Aug 21, 2012 at 12:04 PM, Stanislav Kinsbursky
> <skinsbursky@parallels.com> wrote:
>> 10.08.2012 03:16, David Miller пишет:
>>
>>> From: Stanislav Kinsbursky <skinsbursky@parallels.com>
>>> Date: Thu, 09 Aug 2012 16:50:40 +0400
>>>
>>>> This is a fix for bug, introduced in 3.4 kernel by commit
>>>> 1ab5ecb90cb6a3df1476e052f76a6e8f6511cb3d, which, among other things,
>>>> replaced
>>>> simple sock_put() by sk_release_kernel(). Below is sequence, which leads
>>>> to
>>>> oops for non-persistent devices:
>>>>
>>>> tun_chr_close()
>>>> tun_detach()                            <== tun->socket.file = NULL
>>>> tun_free_netdev()
>>>> sk_release_sock()
>>>> sock_release(sock->file == NULL)
>>>> iput(SOCK_INODE(sock))                  <== dereference on NULL pointer
>>>>
>>>> This patch just removes zeroing of socket's file from __tun_detach().
>>>> sock_release() will do this.
>>>>
>>>> Cc: stable@vger.kernel.org
>>>> Reported-by: Ruan Zhijie <ruanzhijie@hotmail.com>
>>>> Tested-by: Ruan Zhijie <ruanzhijie@hotmail.com>
>>>> Acked-by: Al Viro <viro@ZenIV.linux.org.uk>
>>>> Acked-by: Eric Dumazet <edumazet@google.com>
>>>> Acked-by: Yuchung Cheng <ycheng@google.com>
>>>> Signed-off-by: Stanislav Kinsbursky <skinsbursky@parallels.com>
>>>
>>>
>>> Applied, thanks.
>>>
>>
>> Hi, David.
>> I found out, that this commit: b09e786bd1dd66418b69348cb110f3a64764626a
>> was previous attempt to fix the problem.
>> I believe this commit have to be dropped.
>
> Have you tried testing with that commit reverted? AFAICT from reading
> the code, if you revert b09e786bd1dd66418b69348cb110f3a64764626a then
> the sockets_in_use count becomes incorrect, because sock_release()
> will be calling this_cpu_sub() for each tun socket teardown when there
> was no corresponding this_cpu_add() for the tun socket (because the
> tun socket is not allocated with sock_alloc()).
>
> Can you sketch in more detail why that commit should be dropped?
>

Yep, I've noticed, that first commit patch fixes two problems simultaneously.
Here are they:
1) Dereference of invalid SOCK_INODE()
2) sockets_in_use incorrect value.

But I believe, that introducing new SOCK_EXTERNALLY_ALLOCATED socket flag and 
use it in generic code just to handle tun issues is overkill.
My patch solves first problem mush simpler, than mentioned commit.

About second problem...
What about this:

diff --git a/net/socket.c b/net/socket.c
index dfe5b66..dab462b 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -526,8 +526,8 @@ void sock_release(struct socket *sock)
         if (test_bit(SOCK_EXTERNALLY_ALLOCATED, &sock->flags))
                 return;

-       this_cpu_sub(sockets_in_use, 1);
         if (!sock->file) {
+               this_cpu_sub(sockets_in_use, 1);
                 iput(SOCK_INODE(sock));
                 return;
         }

?


> neal
>


-- 
Best regards,
Stanislav Kinsbursky

^ permalink raw reply related	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2012-08-22  9:14 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-08-09 12:50 [PATCH] tun: don't zeroize sock->file on detach Stanislav Kinsbursky
2012-08-09 23:16 ` David Miller
2012-08-21 16:04   ` Stanislav Kinsbursky
2012-08-21 17:18     ` Neal Cardwell
2012-08-22  9:14       ` Stanislav Kinsbursky

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).