Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: general protection fault in tomoyo_socket_sendmsg_permission
To:     Hillf Danton <hdanton@sina.com>,
        syzbot <syzbot+95ce4b142579611ef0a9@syzkaller.appspotmail.com>
Cc:     linux-kernel@vger.kernel.org,
        linux-security-module@vger.kernel.org,
        Andrey Konovalov <andreyknvl@google.com>,
        Valentina Manea <valentina.manea.m@gmail.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        syzkaller-bugs@googlegroups.com,
        Shuah Khan <skhan@linuxfoundation.org>
References: <000000000000647eff05b3f7e0d4@google.com>
 <20201113120055.11748-1-hdanton@sina.com>
From:   Shuah Khan <skhan@linuxfoundation.org>
Message-ID: <5f71e0c1-d387-6d72-d8e4-edb11cf57f72@linuxfoundation.org>
Date:   Fri, 13 Nov 2020 10:14:40 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.3.2
MIME-Version: 1.0
In-Reply-To: <20201113120055.11748-1-hdanton@sina.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 11/13/20 5:00 AM, Hillf Danton wrote:
> Thu, 12 Nov 2020 23:21:26 -0800
>> syzbot found the following issue on:
>>
>> HEAD commit:    9dbc1c03 Merge tag 'xfs-5.10-fixes-3' of git://git.kernel...
>> git tree:       upstream
>> console output: https://syzkaller.appspot.com/x/log.txt?x=10453034500000
>> kernel config:  https://syzkaller.appspot.com/x/.config?x=1735b7978b1c3721
>> dashboard link: https://syzkaller.appspot.com/bug?extid=95ce4b142579611ef0a9
>> compiler:       gcc (GCC) 10.1.0-syz 20200507
>> userspace arch: i386
>>
>> Unfortunately, I don't have any reproducer for this issue yet.
>>

I would like to see the reproducer for this. I just can't reproduce
this problem.

> 
> Fix 96c2737716d5 ("usbip: move usbip kernel code out of staging")
> by closing the race between readers and writer of ud->tcp_socket on
> sock shutdown. To do that, add waitqueue for usbip device and make
> writer wait for all readers to go home before releasing the socket.
> 
> 
> --- a/drivers/usb/usbip/usbip_common.h
> +++ b/drivers/usb/usbip/usbip_common.h
> @@ -277,6 +277,9 @@ struct usbip_device {
>   		void (*reset)(struct usbip_device *);
>   		void (*unusable)(struct usbip_device *);
>   	} eh_ops;
> +
> +	atomic_t sk_shutdown_count;
> +	wait_queue_head_t sk_shutdown_waitq;
>   };
>   
>   #define kthread_get_run(threadfn, data, namefmt, ...)			   \
> --- a/drivers/usb/usbip/vhci_hcd.c
> +++ b/drivers/usb/usbip/vhci_hcd.c
> @@ -1008,15 +1008,20 @@ static void vhci_shutdown_connection(str
>   
>   	/* kill threads related to this sdev */
>   	if (vdev->ud.tcp_rx) {
> +		atomic_inc(&ud->sk_shutdown_count);
>   		kthread_stop_put(vdev->ud.tcp_rx);
>   		vdev->ud.tcp_rx = NULL;
>   	}
>   	if (vdev->ud.tcp_tx) {
> +		atomic_inc(&ud->sk_shutdown_count);
>   		kthread_stop_put(vdev->ud.tcp_tx);
>   		vdev->ud.tcp_tx = NULL;
>   	}
>   	pr_info("stop threads\n");
>   
> +	wait_event(ud->sk_shutdown_waitq,
> +			!atomic_read(&ud->sk_shutdown_count));
> +
>   	/* active connection is closed */
>   	if (vdev->ud.tcp_socket) {
>   		sockfd_put(vdev->ud.tcp_socket);
> @@ -1104,6 +1109,8 @@ static void vhci_device_init(struct vhci
>   	vdev->ud.eh_ops.reset = vhci_device_reset;
>   	vdev->ud.eh_ops.unusable = vhci_device_unusable;
>   
> +	init_waitqueue_head(&vdev->ud.sk_shutdown_waitq);
> +
>   	usbip_start_eh(&vdev->ud);
>   }
>   
> --- a/drivers/usb/usbip/vhci_rx.c
> +++ b/drivers/usb/usbip/vhci_rx.c
> @@ -264,5 +264,8 @@ int vhci_rx_loop(void *data)
>   		vhci_rx_pdu(ud);
>   	}
>   
> +	if (atomic_dec_and_test(&ud->sk_shutdown_count))
> +		wake_up(&ud->sk_shutdown_waitq);
> +
>   	return 0;
>   }
> --- a/drivers/usb/usbip/vhci_tx.c
> +++ b/drivers/usb/usbip/vhci_tx.c
> @@ -252,5 +252,8 @@ int vhci_tx_loop(void *data)
>   		usbip_dbg_vhci_tx("pending urbs ?, now wake up\n");
>   	}
>   
> +	if (atomic_dec_and_test(&ud->sk_shutdown_count))
> +		wake_up(&ud->sk_shutdown_waitq);
> +
>   	return 0;
>   }
> 


Without a reproducer, it is hard to verify the fix.

thanks,
-- Shuah