* Re: [PATCH 1/1] vsock: fix the race conditions in multi-transport support [not found] <20210131105914.2217229-1-alex.popov@linux.com> @ 2021-01-31 19:27 ` Linus Torvalds 2021-02-01 8:26 ` Stefano Garzarella 1 sibling, 0 replies; 3+ messages in thread From: Linus Torvalds @ 2021-01-31 19:27 UTC (permalink / raw) To: Alexander Popov Cc: David Miller, Jakub Kicinski, Stefano Garzarella, Jorgen Hansen, Stefan Schmidt, Jeff Vander Stoep, Greg KH, Netdev, Linux Kernel Mailing List [ I'm checking lkml for at least some of the emails that I'm cc'd on ] On Sun, Jan 31, 2021 at 2:59 AM Alexander Popov <alex.popov@linux.com> wrote: > > There are multiple similar bugs implicitly introduced by the > commit [...] Note: this got eaten or delayed by the mailing list issues that seem to be plaguing lkml - I'm not seeing it on lore, although google does find it on mail-archive.com. The maintainers are cc'd, but it means - for example - that if maintainers rely on patchwork, I thin kthat will be missing this email too. Linus ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH 1/1] vsock: fix the race conditions in multi-transport support [not found] <20210131105914.2217229-1-alex.popov@linux.com> 2021-01-31 19:27 ` [PATCH 1/1] vsock: fix the race conditions in multi-transport support Linus Torvalds @ 2021-02-01 8:26 ` Stefano Garzarella 2021-02-01 8:52 ` Alexander Popov 1 sibling, 1 reply; 3+ messages in thread From: Stefano Garzarella @ 2021-02-01 8:26 UTC (permalink / raw) To: Alexander Popov Cc: David Miller, Jakub Kicinski, Jorgen Hansen, Stefan Schmidt, Jeff Vander Stoep, Greg KH, Linus Torvalds, netdev, linux-kernel On Sun, Jan 31, 2021 at 01:59:14PM +0300, Alexander Popov wrote: >There are multiple similar bugs implicitly introduced by the >commit c0cfa2d8a788fcf4 ("vsock: add multi-transports support") and >commit 6a2c0962105ae8ce ("vsock: prevent transport modules unloading"). > >The bug pattern: > [1] vsock_sock.transport pointer is copied to a local variable, > [2] lock_sock() is called, > [3] the local variable is used. >VSOCK multi-transport support introduced the race condition: >vsock_sock.transport value may change between [1] and [2]. > >Let's copy vsock_sock.transport pointer to local variables after >the lock_sock() call. We can add: Fixes: c0cfa2d8a788 ("vsock: add multi-transports support") > >Signed-off-by: Alexander Popov <alex.popov@linux.com> >--- > net/vmw_vsock/af_vsock.c | 17 ++++++++++++----- > 1 file changed, 12 insertions(+), 5 deletions(-) > >diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c >index d10916ab4526..28edac1f9aa6 100644 >--- a/net/vmw_vsock/af_vsock.c >+++ b/net/vmw_vsock/af_vsock.c >@@ -997,9 +997,12 @@ static __poll_t vsock_poll(struct file *file, struct socket *sock, > mask |= EPOLLOUT | EPOLLWRNORM | EPOLLWRBAND; > > } else if (sock->type == SOCK_STREAM) { >- const struct vsock_transport *transport = vsk->transport; >+ const struct vsock_transport *transport = NULL; I think we can avoid initializing to NULL since we assign it shortly after. >+ > lock_sock(sk); > >+ transport = vsk->transport; >+ > /* Listening sockets that have connections in their accept > * queue can be read. > */ >@@ -1082,10 +1085,11 @@ static int vsock_dgram_sendmsg(struct socket *sock, struct msghdr *msg, > err = 0; > sk = sock->sk; > vsk = vsock_sk(sk); >- transport = vsk->transport; > > lock_sock(sk); > >+ transport = vsk->transport; >+ > err = vsock_auto_bind(vsk); > if (err) > goto out; >@@ -1544,10 +1548,11 @@ static int vsock_stream_setsockopt(struct >socket *sock, > err = 0; > sk = sock->sk; > vsk = vsock_sk(sk); >- transport = vsk->transport; > > lock_sock(sk); > >+ transport = vsk->transport; >+ > switch (optname) { > case SO_VM_SOCKETS_BUFFER_SIZE: > COPY_IN(val); >@@ -1680,7 +1685,6 @@ static int vsock_stream_sendmsg(struct socket *sock, struct msghdr *msg, > > sk = sock->sk; > vsk = vsock_sk(sk); >- transport = vsk->transport; > total_written = 0; > err = 0; > >@@ -1689,6 +1693,8 @@ static int vsock_stream_sendmsg(struct socket *sock, struct msghdr *msg, > > lock_sock(sk); > >+ transport = vsk->transport; >+ > /* Callers should not provide a destination with stream sockets. */ > if (msg->msg_namelen) { > err = sk->sk_state == TCP_ESTABLISHED ? -EISCONN : -EOPNOTSUPP; >@@ -1823,11 +1829,12 @@ vsock_stream_recvmsg(struct socket *sock, struct msghdr *msg, size_t len, > > sk = sock->sk; > vsk = vsock_sk(sk); >- transport = vsk->transport; > err = 0; > > lock_sock(sk); > >+ transport = vsk->transport; >+ > if (!transport || sk->sk_state != TCP_ESTABLISHED) { > /* Recvmsg is supposed to return 0 if a peer performs an > * orderly shutdown. Differentiate between that case and when a >-- >2.26.2 > Thanks for fixing this issues. With the small changes applied: Reviewed-by: Stefano Garzarella <sgarzare@redhat.com> Thanks, Stefano ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH 1/1] vsock: fix the race conditions in multi-transport support 2021-02-01 8:26 ` Stefano Garzarella @ 2021-02-01 8:52 ` Alexander Popov 0 siblings, 0 replies; 3+ messages in thread From: Alexander Popov @ 2021-02-01 8:52 UTC (permalink / raw) To: Stefano Garzarella Cc: David Miller, Jakub Kicinski, Jorgen Hansen, Stefan Schmidt, Jeff Vander Stoep, Greg KH, Linus Torvalds, netdev, linux-kernel On 01.02.2021 11:26, Stefano Garzarella wrote: > On Sun, Jan 31, 2021 at 01:59:14PM +0300, Alexander Popov wrote: >> There are multiple similar bugs implicitly introduced by the >> commit c0cfa2d8a788fcf4 ("vsock: add multi-transports support") and >> commit 6a2c0962105ae8ce ("vsock: prevent transport modules unloading"). >> >> The bug pattern: >> [1] vsock_sock.transport pointer is copied to a local variable, >> [2] lock_sock() is called, >> [3] the local variable is used. >> VSOCK multi-transport support introduced the race condition: >> vsock_sock.transport value may change between [1] and [2]. >> >> Let's copy vsock_sock.transport pointer to local variables after >> the lock_sock() call. > > We can add: > > Fixes: c0cfa2d8a788 ("vsock: add multi-transports support") > >> >> Signed-off-by: Alexander Popov <alex.popov@linux.com> >> --- >> net/vmw_vsock/af_vsock.c | 17 ++++++++++++----- >> 1 file changed, 12 insertions(+), 5 deletions(-) >> >> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c >> index d10916ab4526..28edac1f9aa6 100644 >> --- a/net/vmw_vsock/af_vsock.c >> +++ b/net/vmw_vsock/af_vsock.c >> @@ -997,9 +997,12 @@ static __poll_t vsock_poll(struct file *file, struct socket *sock, >> mask |= EPOLLOUT | EPOLLWRNORM | EPOLLWRBAND; >> >> } else if (sock->type == SOCK_STREAM) { >> - const struct vsock_transport *transport = vsk->transport; >> + const struct vsock_transport *transport = NULL; > > I think we can avoid initializing to NULL since we assign it shortly > after. > >> + >> lock_sock(sk); >> >> + transport = vsk->transport; >> + >> /* Listening sockets that have connections in their accept >> * queue can be read. >> */ >> @@ -1082,10 +1085,11 @@ static int vsock_dgram_sendmsg(struct socket *sock, struct msghdr *msg, >> err = 0; >> sk = sock->sk; >> vsk = vsock_sk(sk); >> - transport = vsk->transport; >> >> lock_sock(sk); >> >> + transport = vsk->transport; >> + >> err = vsock_auto_bind(vsk); >> if (err) >> goto out; >> @@ -1544,10 +1548,11 @@ static int vsock_stream_setsockopt(struct >> socket *sock, >> err = 0; >> sk = sock->sk; >> vsk = vsock_sk(sk); >> - transport = vsk->transport; >> >> lock_sock(sk); >> >> + transport = vsk->transport; >> + >> switch (optname) { >> case SO_VM_SOCKETS_BUFFER_SIZE: >> COPY_IN(val); >> @@ -1680,7 +1685,6 @@ static int vsock_stream_sendmsg(struct socket *sock, struct msghdr *msg, >> >> sk = sock->sk; >> vsk = vsock_sk(sk); >> - transport = vsk->transport; >> total_written = 0; >> err = 0; >> >> @@ -1689,6 +1693,8 @@ static int vsock_stream_sendmsg(struct socket *sock, struct msghdr *msg, >> >> lock_sock(sk); >> >> + transport = vsk->transport; >> + >> /* Callers should not provide a destination with stream sockets. */ >> if (msg->msg_namelen) { >> err = sk->sk_state == TCP_ESTABLISHED ? -EISCONN : -EOPNOTSUPP; >> @@ -1823,11 +1829,12 @@ vsock_stream_recvmsg(struct socket *sock, struct msghdr *msg, size_t len, >> >> sk = sock->sk; >> vsk = vsock_sk(sk); >> - transport = vsk->transport; >> err = 0; >> >> lock_sock(sk); >> >> + transport = vsk->transport; >> + >> if (!transport || sk->sk_state != TCP_ESTABLISHED) { >> /* Recvmsg is supposed to return 0 if a peer performs an >> * orderly shutdown. Differentiate between that case and when a >> -- >> 2.26.2 >> > > Thanks for fixing this issues. With the small changes applied: > > Reviewed-by: Stefano Garzarella <sgarzare@redhat.com> Hello Stefano, Thanks for the review. I've just sent the v2. Best regards, Alexander ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-02-01 8:53 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20210131105914.2217229-1-alex.popov@linux.com>
2021-01-31 19:27 ` [PATCH 1/1] vsock: fix the race conditions in multi-transport support Linus Torvalds
2021-02-01 8:26 ` Stefano Garzarella
2021-02-01 8:52 ` Alexander Popov
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).