diff mbox series

[v2] tipc: fix shutdown() of connectionless socket

Message ID 8267b7c2-3dc9-41ec-5490-d1080a63be11@I-love.SAKURA.ne.jp
State Accepted
Delegated to: David Miller
Headers show
Series [v2] tipc: fix shutdown() of connectionless socket | expand

Commit Message

Tetsuo Handa Sept. 2, 2020, 1:44 p.m. UTC
syzbot is reporting hung task at nbd_ioctl() [1], for there are two
problems regarding TIPC's connectionless socket's shutdown() operation.

----------
#include <fcntl.h>
#include <sys/socket.h>
#include <sys/ioctl.h>
#include <linux/nbd.h>
#include <unistd.h>

int main(int argc, char *argv[])
{
        const int fd = open("/dev/nbd0", 3);
        alarm(5);
        ioctl(fd, NBD_SET_SOCK, socket(PF_TIPC, SOCK_DGRAM, 0));
        ioctl(fd, NBD_DO_IT, 0); /* To be interrupted by SIGALRM. */
        return 0;
}
----------

One problem is that wait_for_completion() from flush_workqueue() from
nbd_start_device_ioctl() from nbd_ioctl() cannot be completed when
nbd_start_device_ioctl() received a signal at wait_event_interruptible(),
for tipc_shutdown() from kernel_sock_shutdown(SHUT_RDWR) from
nbd_mark_nsock_dead() from sock_shutdown() from nbd_start_device_ioctl()
is failing to wake up a WQ thread sleeping at wait_woken() from
tipc_wait_for_rcvmsg() from sock_recvmsg() from sock_xmit() from
nbd_read_stat() from recv_work() scheduled by nbd_start_device() from
nbd_start_device_ioctl(). Fix this problem by always invoking
sk->sk_state_change() (like inet_shutdown() does) when tipc_shutdown() is
called.

The other problem is that tipc_wait_for_rcvmsg() cannot return when
tipc_shutdown() is called, for tipc_shutdown() sets sk->sk_shutdown to
SEND_SHUTDOWN (despite "how" is SHUT_RDWR) while tipc_wait_for_rcvmsg()
needs sk->sk_shutdown set to RCV_SHUTDOWN or SHUTDOWN_MASK. Fix this
problem by setting sk->sk_shutdown to SHUTDOWN_MASK (like inet_shutdown()
does) when the socket is connectionless.

[1] https://syzkaller.appspot.com/bug?id=3fe51d307c1f0a845485cf1798aa059d12bf18b2

Reported-by: syzbot <syzbot+e36f41d207137b5d12f7@syzkaller.appspotmail.com>
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 net/tipc/socket.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

Comments

David Miller Sept. 2, 2020, 10:50 p.m. UTC | #1
From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Date: Wed, 2 Sep 2020 22:44:16 +0900

> syzbot is reporting hung task at nbd_ioctl() [1], for there are two
> problems regarding TIPC's connectionless socket's shutdown() operation.
 ...
> One problem is that wait_for_completion() from flush_workqueue() from
> nbd_start_device_ioctl() from nbd_ioctl() cannot be completed when
> nbd_start_device_ioctl() received a signal at wait_event_interruptible(),
> for tipc_shutdown() from kernel_sock_shutdown(SHUT_RDWR) from
> nbd_mark_nsock_dead() from sock_shutdown() from nbd_start_device_ioctl()
> is failing to wake up a WQ thread sleeping at wait_woken() from
> tipc_wait_for_rcvmsg() from sock_recvmsg() from sock_xmit() from
> nbd_read_stat() from recv_work() scheduled by nbd_start_device() from
> nbd_start_device_ioctl(). Fix this problem by always invoking
> sk->sk_state_change() (like inet_shutdown() does) when tipc_shutdown() is
> called.
> 
> The other problem is that tipc_wait_for_rcvmsg() cannot return when
> tipc_shutdown() is called, for tipc_shutdown() sets sk->sk_shutdown to
> SEND_SHUTDOWN (despite "how" is SHUT_RDWR) while tipc_wait_for_rcvmsg()
> needs sk->sk_shutdown set to RCV_SHUTDOWN or SHUTDOWN_MASK. Fix this
> problem by setting sk->sk_shutdown to SHUTDOWN_MASK (like inet_shutdown()
> does) when the socket is connectionless.
> 
> [1] https://syzkaller.appspot.com/bug?id=3fe51d307c1f0a845485cf1798aa059d12bf18b2
> 
> Reported-by: syzbot <syzbot+e36f41d207137b5d12f7@syzkaller.appspotmail.com>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>

