Re: [PATCH] tun: don't zeroize sock->file on detach
21.08.2012 21:18, Neal Cardwell пишет:
On Tue, Aug 21, 2012 at 12:04 PM, Stanislav Kinsbursky
wrote:
10.08.2012 03:16, David Miller пишет:
From: Stanislav Kinsbursky
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: sta...@vger.kernel.org
Reported-by: Ruan Zhijie
Tested-by: Ruan Zhijie
Acked-by: Al Viro
Acked-by: Eric Dumazet
Acked-by: Yuchung Cheng
Signed-off-by: Stanislav Kinsbursky
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, >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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] tun: don't zeroize sock-file on detach
21.08.2012 21:18, Neal Cardwell пишет:
On Tue, Aug 21, 2012 at 12:04 PM, Stanislav Kinsbursky
skinsbur...@parallels.com wrote:
10.08.2012 03:16, David Miller пишет:
From: Stanislav Kinsbursky skinsbur...@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: sta...@vger.kernel.org
Reported-by: Ruan Zhijie ruanzhi...@hotmail.com
Tested-by: Ruan Zhijie ruanzhi...@hotmail.com
Acked-by: Al Viro v...@zeniv.linux.org.uk
Acked-by: Eric Dumazet eduma...@google.com
Acked-by: Yuchung Cheng ych...@google.com
Signed-off-by: Stanislav Kinsbursky skinsbur...@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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] tun: don't zeroize sock->file on detach
On Tue, Aug 21, 2012 at 12:04 PM, Stanislav Kinsbursky wrote: > 10.08.2012 03:16, David Miller пишет: > >> From: Stanislav Kinsbursky >> 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: sta...@vger.kernel.org >>> Reported-by: Ruan Zhijie >>> Tested-by: Ruan Zhijie >>> Acked-by: Al Viro >>> Acked-by: Eric Dumazet >>> Acked-by: Yuchung Cheng >>> Signed-off-by: Stanislav Kinsbursky >> >> >> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] tun: don't zeroize sock->file on detach
10.08.2012 03:16, David Miller пишет: From: Stanislav Kinsbursky 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: sta...@vger.kernel.org Reported-by: Ruan Zhijie Tested-by: Ruan Zhijie Acked-by: Al Viro Acked-by: Eric Dumazet Acked-by: Yuchung Cheng Signed-off-by: Stanislav Kinsbursky 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] tun: don't zeroize sock-file on detach
10.08.2012 03:16, David Miller пишет: From: Stanislav Kinsbursky skinsbur...@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: sta...@vger.kernel.org Reported-by: Ruan Zhijie ruanzhi...@hotmail.com Tested-by: Ruan Zhijie ruanzhi...@hotmail.com Acked-by: Al Viro v...@zeniv.linux.org.uk Acked-by: Eric Dumazet eduma...@google.com Acked-by: Yuchung Cheng ych...@google.com Signed-off-by: Stanislav Kinsbursky skinsbur...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] tun: don't zeroize sock-file on detach
On Tue, Aug 21, 2012 at 12:04 PM, Stanislav Kinsbursky skinsbur...@parallels.com wrote: 10.08.2012 03:16, David Miller пишет: From: Stanislav Kinsbursky skinsbur...@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: sta...@vger.kernel.org Reported-by: Ruan Zhijie ruanzhi...@hotmail.com Tested-by: Ruan Zhijie ruanzhi...@hotmail.com Acked-by: Al Viro v...@zeniv.linux.org.uk Acked-by: Eric Dumazet eduma...@google.com Acked-by: Yuchung Cheng ych...@google.com Signed-off-by: Stanislav Kinsbursky skinsbur...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] tun: don't zeroize sock->file on detach
From: Stanislav Kinsbursky 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: sta...@vger.kernel.org > Reported-by: Ruan Zhijie > Tested-by: Ruan Zhijie > Acked-by: Al Viro > Acked-by: Eric Dumazet > Acked-by: Yuchung Cheng > Signed-off-by: Stanislav Kinsbursky Applied, thanks. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] tun: don't zeroize sock->file on detach
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: sta...@vger.kernel.org Reported-by: Ruan Zhijie Tested-by: Ruan Zhijie Acked-by: Al Viro Acked-by: Eric Dumazet Acked-by: Yuchung Cheng Signed-off-by: Stanislav Kinsbursky --- 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 */ -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] tun: don't zeroize sock-file on detach
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: sta...@vger.kernel.org Reported-by: Ruan Zhijie ruanzhi...@hotmail.com Tested-by: Ruan Zhijie ruanzhi...@hotmail.com Acked-by: Al Viro v...@zeniv.linux.org.uk Acked-by: Eric Dumazet eduma...@google.com Acked-by: Yuchung Cheng ych...@google.com Signed-off-by: Stanislav Kinsbursky skinsbur...@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 */ -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] tun: don't zeroize sock-file on detach
From: Stanislav Kinsbursky skinsbur...@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: sta...@vger.kernel.org Reported-by: Ruan Zhijie ruanzhi...@hotmail.com Tested-by: Ruan Zhijie ruanzhi...@hotmail.com Acked-by: Al Viro v...@zeniv.linux.org.uk Acked-by: Eric Dumazet eduma...@google.com Acked-by: Yuchung Cheng ych...@google.com Signed-off-by: Stanislav Kinsbursky skinsbur...@parallels.com Applied, thanks. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] tun: don't zeroize sock->file on detach
From: Yuchung Cheng Date: Wed, 8 Aug 2012 10:48:32 -0700 > On Wed, Aug 8, 2012 at 5:53 AM, Stanislav Kinsbursky > wrote: >> Hi, Dave. >> What about this patch? >> >> >> On Wed, Jul 11, 2012 at 03:48:20PM +0400, Stanislav Kinsbursky wrote: >>> >>> 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. >>> >>> Signed-off-by: Stanislav Kinsbursky > Acked-by: Yuchung Cheng > > I has tested this patch and it works (so my kernel stops crashing > using tun devices). This patch needs to be formally resubmitted to netdev. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] tun: don't zeroize sock->file on detach
On Wed, Aug 8, 2012 at 5:53 AM, Stanislav Kinsbursky wrote: > Hi, Dave. > What about this patch? > > > On Wed, Jul 11, 2012 at 03:48:20PM +0400, Stanislav Kinsbursky wrote: >> >> 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. >> >> Signed-off-by: Stanislav Kinsbursky Acked-by: Yuchung Cheng I has tested this patch and it works (so my kernel stops crashing using tun devices). >> --- >> 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); > > -- > To unsubscribe from this list: send the line "unsubscribe netdev" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] tun: don't zeroize sock->file on detach
Hi, Dave. What about this patch? On Wed, Jul 11, 2012 at 03:48:20PM +0400, Stanislav Kinsbursky wrote: 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. Signed-off-by: Stanislav Kinsbursky --- 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); -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] tun: don't zeroize sock-file on detach
Hi, Dave. What about this patch? On Wed, Jul 11, 2012 at 03:48:20PM +0400, Stanislav Kinsbursky wrote: 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. Signed-off-by: Stanislav Kinsbursky skinsbur...@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); -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] tun: don't zeroize sock-file on detach
On Wed, Aug 8, 2012 at 5:53 AM, Stanislav Kinsbursky skinsbur...@parallels.com wrote: Hi, Dave. What about this patch? On Wed, Jul 11, 2012 at 03:48:20PM +0400, Stanislav Kinsbursky wrote: 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. Signed-off-by: Stanislav Kinsbursky skinsbur...@parallels.com Acked-by: Yuchung Cheng ych...@google.com I has tested this patch and it works (so my kernel stops crashing using tun devices). --- 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); -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] tun: don't zeroize sock-file on detach
From: Yuchung Cheng ych...@google.com Date: Wed, 8 Aug 2012 10:48:32 -0700 On Wed, Aug 8, 2012 at 5:53 AM, Stanislav Kinsbursky skinsbur...@parallels.com wrote: Hi, Dave. What about this patch? On Wed, Jul 11, 2012 at 03:48:20PM +0400, Stanislav Kinsbursky wrote: 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. Signed-off-by: Stanislav Kinsbursky skinsbur...@parallels.com Acked-by: Yuchung Cheng ych...@google.com I has tested this patch and it works (so my kernel stops crashing using tun devices). This patch needs to be formally resubmitted to netdev. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] tun: don't zeroize sock->file on detach
On Wed, 2012-07-11 at 15:48 +0400, Stanislav Kinsbursky wrote: > 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. > > Signed-off-by: Stanislav Kinsbursky > --- > 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 */ > Acked-by: Eric Dumazet Thanks ! -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] tun: don't zeroize sock-file on detach
On Wed, 2012-07-11 at 15:48 +0400, Stanislav Kinsbursky wrote: 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. Signed-off-by: Stanislav Kinsbursky skinsbur...@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 */ Acked-by: Eric Dumazet eduma...@google.com Thanks ! -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] tun: don't zeroize sock->file on detach
On Wed, Jul 11, 2012 at 03:48:20PM +0400, Stanislav Kinsbursky wrote: > 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. > > Signed-off-by: Stanislav Kinsbursky > --- > 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); ACK, but I have to say that I don't like the entire area. The games around sock->file in general tend to be really nasty. Examples: 1) net/9p/trans_fd.c:p9_socket_open(): we come there with freshly created and connected struct socket in *csocket we do sock_map_fd() and bugger off if it fails we do get_file(csocket->file) twice and, having grabbed the references, close the damn fd. What happens if that races with close() on the same fd before we get to those get_file()? We hit sock_close(), which calls sock_release(), which clears csocket->file. Boom - atomic_inc_long(>f_count) is not going to do us any good. Outright bug, mitigated only by the fact that all callchains to that place go through mount(2), so you have elevated privs anyway. 2) with this sucker we hit an interesting interplay with vhost; note that the total effect of tun_get_socket() does *not* include any refcount changes. Nor should it - the caller has a valid reference to struct file, after all. Eventually the caller proceeds to drop the same reference, by doing fput(sock->file). And it will be the same struct file, but proving that takes a lot of digging through the tun.c guts; the crucial observation is that we never get to __tun_detach() as long as we have a reference to opened (cdev) file that has been successfully attached at some point and that ones that hadn't been attached at all wouldn't have passed through tun_get_socket(). IOW, it works, but it's brittle as hell. Unless I've missed something in the analysis and it's really broken, that is. Frankly, I would prefer to keep the reference to struct file for vhost explicitly in vhost data structures. Would be less dependent on the guts of tun/macvtap/whatnot that way... 3) iscsi goes as far as allocating fake struct file (with kzalloc(), and $DEITY help you if you ever call fput() on that), presumably for the sake of sctp. The only place in sctp stack I see looking at sock->file is /* in-kernel sockets don't generally have a file allocated to them * if all they do is call sock_create_kern(). */ if (sk->sk_socket->file) f_flags = sk->sk_socket->file->f_flags; timeo = sock_sndtimeo(sk, f_flags & O_NONBLOCK); in __sctp_connect() and AFAICS we could bloody well have left it NULL - we leave ->f_flags zero in that code anyway and that's what __sctp_connect() will presume on NULL ->file. I'm not familiar enough with sctp or iscsi, but at the first look it seems to be asking for removal of all those games with ->file in the latter. I really wonder if we have a single legitimate case for anything other than sock_alloc_file() setting sock->file. Anyone? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] tun: don't zeroize sock-file on detach
On Wed, Jul 11, 2012 at 03:48:20PM +0400, Stanislav Kinsbursky wrote: 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. Signed-off-by: Stanislav Kinsbursky skinsbur...@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); ACK, but I have to say that I don't like the entire area. The games around sock-file in general tend to be really nasty. Examples: 1) net/9p/trans_fd.c:p9_socket_open(): we come there with freshly created and connected struct socket in *csocket we do sock_map_fd() and bugger off if it fails we do get_file(csocket-file) twice and, having grabbed the references, close the damn fd. What happens if that races with close() on the same fd before we get to those get_file()? We hit sock_close(), which calls sock_release(), which clears csocket-file. Boom - atomic_inc_long(NULL-f_count) is not going to do us any good. Outright bug, mitigated only by the fact that all callchains to that place go through mount(2), so you have elevated privs anyway. 2) with this sucker we hit an interesting interplay with vhost; note that the total effect of tun_get_socket() does *not* include any refcount changes. Nor should it - the caller has a valid reference to struct file, after all. Eventually the caller proceeds to drop the same reference, by doing fput(sock-file). And it will be the same struct file, but proving that takes a lot of digging through the tun.c guts; the crucial observation is that we never get to __tun_detach() as long as we have a reference to opened (cdev) file that has been successfully attached at some point and that ones that hadn't been attached at all wouldn't have passed through tun_get_socket(). IOW, it works, but it's brittle as hell. Unless I've missed something in the analysis and it's really broken, that is. Frankly, I would prefer to keep the reference to struct file for vhost explicitly in vhost data structures. Would be less dependent on the guts of tun/macvtap/whatnot that way... 3) iscsi goes as far as allocating fake struct file (with kzalloc(), and $DEITY help you if you ever call fput() on that), presumably for the sake of sctp. The only place in sctp stack I see looking at sock-file is /* in-kernel sockets don't generally have a file allocated to them * if all they do is call sock_create_kern(). */ if (sk-sk_socket-file) f_flags = sk-sk_socket-file-f_flags; timeo = sock_sndtimeo(sk, f_flags O_NONBLOCK); in __sctp_connect() and AFAICS we could bloody well have left it NULL - we leave -f_flags zero in that code anyway and that's what __sctp_connect() will presume on NULL -file. I'm not familiar enough with sctp or iscsi, but at the first look it seems to be asking for removal of all those games with -file in the latter. I really wonder if we have a single legitimate case for anything other than sock_alloc_file() setting sock-file. Anyone? -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[RFC PATCH] tun: don't zeroize sock->file on detach
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. Signed-off-by: Stanislav Kinsbursky --- 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 */ -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[RFC PATCH] tun: don't zeroize sock-file on detach
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. Signed-off-by: Stanislav Kinsbursky skinsbur...@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 */ -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
