| Message ID | 20210219094744.3577-1-penguin-kernel@I-love.SAKURA.ne.jp |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [(repost)] usb: usbip: serialize attach/detach operations | expand |
On Sat, Feb 20, 2021 at 12:08:32AM +0900, Tetsuo Handa wrote: > syzbot is reporting an ERR_PTR(-EINTR) pointer dereference at > vhci_shutdown_connection() [1], for kthread_create() became killable due > to commit 786235eeba0e1e85 ("kthread: make kthread_create() killable"). > > When SIGKILLed while attach_store() is calling kthread_get_run(), > ERR_PTR(-EINTR) is stored into vdev->ud.tcp_{rx,tx}, and then > kthread_stop_put() is called on vdev->ud.tcp_{rx,tx} from > vhci_shutdown_connection() because vdev->ud.tcp_{rx,tx} != NULL. > > Prior to commit 9720b4bc76a83807 ("staging/usbip: convert to kthread"), > "current" pointer is assigned to vdev->ud.tcp_{rx,tx} by usbip_thread() > kernel thread, and hence vdev->ud.tcp_{rx,tx} != NULL means a valid task > pointer. However, this patch does not make kthread_get_run() return NULL > when kthread_create() failed, for this patch removes kthread_get_run() in > order to fix the other bug described below. > > syzbot is also reporting a NULL pointer dereference at sock_sendmsg() [2], > for lack of serialization between attach_store() and event_handler() > causes vhci_shutdown_connection() to observe vdev->ud.tcp_tx == NULL while > vdev->ud.tcp_socket != NULL. Please read the reference link for details of > this race window. > > Therefore, this patch does the following things in order to fix reported > bugs and other possible bugs. > > (1) Handle kthread_create() failure (which fixes [1]) by grouping socket > lookup, kthread_create() and get_task_struct() into > usbip_prepare_threads() function. > > (2) Serialize usbip_sockfd_store(), detach_store(), attach_store() and > ud->eh_ops.{shutdown,reset,unusable}() operations using > usbip_event_mutex mutex (which fixes [2]). Introducing such large > mutex should be safe because ud->tcp_{tx,rx} must not wait for > event_handler() to flush because event_handler() is processed by a > singlethreaded workqueue. > > (3) Add SOCK_STREAM check into usbip_prepare_threads(), for current code > is not verifying that a file descriptor passed is actually a stream > socket. If the file descriptor passed was a SOCK_DGRAM socket, > sock_recvmsg() can't detect end of stream. > > (4) Don't perform ud->tcp_socket = NULL in vhci_device_reset(). > Since ud->tcp_{tx,rx} depend on ud->tcp_socket != NULL whereas > ud->tcp_socket and ud->tcp_{tx,rx} are assigned at the same time, > it is never safe to reset ud->tcp_socket from vhci_device_reset() > without calling kthread_stop_put() from vhci_shutdown_connection(). > > (5) usbip_sockfd_store() must perform > > if ({sdev,udc}->ud.status != SDEV_ST_AVAILABLE) { > /* misc assignments for attach operation */ > {sdev,udc}->ud.status = SDEV_ST_USED; > } > > atomically, or multiple ud->tcp_{tx,rx} are created (which will later > cause a crash like [2]) and refcount on ud->tcp_socket is leaked when > usbip_sockfd_store() is concurrently called. > > [1] https://syzkaller.appspot.com/bug?extid=a93fba6d384346a761e3 > [2] https://syzkaller.appspot.com/bug?extid=95ce4b142579611ef0a9 > > Reported-and-tested-by: syzbot <syzbot+a93fba6d384346a761e3@syzkaller.appspotmail.com> > Reported-by: syzbot <syzbot+bf1a360e305ee719e364@syzkaller.appspotmail.com> > Reported-by: syzbot <syzbot+95ce4b142579611ef0a9@syzkaller.appspotmail.com> > References: https://lkml.kernel.org/r/676d4518-0faa-9fab-15db-0db8d216d7fb@i-love.sakura.ne.jp > Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > Fixes: 9720b4bc76a83807 ("staging/usbip: convert to kthread") > --- > drivers/usb/usbip/stub_dev.c | 56 ++++++++++++++++++-------------- > drivers/usb/usbip/usbip_common.c | 55 +++++++++++++++++++++++++++++++ > drivers/usb/usbip/usbip_common.h | 25 +++++++------- > drivers/usb/usbip/usbip_event.c | 15 +++++++++ > drivers/usb/usbip/vhci_hcd.c | 6 ---- > drivers/usb/usbip/vhci_sysfs.c | 50 ++++++++++++++++++++-------- > drivers/usb/usbip/vudc_sysfs.c | 50 ++++++++++++++++------------ > 7 files changed, 181 insertions(+), 76 deletions(-) What changed from v1? Why isn't that info below the --- line? Please do a v3 with that fixed up. thanks, greg k-h
On 2/19/21 8:53 AM, Greg Kroah-Hartman wrote: > On Sat, Feb 20, 2021 at 12:08:32AM +0900, Tetsuo Handa wrote: >> syzbot is reporting an ERR_PTR(-EINTR) pointer dereference at >> vhci_shutdown_connection() [1], for kthread_create() became killable due >> to commit 786235eeba0e1e85 ("kthread: make kthread_create() killable"). >> >> When SIGKILLed while attach_store() is calling kthread_get_run(), >> ERR_PTR(-EINTR) is stored into vdev->ud.tcp_{rx,tx}, and then >> kthread_stop_put() is called on vdev->ud.tcp_{rx,tx} from >> vhci_shutdown_connection() because vdev->ud.tcp_{rx,tx} != NULL. >> >> Prior to commit 9720b4bc76a83807 ("staging/usbip: convert to kthread"), >> "current" pointer is assigned to vdev->ud.tcp_{rx,tx} by usbip_thread() >> kernel thread, and hence vdev->ud.tcp_{rx,tx} != NULL means a valid task >> pointer. However, this patch does not make kthread_get_run() return NULL >> when kthread_create() failed, for this patch removes kthread_get_run() in >> order to fix the other bug described below. >> >> syzbot is also reporting a NULL pointer dereference at sock_sendmsg() [2], >> for lack of serialization between attach_store() and event_handler() >> causes vhci_shutdown_connection() to observe vdev->ud.tcp_tx == NULL while >> vdev->ud.tcp_socket != NULL. Please read the reference link for details of >> this race window. >> >> Therefore, this patch does the following things in order to fix reported >> bugs and other possible bugs. >> >> (1) Handle kthread_create() failure (which fixes [1]) by grouping socket >> lookup, kthread_create() and get_task_struct() into >> usbip_prepare_threads() function. >> >> (2) Serialize usbip_sockfd_store(), detach_store(), attach_store() and >> ud->eh_ops.{shutdown,reset,unusable}() operations using >> usbip_event_mutex mutex (which fixes [2]). Introducing such large >> mutex should be safe because ud->tcp_{tx,rx} must not wait for >> event_handler() to flush because event_handler() is processed by a >> singlethreaded workqueue. >> >> (3) Add SOCK_STREAM check into usbip_prepare_threads(), for current code >> is not verifying that a file descriptor passed is actually a stream >> socket. If the file descriptor passed was a SOCK_DGRAM socket, >> sock_recvmsg() can't detect end of stream. >> >> (4) Don't perform ud->tcp_socket = NULL in vhci_device_reset(). >> Since ud->tcp_{tx,rx} depend on ud->tcp_socket != NULL whereas >> ud->tcp_socket and ud->tcp_{tx,rx} are assigned at the same time, >> it is never safe to reset ud->tcp_socket from vhci_device_reset() >> without calling kthread_stop_put() from vhci_shutdown_connection(). >> >> (5) usbip_sockfd_store() must perform >> >> if ({sdev,udc}->ud.status != SDEV_ST_AVAILABLE) { >> /* misc assignments for attach operation */ >> {sdev,udc}->ud.status = SDEV_ST_USED; >> } >> >> atomically, or multiple ud->tcp_{tx,rx} are created (which will later >> cause a crash like [2]) and refcount on ud->tcp_socket is leaked when >> usbip_sockfd_store() is concurrently called. >> >> [1] https://syzkaller.appspot.com/bug?extid=a93fba6d384346a761e3 >> [2] https://syzkaller.appspot.com/bug?extid=95ce4b142579611ef0a9 >> >> Reported-and-tested-by: syzbot <syzbot+a93fba6d384346a761e3@syzkaller.appspotmail.com> >> Reported-by: syzbot <syzbot+bf1a360e305ee719e364@syzkaller.appspotmail.com> >> Reported-by: syzbot <syzbot+95ce4b142579611ef0a9@syzkaller.appspotmail.com> >> References: https://lkml.kernel.org/r/676d4518-0faa-9fab-15db-0db8d216d7fb@i-love.sakura.ne.jp >> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> >> Fixes: 9720b4bc76a83807 ("staging/usbip: convert to kthread") >> --- >> drivers/usb/usbip/stub_dev.c | 56 ++++++++++++++++++-------------- >> drivers/usb/usbip/usbip_common.c | 55 +++++++++++++++++++++++++++++++ >> drivers/usb/usbip/usbip_common.h | 25 +++++++------- >> drivers/usb/usbip/usbip_event.c | 15 +++++++++ >> drivers/usb/usbip/vhci_hcd.c | 6 ---- >> drivers/usb/usbip/vhci_sysfs.c | 50 ++++++++++++++++++++-------- >> drivers/usb/usbip/vudc_sysfs.c | 50 ++++++++++++++++------------ >> 7 files changed, 181 insertions(+), 76 deletions(-) > > What changed from v1? Why isn't that info below the --- line? > > Please do a v3 with that fixed up. > +1 on this. Also please run the usbip test on your changes: tools/testing/selftests/drivers/usb/usbip/usbip_test.sh thanks, -- Shuah
On 2021/02/20 1:00, Shuah Khan wrote: > On 2/19/21 8:53 AM, Greg Kroah-Hartman wrote: >> What changed from v1? Why isn't that info below the --- line? >> >> Please do a v3 with that fixed up. >> > > +1 on this. v2 fixed the PTR_ERR() access which was reported in v1 as below. On 2021/02/20 2:10, Julia Lawall wrote: > From: kernel test robot <lkp@intel.com> > > PTR_ERR should access the value just tested by IS_ERR > > Generated by: scripts/coccinelle/tests/odd_ptr_err.cocci > > CC: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > Reported-by: kernel test robot <lkp@intel.com> > Signed-off-by: kernel test robot <lkp@intel.com> > Signed-off-by: Julia Lawall <julia.lawall@inria.fr> > > Also please run the usbip test on your changes: > tools/testing/selftests/drivers/usb/usbip/usbip_test.sh Too much requirements for me to run that test. I'm not a user of your module. Please run that test on v2 using your environment. root@fuzz:~/usbip-utils# ~/usbip-utils/src/usbip list -l - busid 3-1 (0e0f:0003) VMware, Inc. : Virtual Mouse (0e0f:0003) root@fuzz:~/usbip-utils# ~/usbip-utils/src/usbipd -D root@fuzz:~/usbip-utils# pidof usbipd 11191 root@fuzz:~/usbip-utils# ~/usbip-utils/src/usbip list -l - busid 3-1 (0e0f:0003) VMware, Inc. : Virtual Mouse (0e0f:0003) root@fuzz:~/usbip-utils# ~/usbip-utils/src/usbip list --remote 127.0.0.1 Exportable USB devices ====================== - 127.0.0.1 3-1: VMware, Inc. : Virtual Mouse (0e0f:0003) : /sys/devices/pci0000:00/0000:00:15.0/0000:03:00.0/usb3/3-1 : (Defined at Interface level) (00/00/00) root@fuzz:~/usbip-utils#
On Sat, Feb 20, 2021 at 10:10:03AM +0900, Tetsuo Handa wrote: > On 2021/02/20 1:00, Shuah Khan wrote: > > On 2/19/21 8:53 AM, Greg Kroah-Hartman wrote: > >> What changed from v1? Why isn't that info below the --- line? > >> > >> Please do a v3 with that fixed up. > >> > > > > +1 on this. > > v2 fixed the PTR_ERR() access which was reported in v1 as below. <snip> Great, please add that to the v3 patch when you resubmit it. > > On 2021/02/20 2:10, Julia Lawall wrote: > > From: kernel test robot <lkp@intel.com> > > > > PTR_ERR should access the value just tested by IS_ERR > > > > Generated by: scripts/coccinelle/tests/odd_ptr_err.cocci > > > > CC: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > > Reported-by: kernel test robot <lkp@intel.com> > > Signed-off-by: kernel test robot <lkp@intel.com> > > Signed-off-by: Julia Lawall <julia.lawall@inria.fr> > > > > > Also please run the usbip test on your changes: > > tools/testing/selftests/drivers/usb/usbip/usbip_test.sh > > Too much requirements for me to run that test. I'm not a user of your module. It should be self-contained, what requirements do you see that you do not have? thanks, greg k-h
On 2021/02/20 15:58, Greg Kroah-Hartman wrote: >>> Also please run the usbip test on your changes: >>> tools/testing/selftests/drivers/usb/usbip/usbip_test.sh >> >> Too much requirements for me to run that test. I'm not a user of your module. > > It should be self-contained, what requirements do you see that you do > not have? It asks me to build usbip tools from source code. I took source code from https://github.com/lucianm/usbip-utils , but I couldn't rebuild without removing -Werror from Makefile . Where is maintained source code? Please embed appropriate URL into usbip_test.sh . Too many error messages to convince that the test succeeded. The only device listed in my environment is a Virtual Mouse, which makes me wonder if the test did work. Therefore, I think I should wait for your test result in your environment which would list appropriate devices. ----- console output ----- root@fuzz:~# linux/tools/testing/selftests/drivers/usb/usbip/usbip_test.sh usbip_test.sh -b <busid> -p <usbip tools path> root@fuzz:~# ~/usbip-utils/src/usbip list -l - busid 3-1 (0e0f:0003) VMware, Inc. : Virtual Mouse (0e0f:0003) root@fuzz:~# linux/tools/testing/selftests/drivers/usb/usbip/usbip_test.sh -b 3-1 -p ~/usbip-utils/ Running USB over IP Testing on 3-1 Load usbip_host module usbip_test: module usbip_host is loaded [OK] Load vhci_hcd module usbip_test: module vhci_hcd is loaded [OK] ============================================================== Expect to see export-able devices - busid 3-1 (0e0f:0003) VMware, Inc. : Virtual Mouse (0e0f:0003) ============================================================== Run lsusb to see all usb devices /: Bus 37.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M /: Bus 36.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 480M /: Bus 35.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M /: Bus 34.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 480M /: Bus 33.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M /: Bus 32.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 480M /: Bus 31.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M /: Bus 30.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 480M /: Bus 29.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M /: Bus 28.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 480M /: Bus 27.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M /: Bus 26.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 480M /: Bus 25.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M /: Bus 24.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 480M /: Bus 23.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M /: Bus 22.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 480M /: Bus 21.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M /: Bus 20.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 480M /: Bus 19.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M /: Bus 18.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 480M /: Bus 17.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M /: Bus 16.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 480M /: Bus 15.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M /: Bus 14.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 480M /: Bus 13.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M /: Bus 12.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 480M /: Bus 11.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M /: Bus 10.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 480M /: Bus 09.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M /: Bus 08.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 480M /: Bus 07.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M /: Bus 06.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 480M /: Bus 05.Port 1: Dev 1, Class=root_hub, Driver=dummy_hcd/1p, 480M /: Bus 04.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/4p, 5000M /: Bus 03.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/4p, 480M |__ Port 1: Dev 2, If 0, Class=Human Interface Device, Driver=usbhid, 12M /: Bus 02.Port 1: Dev 1, Class=root_hub, Driver=uhci_hcd/2p, 12M /: Bus 01.Port 1: Dev 1, Class=root_hub, Driver=ehci-pci/6p, 480M ============================================================== Get exported devices from localhost - expect to see none usbip: info: no exportable devices found on localhost ============================================================== bind devices bind device on busid 3-1: complete ============================================================== Run lsusb - bound devices should be under usbip_host control /: Bus 37.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M /: Bus 36.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 480M /: Bus 35.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M /: Bus 34.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 480M /: Bus 33.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M /: Bus 32.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 480M /: Bus 31.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M /: Bus 30.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 480M /: Bus 29.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M /: Bus 28.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 480M /: Bus 27.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M /: Bus 26.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 480M /: Bus 25.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M /: Bus 24.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 480M /: Bus 23.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M /: Bus 22.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 480M /: Bus 21.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M /: Bus 20.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 480M /: Bus 19.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M /: Bus 18.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 480M /: Bus 17.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M /: Bus 16.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 480M /: Bus 15.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M /: Bus 14.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 480M /: Bus 13.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M /: Bus 12.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 480M /: Bus 11.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M /: Bus 10.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 480M /: Bus 09.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M /: Bus 08.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 480M /: Bus 07.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M /: Bus 06.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 480M /: Bus 05.Port 1: Dev 1, Class=root_hub, Driver=dummy_hcd/1p, 480M /: Bus 04.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/4p, 5000M /: Bus 03.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/4p, 480M /: Bus 02.Port 1: Dev 1, Class=root_hub, Driver=uhci_hcd/2p, 12M /: Bus 01.Port 1: Dev 1, Class=root_hub, Driver=ehci-pci/6p, 480M ============================================================== bind devices - expect already bound messages usbip: error: device on busid 3-1 is already bound to usbip-host ============================================================== Get exported devices from localhost - expect to see exported devices Exportable USB devices ====================== - localhost 3-1: VMware, Inc. : Virtual Mouse (0e0f:0003) : /sys/devices/pci0000:00/0000:00:15.0/0000:03:00.0/usb3/3-1 : (Defined at Interface level) (00/00/00) ============================================================== unbind devices unbind device on busid 3-1: complete ============================================================== Run lsusb - bound devices should be rebound to original drivers /: Bus 37.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M /: Bus 36.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 480M /: Bus 35.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M /: Bus 34.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 480M /: Bus 33.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M /: Bus 32.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 480M /: Bus 31.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M /: Bus 30.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 480M /: Bus 29.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M /: Bus 28.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 480M /: Bus 27.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M /: Bus 26.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 480M /: Bus 25.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M /: Bus 24.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 480M /: Bus 23.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M /: Bus 22.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 480M /: Bus 21.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M /: Bus 20.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 480M /: Bus 19.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M /: Bus 18.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 480M /: Bus 17.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M /: Bus 16.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 480M /: Bus 15.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M /: Bus 14.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 480M /: Bus 13.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M /: Bus 12.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 480M /: Bus 11.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M /: Bus 10.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 480M /: Bus 09.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M /: Bus 08.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 480M /: Bus 07.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M /: Bus 06.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 480M /: Bus 05.Port 1: Dev 1, Class=root_hub, Driver=dummy_hcd/1p, 480M /: Bus 04.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/4p, 5000M /: Bus 03.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/4p, 480M /: Bus 02.Port 1: Dev 1, Class=root_hub, Driver=uhci_hcd/2p, 12M /: Bus 01.Port 1: Dev 1, Class=root_hub, Driver=ehci-pci/6p, 480M ============================================================== unbind devices - expect no devices bound message usbip: error: Device is not bound to usbip-host driver. ============================================================== Get exported devices from localhost - expect to see none usbip: error: failed to get device list from localhost ============================================================== List imported devices - expect to see none usbip: error: open vhci_driver usbip: error: list imported devices ============================================================== Import devices from localhost - should fail with no devices usbip: error: recv op_common usbip: error: query ============================================================== bind devices bind device on busid 3-1: complete ============================================================== List imported devices - expect to see exported devices Exportable USB devices ====================== - localhost 3-1: VMware, Inc. : Virtual Mouse (0e0f:0003) : /sys/devices/pci0000:00/0000:00:15.0/0000:03:00.0/usb3/3-1 : (Defined at Interface level) (00/00/00) ============================================================== List imported devices - expect to see none usbip: error: open vhci_driver usbip: error: list imported devices ============================================================== Import devices from localhost - should work usbip: error: open vhci_driver usbip: error: query ============================================================== List imported devices - expect to see imported devices usbip: error: open vhci_driver usbip: error: list imported devices ============================================================== Import devices from localhost - expect already imported messages usbip: error: open vhci_driver usbip: error: query ============================================================== Un-import devices usbip: error: open vhci_driver usbip: error: open vhci_driver ============================================================== List imported devices - expect to see none usbip: error: open vhci_driver usbip: error: list imported devices ============================================================== Un-import devices - expect no devices to detach messages usbip: error: open vhci_driver usbip: error: open vhci_driver ============================================================== Detach invalid port tests - expect invalid port error message usbip: error: open vhci_driver ============================================================== Expect to see export-able devices - busid 3-1 (0e0f:0003) VMware, Inc. : Virtual Mouse (0e0f:0003) ============================================================== Remove usbip_host module rmmod: ERROR: Module usbip_host is builtin. Run lsusb - bound devices should be rebound to original drivers /: Bus 37.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M /: Bus 36.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 480M /: Bus 35.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M /: Bus 34.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 480M /: Bus 33.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M /: Bus 32.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 480M /: Bus 31.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M /: Bus 30.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 480M /: Bus 29.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M /: Bus 28.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 480M /: Bus 27.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M /: Bus 26.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 480M /: Bus 25.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M /: Bus 24.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 480M /: Bus 23.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M /: Bus 22.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 480M /: Bus 21.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M /: Bus 20.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 480M /: Bus 19.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M /: Bus 18.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 480M /: Bus 17.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M /: Bus 16.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 480M /: Bus 15.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M /: Bus 14.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 480M /: Bus 13.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M /: Bus 12.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 480M /: Bus 11.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M /: Bus 10.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 480M /: Bus 09.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M /: Bus 08.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 480M /: Bus 07.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M /: Bus 06.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 480M /: Bus 05.Port 1: Dev 1, Class=root_hub, Driver=dummy_hcd/1p, 480M /: Bus 04.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/4p, 5000M /: Bus 03.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/4p, 480M /: Bus 02.Port 1: Dev 1, Class=root_hub, Driver=uhci_hcd/2p, 12M /: Bus 01.Port 1: Dev 1, Class=root_hub, Driver=ehci-pci/6p, 480M ============================================================== Run bind without usbip_host - expect fail usbip: error: device on busid 3-1 is already bound to usbip-host ============================================================== Run lsusb - devices that failed to bind aren't bound to any driver /: Bus 37.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M /: Bus 36.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 480M /: Bus 35.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M /: Bus 34.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 480M /: Bus 33.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M /: Bus 32.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 480M /: Bus 31.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M /: Bus 30.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 480M /: Bus 29.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M /: Bus 28.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 480M /: Bus 27.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M /: Bus 26.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 480M /: Bus 25.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M /: Bus 24.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 480M /: Bus 23.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M /: Bus 22.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 480M /: Bus 21.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M /: Bus 20.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 480M /: Bus 19.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M /: Bus 18.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 480M /: Bus 17.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M /: Bus 16.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 480M /: Bus 15.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M /: Bus 14.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 480M /: Bus 13.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M /: Bus 12.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 480M /: Bus 11.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M /: Bus 10.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 480M /: Bus 09.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M /: Bus 08.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 480M /: Bus 07.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M /: Bus 06.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 480M /: Bus 05.Port 1: Dev 1, Class=root_hub, Driver=dummy_hcd/1p, 480M /: Bus 04.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/4p, 5000M /: Bus 03.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/4p, 480M /: Bus 02.Port 1: Dev 1, Class=root_hub, Driver=uhci_hcd/2p, 12M /: Bus 01.Port 1: Dev 1, Class=root_hub, Driver=ehci-pci/6p, 480M ============================================================== modprobe usbip_host - does it work? Should see -busid- is not in match_busid table... skip! dmesg ============================================================== ============================================================== End of USB over IP Testing on 3-1 root@fuzz:~# echo $? 0 ----- dmesg output ----- [ 95.256736] usbip-host 3-1: usbip-host: register new device (bus 3 dev 2) [ 95.897937] usbipd[1202]: segfault at 0 ip 00007f9eebe2d4c2 sp 00007ffc4ecd52f0 error 4 in libusbip.so.0.0.1[7f9eebe2b000+7000] [ 95.902053] Code: 64 e2 ff ff 49 8b 7d 00 48 89 c6 48 89 c3 e8 75 e0 ff ff 48 89 c7 e8 cd e1 ff ff b9 0b 00 00 00 48 8d 3d 49 4f 00 00 48 89 c6 <f3> a6 0f 97 c1 80 d9 00 44 0f be f1 45 85 f6 75 ad be 58 01 00 00 [ 97.102442] usbipd[1234]: segfault at 0 ip 00007f9eebe2d4c2 sp 00007ffc4ecd52f0 error 4 in libusbip.so.0.0.1[7f9eebe2b000+7000] [ 97.106679] Code: 64 e2 ff ff 49 8b 7d 00 48 89 c6 48 89 c3 e8 75 e0 ff ff 48 89 c7 e8 cd e1 ff ff b9 0b 00 00 00 48 8d 3d 49 4f 00 00 48 89 c6 <f3> a6 0f 97 c1 80 d9 00 44 0f be f1 45 85 f6 75 ad be 58 01 00 00 [ 97.462294] usbip-host 3-1: usbip-host: register new device (bus 3 dev 2) [ 97.816252] usbip-host 3-1: stub up [ 97.818997] usbip-host 3-1: recv a header, 0 [ 97.969602] usbip-host 3-1: reset full-speed USB device number 2 using xhci_hcd [ 113.280217] usbip-host 3-1: device descriptor read/64, error -110 [ 128.702195] usbip-host 3-1: device descriptor read/64, error -110 [ 128.999228] usbip-host 3-1: reset full-speed USB device number 2 using xhci_hcd [ 144.311886] usbip-host 3-1: device descriptor read/64, error -110 [ 159.749491] usbip-host 3-1: device descriptor read/64, error -110 [ 160.062046] usbip-host 3-1: reset full-speed USB device number 2 using xhci_hcd [ 160.110668] usbip-host 3-1: device reset [ 160.159742] usbip-host 3-1: stub up [ 160.164251] usbip-host 3-1: recv a header, 0 [ 160.318951] usbip-host 3-1: reset full-speed USB device number 2 using xhci_hcd [ 175.575825] usbip-host 3-1: device descriptor read/64, error -110 [ 191.029443] usbip-host 3-1: device descriptor read/64, error -110 [ 191.326302] usbip-host 3-1: reset full-speed USB device number 2 using xhci_hcd [ 206.638840] usbip-host 3-1: device descriptor read/64, error -110 [ 222.061243] usbip-host 3-1: device descriptor read/64, error -110 [ 222.358197] usbip-host 3-1: reset full-speed USB device number 2 using xhci_hcd [ 222.406409] usbip-host 3-1: device reset
On 2/20/21 2:51 AM, Tetsuo Handa wrote: > On 2021/02/20 15:58, Greg Kroah-Hartman wrote: >>>> Also please run the usbip test on your changes: >>>> tools/testing/selftests/drivers/usb/usbip/usbip_test.sh >>> >>> Too much requirements for me to run that test. I'm not a user of your module. >> >> It should be self-contained, what requirements do you see that you do >> not have? > > It asks me to build usbip tools from source code. I took source code from > https://github.com/lucianm/usbip-utils , but I couldn't rebuild without > removing -Werror from Makefile . Where is maintained source code? > Please embed appropriate URL into usbip_test.sh . > usbip tools are part of the kernel source. Please look under: tools/usb/usbip > Too many error messages to convince that the test succeeded. The only device listed > in my environment is a Virtual Mouse, which makes me wonder if the test did work. > Therefore, I think I should wait for your test result in your environment which > would list appropriate devices. > Please use the right tool version from the kernel sources. I usually run with server and client running on the same system, client going over the loopback interface. Server exports any input devices mouse or keyboard. I will run the test on your v3. thanks, -- Shuah
On 2021/02/23 0:34, Shuah Khan wrote: > usbip tools are part of the kernel source. Please look under: > > tools/usb/usbip Oh, I didn't know it. OK, I can rebuild that one without any modification. Please explain it from usbip_test.sh . > Please use the right tool version from the kernel sources. I usually > run with server and client running on the same system, client going > over the loopback interface. Server exports any input devices mouse > or keyboard. OK. I ran the test, and I did not find any difference between with and without my patch. But I found there is a difference between the first run and the second run. My patch is irrelevant to this difference. ~/linux/tools/testing/selftests/drivers/usb/usbip/usbip_test.sh -b 3-1 -p ~/linux/tools/usb/usbip/ @@ -45,15 +45,22 @@ /: Bus 05.Port 1: Dev 1, Class=root_hub, Driver=dummy_hcd/1p, 480M /: Bus 04.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/4p, 5000M /: Bus 03.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/4p, 480M - |__ Port 1: Dev 2, If 0, Class=Human Interface Device, Driver=usbhid, 12M + |__ Port 1: Dev 2, If 0, Class=Human Interface Device, Driver=, 12M /: Bus 02.Port 1: Dev 1, Class=root_hub, Driver=uhci_hcd/2p, 12M /: Bus 01.Port 1: Dev 1, Class=root_hub, Driver=ehci-pci/6p, 480M ============================================================== Get exported devices from localhost - expect to see none -usbip: info: no exportable devices found on localhost +Exportable USB devices +====================== + - localhost + 3-1: VMware, Inc. : Virtual Mouse (0e0f:0003) + : /sys/devices/pci0000:00/0000:00:15.0/0000:03:00.0/usb3/3-1 + : (Defined at Interface level) (00/00/00) + : 0 - Human Interface Device / Boot Interface Subclass / Mouse (03/01/02) + ============================================================== bind devices -usbip: info: bind device on busid 3-1: complete +usbip: error: device on busid 3-1 is already bound to usbip-host ============================================================== Run lsusb - bound devices should be under usbip_host control /: Bus 37.Port 1: Dev 1, Class=root_hub, Driver=vhci_hcd/8p, 5000M @@ -91,6 +98,7 @@ /: Bus 05.Port 1: Dev 1, Class=root_hub, Driver=dummy_hcd/1p, 480M /: Bus 04.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/4p, 5000M /: Bus 03.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/4p, 480M + |__ Port 1: Dev 2, If 0, Class=Human Interface Device, Driver=, 12M /: Bus 02.Port 1: Dev 1, Class=root_hub, Driver=uhci_hcd/2p, 12M /: Bus 01.Port 1: Dev 1, Class=root_hub, Driver=ehci-pci/6p, 480M ============================================================== @@ -104,6 +112,7 @@ 3-1: VMware, Inc. : Virtual Mouse (0e0f:0003) : /sys/devices/pci0000:00/0000:00:15.0/0000:03:00.0/usb3/3-1 : (Defined at Interface level) (00/00/00) + : 0 - Human Interface Device / Boot Interface Subclass / Mouse (03/01/02) ============================================================== unbind devices
diff --git a/drivers/usb/usbip/stub_dev.c b/drivers/usb/usbip/stub_dev.c index 2305d425e6c9..f63a22562ead 100644 --- a/drivers/usb/usbip/stub_dev.c +++ b/drivers/usb/usbip/stub_dev.c @@ -39,12 +39,11 @@ static DEVICE_ATTR_RO(usbip_status); * is used to transfer usbip requests by kernel threads. -1 is a magic number * by which usbip connection is finished. */ -static ssize_t usbip_sockfd_store(struct device *dev, struct device_attribute *attr, - const char *buf, size_t count) +static ssize_t __usbip_sockfd_store(struct device *dev, struct device_attribute *attr, + const char *buf, size_t count) { struct stub_device *sdev = dev_get_drvdata(dev); int sockfd = 0; - struct socket *socket; int rv; if (!sdev) { @@ -57,7 +56,12 @@ static ssize_t usbip_sockfd_store(struct device *dev, struct device_attribute *a return -EINVAL; if (sockfd != -1) { - int err; + struct usbip_thread_info uti; + int err = usbip_prepare_threads(&uti, &sdev->ud, sockfd, + stub_tx_loop, "stub_tx", stub_rx_loop, "stub_rx"); + + if (err) + return err; dev_info(dev, "stub up\n"); @@ -65,44 +69,46 @@ static ssize_t usbip_sockfd_store(struct device *dev, struct device_attribute *a if (sdev->ud.status != SDEV_ST_AVAILABLE) { dev_err(dev, "not ready\n"); - goto err; + spin_unlock_irq(&sdev->ud.lock); + usbip_unprepare_threads(&uti); + return -EINVAL; } - socket = sockfd_lookup(sockfd, &err); - if (!socket) - goto err; - - sdev->ud.tcp_socket = socket; + sdev->ud.tcp_socket = uti.tcp_socket; sdev->ud.sockfd = sockfd; - - spin_unlock_irq(&sdev->ud.lock); - - sdev->ud.tcp_rx = kthread_get_run(stub_rx_loop, &sdev->ud, - "stub_rx"); - sdev->ud.tcp_tx = kthread_get_run(stub_tx_loop, &sdev->ud, - "stub_tx"); - - spin_lock_irq(&sdev->ud.lock); + sdev->ud.tcp_rx = uti.tcp_rx; + sdev->ud.tcp_tx = uti.tcp_tx; sdev->ud.status = SDEV_ST_USED; spin_unlock_irq(&sdev->ud.lock); + wake_up_process(sdev->ud.tcp_rx); + wake_up_process(sdev->ud.tcp_tx); } else { dev_info(dev, "stub down\n"); spin_lock_irq(&sdev->ud.lock); - if (sdev->ud.status != SDEV_ST_USED) - goto err; - + if (sdev->ud.status != SDEV_ST_USED) { + spin_unlock_irq(&sdev->ud.lock); + return -EINVAL; + } spin_unlock_irq(&sdev->ud.lock); usbip_event_add(&sdev->ud, SDEV_EVENT_DOWN); } return count; +} -err: - spin_unlock_irq(&sdev->ud.lock); - return -EINVAL; +static ssize_t usbip_sockfd_store(struct device *dev, struct device_attribute *attr, + const char *buf, size_t count) +{ + ssize_t ret = usbip_event_lock_killable(); + + if (ret) + return ret; + ret = __usbip_sockfd_store(dev, attr, buf, count); + usbip_event_unlock(); + return ret; } static DEVICE_ATTR_WO(usbip_sockfd); diff --git a/drivers/usb/usbip/usbip_common.c b/drivers/usb/usbip/usbip_common.c index 2ab99244bc31..88d893063001 100644 --- a/drivers/usb/usbip/usbip_common.c +++ b/drivers/usb/usbip/usbip_common.c @@ -748,6 +748,61 @@ int usbip_recv_xbuff(struct usbip_device *ud, struct urb *urb) } EXPORT_SYMBOL_GPL(usbip_recv_xbuff); +int usbip_prepare_threads(struct usbip_thread_info *uti, + struct usbip_device *ud, int sockfd, + int (*tx_fn)(void *data), const char *tx_name, + int (*rx_fn)(void *data), const char *rx_name) +{ + int err; + struct socket *socket; + struct task_struct *tx; + struct task_struct *rx; + + /* Extract socket from fd. */ + socket = sockfd_lookup(sockfd, &err); + if (!socket) + return -EINVAL; + /* Verify that this is a stream socket. */ + if (socket->type != SOCK_STREAM) { + err = -EINVAL; + goto out_socket; + } + /* Create threads for this socket. */ + rx = kthread_create(rx_fn, ud, rx_name); + if (IS_ERR(rx)) { + err = PTR_ERR(rx); + goto out_socket; + } + tx = kthread_create(tx_fn, ud, tx_name); + if (IS_ERR(tx)) { + err = PTR_ERR(rx); + goto out_rx; + } + uti->tcp_socket = socket; + get_task_struct(rx); + uti->tcp_rx = rx; + get_task_struct(tx); + uti->tcp_tx = tx; + return 0; + out_rx: + kthread_stop(rx); + out_socket: + sockfd_put(socket); + return err; +} +EXPORT_SYMBOL_GPL(usbip_prepare_threads); + +void usbip_unprepare_threads(struct usbip_thread_info *uti) +{ + kthread_stop_put(uti->tcp_tx); + uti->tcp_tx = NULL; + kthread_stop_put(uti->tcp_rx); + uti->tcp_rx = NULL; + sockfd_put(uti->tcp_socket); + uti->tcp_socket = NULL; +} +EXPORT_SYMBOL_GPL(usbip_unprepare_threads); + static int __init usbip_core_init(void) { return usbip_init_eh(); diff --git a/drivers/usb/usbip/usbip_common.h b/drivers/usb/usbip/usbip_common.h index 8be857a4fa13..c5e8e61ddf91 100644 --- a/drivers/usb/usbip/usbip_common.h +++ b/drivers/usb/usbip/usbip_common.h @@ -279,17 +279,6 @@ struct usbip_device { } eh_ops; }; -#define kthread_get_run(threadfn, data, namefmt, ...) \ -({ \ - struct task_struct *__k \ - = kthread_create(threadfn, data, namefmt, ## __VA_ARGS__); \ - if (!IS_ERR(__k)) { \ - get_task_struct(__k); \ - wake_up_process(__k); \ - } \ - __k; \ -}) - #define kthread_stop_put(k) \ do { \ kthread_stop(k); \ @@ -309,6 +298,18 @@ void usbip_header_correct_endian(struct usbip_header *pdu, int send); struct usbip_iso_packet_descriptor* usbip_alloc_iso_desc_pdu(struct urb *urb, ssize_t *bufflen); +struct usbip_thread_info { + struct socket *tcp_socket; + struct task_struct *tcp_tx; + struct task_struct *tcp_rx; +}; + +int usbip_prepare_threads(struct usbip_thread_info *uti, + struct usbip_device *ud, int sockfd, + int (*tx_fn)(void *data), const char *tx_name, + int (*rx_fn)(void *data), const char *rx_name); +void usbip_unprepare_threads(struct usbip_thread_info *uti); + /* some members of urb must be substituted before. */ int usbip_recv_iso(struct usbip_device *ud, struct urb *urb); void usbip_pad_iso(struct usbip_device *ud, struct urb *urb); @@ -322,6 +323,8 @@ void usbip_stop_eh(struct usbip_device *ud); void usbip_event_add(struct usbip_device *ud, unsigned long event); int usbip_event_happened(struct usbip_device *ud); int usbip_in_eh(struct task_struct *task); +int usbip_event_lock_killable(void); +void usbip_event_unlock(void); static inline int interface_to_busnum(struct usb_interface *interface) { diff --git a/drivers/usb/usbip/usbip_event.c b/drivers/usb/usbip/usbip_event.c index 5d88917c9631..e05b858f346d 100644 --- a/drivers/usb/usbip/usbip_event.c +++ b/drivers/usb/usbip/usbip_event.c @@ -58,6 +58,19 @@ static struct usbip_device *get_event(void) } static struct task_struct *worker_context; +static DEFINE_MUTEX(usbip_event_mutex); + +int usbip_event_lock_killable(void) +{ + return mutex_lock_killable(&usbip_event_mutex); +} +EXPORT_SYMBOL_GPL(usbip_event_lock_killable); + +void usbip_event_unlock(void) +{ + mutex_unlock(&usbip_event_mutex); +} +EXPORT_SYMBOL_GPL(usbip_event_unlock); static void event_handler(struct work_struct *work) { @@ -68,6 +81,7 @@ static void event_handler(struct work_struct *work) } while ((ud = get_event()) != NULL) { + mutex_lock(&usbip_event_mutex); usbip_dbg_eh("pending event %lx\n", ud->event); /* @@ -91,6 +105,7 @@ static void event_handler(struct work_struct *work) unset_event(ud, USBIP_EH_UNUSABLE); } + mutex_unlock(&usbip_event_mutex); wake_up(&ud->eh_waitq); } } diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c index 3209b5ddd30c..326182bf062d 100644 --- a/drivers/usb/usbip/vhci_hcd.c +++ b/drivers/usb/usbip/vhci_hcd.c @@ -1072,12 +1072,6 @@ static void vhci_device_reset(struct usbip_device *ud) usb_put_dev(vdev->udev); vdev->udev = NULL; - - if (ud->tcp_socket) { - sockfd_put(ud->tcp_socket); - ud->tcp_socket = NULL; - ud->sockfd = -1; - } ud->status = VDEV_ST_NULL; spin_unlock_irqrestore(&ud->lock, flags); diff --git a/drivers/usb/usbip/vhci_sysfs.c b/drivers/usb/usbip/vhci_sysfs.c index be37aec250c2..4d7afd1bf890 100644 --- a/drivers/usb/usbip/vhci_sysfs.c +++ b/drivers/usb/usbip/vhci_sysfs.c @@ -225,8 +225,8 @@ static int valid_port(__u32 *pdev_nr, __u32 *rhport) return 1; } -static ssize_t detach_store(struct device *dev, struct device_attribute *attr, - const char *buf, size_t count) +static ssize_t __detach_store(struct device *dev, struct device_attribute *attr, + const char *buf, size_t count) { __u32 port = 0, pdev_nr = 0, rhport = 0; struct usb_hcd *hcd; @@ -263,6 +263,17 @@ static ssize_t detach_store(struct device *dev, struct device_attribute *attr, return count; } +static ssize_t detach_store(struct device *dev, struct device_attribute *attr, + const char *buf, size_t count) +{ + ssize_t ret = usbip_event_lock_killable(); + + if (ret) + return ret; + ret = __detach_store(dev, attr, buf, count); + usbip_event_unlock(); + return ret; +} static DEVICE_ATTR_WO(detach); static int valid_args(__u32 *pdev_nr, __u32 *rhport, @@ -300,10 +311,10 @@ static int valid_args(__u32 *pdev_nr, __u32 *rhport, * * write() returns 0 on success, else negative errno. */ -static ssize_t attach_store(struct device *dev, struct device_attribute *attr, - const char *buf, size_t count) +static ssize_t __attach_store(struct device *dev, struct device_attribute *attr, + const char *buf, size_t count) { - struct socket *socket; + struct usbip_thread_info uti; int sockfd = 0; __u32 port = 0, pdev_nr = 0, rhport = 0, devid = 0, speed = 0; struct usb_hcd *hcd; @@ -347,10 +358,10 @@ static ssize_t attach_store(struct device *dev, struct device_attribute *attr, else vdev = &vhci->vhci_hcd_hs->vdev[rhport]; - /* Extract socket from fd. */ - socket = sockfd_lookup(sockfd, &err); - if (!socket) - return -EINVAL; + err = usbip_prepare_threads(&uti, &vdev->ud, sockfd, + vhci_tx_loop, "vhci_tx", vhci_rx_loop, "vhci_rx"); + if (err) + return err; /* now need lock until setting vdev status as used */ @@ -363,7 +374,7 @@ static ssize_t attach_store(struct device *dev, struct device_attribute *attr, spin_unlock(&vdev->ud.lock); spin_unlock_irqrestore(&vhci->lock, flags); - sockfd_put(socket); + usbip_unprepare_threads(&uti); dev_err(dev, "port %d already used\n", rhport); /* @@ -381,20 +392,33 @@ static ssize_t attach_store(struct device *dev, struct device_attribute *attr, vdev->devid = devid; vdev->speed = speed; vdev->ud.sockfd = sockfd; - vdev->ud.tcp_socket = socket; + vdev->ud.tcp_socket = uti.tcp_socket; + vdev->ud.tcp_rx = uti.tcp_rx; + vdev->ud.tcp_tx = uti.tcp_tx; vdev->ud.status = VDEV_ST_NOTASSIGNED; spin_unlock(&vdev->ud.lock); spin_unlock_irqrestore(&vhci->lock, flags); /* end the lock */ - vdev->ud.tcp_rx = kthread_get_run(vhci_rx_loop, &vdev->ud, "vhci_rx"); - vdev->ud.tcp_tx = kthread_get_run(vhci_tx_loop, &vdev->ud, "vhci_tx"); + wake_up_process(vdev->ud.tcp_rx); + wake_up_process(vdev->ud.tcp_tx); rh_port_connect(vdev, speed); return count; } +static ssize_t attach_store(struct device *dev, struct device_attribute *attr, + const char *buf, size_t count) +{ + ssize_t ret = usbip_event_lock_killable(); + + if (ret) + return ret; + ret = __attach_store(dev, attr, buf, count); + usbip_event_unlock(); + return ret; +} static DEVICE_ATTR_WO(attach); #define MAX_STATUS_NAME 16 diff --git a/drivers/usb/usbip/vudc_sysfs.c b/drivers/usb/usbip/vudc_sysfs.c index 100f680c572a..ff3cf225a4fa 100644 --- a/drivers/usb/usbip/vudc_sysfs.c +++ b/drivers/usb/usbip/vudc_sysfs.c @@ -90,14 +90,13 @@ static ssize_t dev_desc_read(struct file *file, struct kobject *kobj, } static BIN_ATTR_RO(dev_desc, sizeof(struct usb_device_descriptor)); -static ssize_t usbip_sockfd_store(struct device *dev, struct device_attribute *attr, - const char *in, size_t count) +static ssize_t __usbip_sockfd_store(struct device *dev, struct device_attribute *attr, + const char *in, size_t count) { struct vudc *udc = (struct vudc *) dev_get_drvdata(dev); int rv; int sockfd = 0; - int err; - struct socket *socket; + struct usbip_thread_info uti = { }; unsigned long flags; int ret; @@ -109,6 +108,14 @@ static ssize_t usbip_sockfd_store(struct device *dev, struct device_attribute *a dev_err(dev, "no device"); return -ENODEV; } + + if (sockfd != -1) { + ret = usbip_prepare_threads(&uti, &udc->ud, sockfd, + v_tx_loop, "vudc_tx", v_rx_loop, "vudc_rx"); + if (ret) + return ret; + } + spin_lock_irqsave(&udc->lock, flags); /* Don't export what we don't have */ if (!udc->driver || !udc->pullup) { @@ -130,28 +137,17 @@ static ssize_t usbip_sockfd_store(struct device *dev, struct device_attribute *a ret = -EINVAL; goto unlock_ud; } - - socket = sockfd_lookup(sockfd, &err); - if (!socket) { - dev_err(dev, "failed to lookup sock"); - ret = -EINVAL; - goto unlock_ud; - } - - udc->ud.tcp_socket = socket; - + udc->ud.tcp_socket = uti.tcp_socket; + udc->ud.tcp_rx = uti.tcp_rx; + udc->ud.tcp_tx = uti.tcp_tx; + udc->ud.status = SDEV_ST_USED; spin_unlock_irq(&udc->ud.lock); spin_unlock_irqrestore(&udc->lock, flags); - udc->ud.tcp_rx = kthread_get_run(&v_rx_loop, - &udc->ud, "vudc_rx"); - udc->ud.tcp_tx = kthread_get_run(&v_tx_loop, - &udc->ud, "vudc_tx"); + wake_up_process(udc->ud.tcp_rx); + wake_up_process(udc->ud.tcp_tx); spin_lock_irqsave(&udc->lock, flags); - spin_lock_irq(&udc->ud.lock); - udc->ud.status = SDEV_ST_USED; - spin_unlock_irq(&udc->ud.lock); ktime_get_ts64(&udc->start_time); v_start_timer(udc); @@ -181,7 +177,19 @@ static ssize_t usbip_sockfd_store(struct device *dev, struct device_attribute *a spin_unlock_irq(&udc->ud.lock); unlock: spin_unlock_irqrestore(&udc->lock, flags); + if (uti.tcp_socket) + usbip_unprepare_threads(&uti); + return ret; +} +static ssize_t usbip_sockfd_store(struct device *dev, struct device_attribute *attr, + const char *in, size_t count) +{ + ssize_t ret = usbip_event_lock_killable(); + if (ret) + return ret; + ret = __usbip_sockfd_store(dev, attr, in, count); + usbip_event_unlock(); return ret; } static DEVICE_ATTR_WO(usbip_sockfd);
syzbot is reporting an ERR_PTR(-EINTR) pointer dereference at vhci_shutdown_connection() [1], for kthread_create() became killable due to commit 786235eeba0e1e85 ("kthread: make kthread_create() killable"). When SIGKILLed while attach_store() is calling kthread_get_run(), ERR_PTR(-EINTR) is stored into vdev->ud.tcp_{rx,tx}, and then kthread_stop_put() is called on vdev->ud.tcp_{rx,tx} from vhci_shutdown_connection() because vdev->ud.tcp_{rx,tx} != NULL. Prior to commit 9720b4bc76a83807 ("staging/usbip: convert to kthread"), "current" pointer is assigned to vdev->ud.tcp_{rx,tx} by usbip_thread() kernel thread, and hence vdev->ud.tcp_{rx,tx} != NULL means a valid task pointer. However, this patch does not make kthread_get_run() return NULL when kthread_create() failed, for this patch removes kthread_get_run() in order to fix the other bug described below. syzbot is also reporting a NULL pointer dereference at sock_sendmsg() [2], for lack of serialization between attach_store() and event_handler() causes vhci_shutdown_connection() to observe vdev->ud.tcp_tx == NULL while vdev->ud.tcp_socket != NULL. Please read the reference link for details of this race window. Therefore, this patch does the following things in order to fix reported bugs and other possible bugs. (1) Handle kthread_create() failure (which fixes [1]) by grouping socket lookup, kthread_create() and get_task_struct() into usbip_prepare_threads() function. (2) Serialize usbip_sockfd_store(), detach_store(), attach_store() and ud->eh_ops.{shutdown,reset,unusable}() operations using usbip_event_mutex mutex (which fixes [2]). Introducing such large mutex should be safe because ud->tcp_{tx,rx} must not wait for event_handler() to flush because event_handler() is processed by a singlethreaded workqueue. (3) Add SOCK_STREAM check into usbip_prepare_threads(), for current code is not verifying that a file descriptor passed is actually a stream socket. If the file descriptor passed was a SOCK_DGRAM socket, sock_recvmsg() can't detect end of stream. (4) Don't perform ud->tcp_socket = NULL in vhci_device_reset(). Since ud->tcp_{tx,rx} depend on ud->tcp_socket != NULL whereas ud->tcp_socket and ud->tcp_{tx,rx} are assigned at the same time, it is never safe to reset ud->tcp_socket from vhci_device_reset() without calling kthread_stop_put() from vhci_shutdown_connection(). (5) usbip_sockfd_store() must perform if ({sdev,udc}->ud.status != SDEV_ST_AVAILABLE) { /* misc assignments for attach operation */ {sdev,udc}->ud.status = SDEV_ST_USED; } atomically, or multiple ud->tcp_{tx,rx} are created (which will later cause a crash like [2]) and refcount on ud->tcp_socket is leaked when usbip_sockfd_store() is concurrently called. [1] https://syzkaller.appspot.com/bug?extid=a93fba6d384346a761e3 [2] https://syzkaller.appspot.com/bug?extid=95ce4b142579611ef0a9 Reported-and-tested-by: syzbot <syzbot+a93fba6d384346a761e3@syzkaller.appspotmail.com> Reported-by: syzbot <syzbot+bf1a360e305ee719e364@syzkaller.appspotmail.com> Reported-by: syzbot <syzbot+95ce4b142579611ef0a9@syzkaller.appspotmail.com> References: https://lkml.kernel.org/r/676d4518-0faa-9fab-15db-0db8d216d7fb@i-love.sakura.ne.jp Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Fixes: 9720b4bc76a83807 ("staging/usbip: convert to kthread") --- drivers/usb/usbip/stub_dev.c | 56 ++++++++++++++++++-------------- drivers/usb/usbip/usbip_common.c | 55 +++++++++++++++++++++++++++++++ drivers/usb/usbip/usbip_common.h | 25 +++++++------- drivers/usb/usbip/usbip_event.c | 15 +++++++++ drivers/usb/usbip/vhci_hcd.c | 6 ---- drivers/usb/usbip/vhci_sysfs.c | 50 ++++++++++++++++++++-------- drivers/usb/usbip/vudc_sysfs.c | 50 ++++++++++++++++------------ 7 files changed, 181 insertions(+), 76 deletions(-)