Applied and queued up for -stable, thank you.
Tetsuo Handa Sept. 3, 2020, 2:24 p.m. UTC | #2
Hello, Parthasarathy.

I have a question regarding commit 6f00089c7372ba97 ("tipc: remove SS_DISCONNECTING state").
That commit added

	sk->sk_shutdown = SEND_SHUTDOWN;

into tipc_shutdown(). What is the reason you chose SEND_SHUTDOWN despite how == SHUT_RDWR ?

Since Wouter commented that NBD expects SOCK_STREAM sockets, I think that passing TIPC's
stream socket is legal. And I can trigger hung task warning using a reproducer shown below.

----------
#include <fcntl.h>
#include <sys/socket.h>
#include <sys/ioctl.h>
#include <linux/nbd.h>
#include <unistd.h>

int main(int argc, char *argv[])
{
        const int fd = open("/dev/nbd0", 3);
        int fds[2] = { -1, -1 };
        alarm(5);
        socketpair(PF_TIPC, SOCK_STREAM, 0, fds);
        ioctl(fd, NBD_SET_SOCK, fds[0]);
        ioctl(fd, NBD_DO_IT, 0); /* To be interrupted by SIGALRM. */
        return 0;
}
----------

Applying a patch shown below solves the hung task warning, but I can't evaluate
the side effect of this patch, for I don't know why you chose SEND_SHUTDOWN and
how TIPC socket works. Can we apply this patch?

----------
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -2771,10 +2771,7 @@ static int tipc_shutdown(struct socket *sock, int how)
 
 	trace_tipc_sk_shutdown(sk, NULL, TIPC_DUMP_ALL, " ");
 	__tipc_shutdown(sock, TIPC_CONN_SHUTDOWN);
-	if (tipc_sk_type_connectionless(sk))
-		sk->sk_shutdown = SHUTDOWN_MASK;
-	else
-		sk->sk_shutdown = SEND_SHUTDOWN;
+	sk->sk_shutdown = SHUTDOWN_MASK;
 
 	if (sk->sk_state == TIPC_DISCONNECTING) {
 		/* Discard any unreceived messages */
----------
diff mbox series

Patch

diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index 2679e97e0389..ebd280e767bd 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -2771,18 +2771,21 @@  static int tipc_shutdown(struct socket *sock, int how)
 
 	trace_tipc_sk_shutdown(sk, NULL, TIPC_DUMP_ALL, " ");
 	__tipc_shutdown(sock, TIPC_CONN_SHUTDOWN);
-	sk->sk_shutdown = SEND_SHUTDOWN;
+	if (tipc_sk_type_connectionless(sk))
+		sk->sk_shutdown = SHUTDOWN_MASK;
+	else
+		sk->sk_shutdown = SEND_SHUTDOWN;
 
 	if (sk->sk_state == TIPC_DISCONNECTING) {
 		/* Discard any unreceived messages */
 		__skb_queue_purge(&sk->sk_receive_queue);
 
-		/* Wake up anyone sleeping in poll */
-		sk->sk_state_change(sk);
 		res = 0;
 	} else {
 		res = -ENOTCONN;
 	}
+	/* Wake up anyone sleeping in poll. */
+	sk->sk_state_change(sk);
 
 	release_sock(sk);
 	return res;