tipc: fix shutdown() of connectionless socket

syzbot is reporting hung task at nbd_ioctl() [1], for there are two
problems regarding TIPC's connectionless socket's shutdown() operation.
I found C reproducer for this problem (shown below) from "no output from
test machine (2)" report.

----------

int main(int argc, char *argv[])
{
        const int fd = open("/dev/nbd0", 3);
        ioctl(fd, NBD_SET_SOCK, socket(PF_TIPC, SOCK_DGRAM, 0));
        ioctl(fd, NBD_DO_IT, 0);
        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(-)

On 2020/09/03 20:31, Wouter Verhelst wrote:
> So.
> 
> On Wed, Sep 02, 2020 at 08:09:54PM +0900, Tetsuo Handa wrote:
>> syzbot is reporting hung task at nbd_ioctl() [1], for there are two
>> problems regarding TIPC's connectionless socket's shutdown() operation.
>> I found C reproducer for this problem (shown below) from "no output from
>> test machine (2)" report.
>>
>> ----------
>>
>> int main(int argc, char *argv[])
>> {
>>         const int fd = open("/dev/nbd0", 3);
>>         ioctl(fd, NBD_SET_SOCK, socket(PF_TIPC, SOCK_DGRAM, 0));
> 
> NBD expects a stream socket, not a datagram one.
> 
>>         ioctl(fd, NBD_DO_IT, 0);
> 
> This is supposed to sit and wait until someone disconnects the device
> again (which you probably cna't do with datagram sockets). Changing that
> changes a userspace API.
> 

Excuse me, but other datagram sockets (e.g. socket(PF_INET, SOCK_DGRAM, 0)) does not
hit this problem. What do you want to do? Add a "whether the file descriptor passed
to ioctl(NBD_SET_SOCK) is a SOCK_STREAM socket" test to the NBD side?

I think that regardless of whether NBD expects only SOCK_STREAM sockets,
tipc_wait_for_rcvmsg() on a SOCK_DGRAM socket can't return is a bug.
David Miller already applied this patch and queued up for -stable.
Do we need to revert this patch?

On Thu, Sep 03, 2020 at 08:57:01PM +0900, Tetsuo Handa wrote:
> On 2020/09/03 20:31, Wouter Verhelst wrote:
> > So.
> > 
> > On Wed, Sep 02, 2020 at 08:09:54PM +0900, Tetsuo Handa wrote:
> >> syzbot is reporting hung task at nbd_ioctl() [1], for there are two
> >> problems regarding TIPC's connectionless socket's shutdown() operation.
> >> I found C reproducer for this problem (shown below) from "no output from
> >> test machine (2)" report.
> >>
> >> ----------
> >>
> >> int main(int argc, char *argv[])
> >> {
> >>         const int fd = open("/dev/nbd0", 3);
> >>         ioctl(fd, NBD_SET_SOCK, socket(PF_TIPC, SOCK_DGRAM, 0));
> > 
> > NBD expects a stream socket, not a datagram one.
> > 
> >>         ioctl(fd, NBD_DO_IT, 0);
> > 
> > This is supposed to sit and wait until someone disconnects the device
> > again (which you probably cna't do with datagram sockets). Changing that
> > changes a userspace API.
> > 
> 
> Excuse me, but other datagram sockets (e.g. socket(PF_INET, SOCK_DGRAM, 0)) does not
> hit this problem. What do you want to do? Add a "whether the file descriptor passed
> to ioctl(NBD_SET_SOCK) is a SOCK_STREAM socket" test to the NBD side?

I missed originally that you were checking whether the passed socket is
in fact a SOCK_DGRAM socket, and limiting the changes to that. That's
fine, because NBD doesn't deal with SOCK_DGRAM sockets anyway (i.e.,
passing a SOCK_DGRAM socket to the NBD device is undefined behavior). If
the behavior also changes for SOCK_STREAM sockets then that would be a
problem that would need to be reverted, but otherwise it's fine.