* Re: general protection fault in tomoyo_socket_sendmsg_permission [not found] <000000000000647eff05b3f7e0d4@google.com> @ 2020-11-13 10:49 ` Tetsuo Handa 2021-02-19 0:33 ` [PATCH] usb: usbip: serialize attach/detach operations Tetsuo Handa 1 sibling, 0 replies; 31+ messages in thread From: Tetsuo Handa @ 2020-11-13 10:49 UTC (permalink / raw) To: Valentina Manea, Shuah Khan, Shuah Khan, USB list; +Cc: syzkaller-bugs, syzbot syzbot reported that LSM module which inspects "struct socket"->sk experiences NULL pointer dereference because vdev->ud.tcp_socket was set to NULL while vdev->ud.tcp_tx kernel thread is still running. Is serialization between attach_store() and vhci_shutdown_connection() correct? [ 1151.164402][T23363] vhci_hcd: connection closed [ 1151.167346][ T240] vhci_hcd: stop threads [ 1151.178329][T26761] usb 17-1: new low-speed USB device number 2 using vhci_hcd [ 1151.181245][ T240] vhci_hcd: release socket [ 1151.195480][T23364] general protection fault, probably for non-canonical address 0xdffffc0000000003: 0000 [#1] PREEMPT SMP KASAN [ 1151.207194][T23364] KASAN: null-ptr-deref in range [0x0000000000000018-0x000000000000001f] [ 1151.215600][T23364] CPU: 0 PID: 23364 Comm: vhci_tx Not tainted 5.10.0-rc2-syzkaller #0 [ 1151.223740][T23364] Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 [ 1151.233909][T23364] RIP: 0010:tomoyo_socket_sendmsg_permission+0x80/0x340 [ 1151.240845][T23364] Code: c7 40 0c f3 f3 f3 f3 65 48 8b 04 25 28 00 00 00 48 89 84 24 a0 00 00 00 31 c0 e8 0b 70 f1 fd 48 8d 7d 18 48 89 f8 48 c1 e8 03 <42> 80 3c 20 00 0f 85 7d 02 00 00 48 b8 00 00 00 00 00 fc ff df 4c [ 1151.260448][T23364] RSP: 0018:ffffc90016f77ad0 EFLAGS: 00010206 [ 1151.260802][ T240] vhci_hcd: disconnect device static void vhci_shutdown_connection(struct usbip_device *ud) { struct vhci_device *vdev = container_of(ud, struct vhci_device, ud); /* need this? see stub_dev.c */ if (ud->tcp_socket) { pr_debug("shutdown tcp_socket %d\n", ud->sockfd); kernel_sock_shutdown(ud->tcp_socket, SHUT_RDWR); } /* kill threads related to this sdev */ if (vdev->ud.tcp_rx) { kthread_stop_put(vdev->ud.tcp_rx); vdev->ud.tcp_rx = NULL; } if (vdev->ud.tcp_tx) { kthread_stop_put(vdev->ud.tcp_tx); vdev->ud.tcp_tx = NULL; } pr_info("stop threads\n"); /* active connection is closed */ if (vdev->ud.tcp_socket) { sockfd_put(vdev->ud.tcp_socket); vdev->ud.tcp_socket = NULL; vdev->ud.sockfd = -1; } pr_info("release socket\n"); On 2020/11/13 16:21, syzbot wrote: > Hello, > > 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. > > IMPORTANT: if you fix the issue, please add the following tag to the commit: > Reported-by: syzbot+95ce4b142579611ef0a9@syzkaller.appspotmail.com > > general protection fault, probably for non-canonical address 0xdffffc0000000003: 0000 [#1] PREEMPT SMP KASAN > KASAN: null-ptr-deref in range [0x0000000000000018-0x000000000000001f] > CPU: 0 PID: 23364 Comm: vhci_tx Not tainted 5.10.0-rc2-syzkaller #0 > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 > RIP: 0010:tomoyo_socket_sendmsg_permission+0x80/0x340 security/tomoyo/network.c:762 > Code: c7 40 0c f3 f3 f3 f3 65 48 8b 04 25 28 00 00 00 48 89 84 24 a0 00 00 00 31 c0 e8 0b 70 f1 fd 48 8d 7d 18 48 89 f8 48 c1 e8 03 <42> 80 3c 20 00 0f 85 7d 02 00 00 48 b8 00 00 00 00 00 fc ff df 4c > RSP: 0018:ffffc90016f77ad0 EFLAGS: 00010206 > RAX: 0000000000000003 RBX: 1ffff92002deef5b RCX: ffffffff83b3789e > RDX: ffff8880186c8000 RSI: ffffffff837e9945 RDI: 0000000000000018 > RBP: 0000000000000000 R08: 0000000000000030 R09: ffffc90016f77d97 > R10: 0000000000000000 R11: 0000000000000000 R12: dffffc0000000000 > R13: ffffc90016f77ce8 R14: ffffc90016f77ce8 R15: 0000000000000000 > FS: 0000000000000000(0000) GS:ffff8880b9e00000(0000) knlGS:0000000000000000 > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > CR2: 0000562623b70868 CR3: 000000002af19000 CR4: 00000000001526f0 > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 > Call Trace: > security_socket_sendmsg+0x50/0xb0 security/security.c:2140 > sock_sendmsg+0x3f/0x120 net/socket.c:668 > vhci_send_cmd_submit+0x897/0xf90 drivers/usb/usbip/vhci_tx.c:139 > vhci_tx_loop+0xfc/0x370 drivers/usb/usbip/vhci_tx.c:241 > kthread+0x3af/0x4a0 kernel/kthread.c:292 > ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:296 > Modules linked in: > ---[ end trace 535369ec3e6ef304 ]--- > RIP: 0010:tomoyo_socket_sendmsg_permission+0x80/0x340 security/tomoyo/network.c:762 > Code: c7 40 0c f3 f3 f3 f3 65 48 8b 04 25 28 00 00 00 48 89 84 24 a0 00 00 00 31 c0 e8 0b 70 f1 fd 48 8d 7d 18 48 89 f8 48 c1 e8 03 <42> 80 3c 20 00 0f 85 7d 02 00 00 48 b8 00 00 00 00 00 fc ff df 4c > RSP: 0018:ffffc90016f77ad0 EFLAGS: 00010206 > RAX: 0000000000000003 RBX: 1ffff92002deef5b RCX: ffffffff83b3789e > RDX: ffff8880186c8000 RSI: ffffffff837e9945 RDI: 0000000000000018 > RBP: 0000000000000000 R08: 0000000000000030 R09: ffffc90016f77d97 > R10: 0000000000000000 R11: 0000000000000000 R12: dffffc0000000000 > R13: ffffc90016f77ce8 R14: ffffc90016f77ce8 R15: 0000000000000000 > FS: 0000000000000000(0000) GS:ffff8880b9e00000(0000) knlGS:0000000000000000 > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > CR2: 00007f0c3a930740 CR3: 000000002af19000 CR4: 00000000001526f0 > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 > > > --- > This report is generated by a bot. It may contain errors. > See https://goo.gl/tpsmEJ for more information about syzbot. > syzbot engineers can be reached at syzkaller@googlegroups.com. > > syzbot will keep track of this issue. See: > https://goo.gl/tpsmEJ#status for how to communicate with syzbot. > ^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH] usb: usbip: serialize attach/detach operations [not found] <000000000000647eff05b3f7e0d4@google.com> 2020-11-13 10:49 ` general protection fault in tomoyo_socket_sendmsg_permission Tetsuo Handa @ 2021-02-19 0:33 ` Tetsuo Handa 2021-02-19 9:47 ` [PATCH (repost)] " Tetsuo Handa 1 sibling, 1 reply; 31+ messages in thread From: Tetsuo Handa @ 2021-02-19 0:33 UTC (permalink / raw) To: Valentina Manea, Shuah Khan, Greg Kroah-Hartman Cc: Arnd Bergmann, linux-usb, Tetsuo Handa 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 tx_thread/rx_thread 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 tcp_socket = NULL in vhci_device_reset(). Since tx_thread/rx_thread depends on tcp_socket != NULL whereas tcp_socket and tx_thread/rx_thread are assigned at the same time, it is never safe to reset 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 tx_thread/rx_thread can be created (which will later cause a crash like [2]) and socket's refcount 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(-) 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); -- 2.18.4 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH (repost)] usb: usbip: serialize attach/detach operations 2021-02-19 0:33 ` [PATCH] usb: usbip: serialize attach/detach operations Tetsuo Handa @ 2021-02-19 9:47 ` Tetsuo Handa 2021-02-19 15:08 ` [PATCH v2] " Tetsuo Handa 0 siblings, 1 reply; 31+ messages in thread From: Tetsuo Handa @ 2021-02-19 9:47 UTC (permalink / raw) To: Valentina Manea, Shuah Khan, Greg Kroah-Hartman Cc: Arnd Bergmann, linux-usb, Tetsuo Handa 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(-) 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); -- 2.18.4 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v2] usb: usbip: serialize attach/detach operations 2021-02-19 9:47 ` [PATCH (repost)] " Tetsuo Handa @ 2021-02-19 15:08 ` Tetsuo Handa 2021-02-19 15:53 ` Greg Kroah-Hartman 0 siblings, 1 reply; 31+ messages in thread From: Tetsuo Handa @ 2021-02-19 15:08 UTC (permalink / raw) To: Valentina Manea, Shuah Khan, Greg Kroah-Hartman Cc: Arnd Bergmann, linux-usb, Tetsuo Handa 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(-) 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..f80098c3dd10 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(tx); + 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); -- 2.18.4 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH v2] usb: usbip: serialize attach/detach operations 2021-02-19 15:08 ` [PATCH v2] " Tetsuo Handa @ 2021-02-19 15:53 ` Greg Kroah-Hartman 2021-02-19 16:00 ` Shuah Khan 0 siblings, 1 reply; 31+ messages in thread From: Greg Kroah-Hartman @ 2021-02-19 15:53 UTC (permalink / raw) To: Tetsuo Handa; +Cc: Valentina Manea, Shuah Khan, Arnd Bergmann, linux-usb 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2] usb: usbip: serialize attach/detach operations 2021-02-19 15:53 ` Greg Kroah-Hartman @ 2021-02-19 16:00 ` Shuah Khan 2021-02-20 1:10 ` Tetsuo Handa 0 siblings, 1 reply; 31+ messages in thread From: Shuah Khan @ 2021-02-19 16:00 UTC (permalink / raw) To: Greg Kroah-Hartman, Tetsuo Handa Cc: Valentina Manea, Shuah Khan, Arnd Bergmann, linux-usb, Shuah Khan 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2] usb: usbip: serialize attach/detach operations 2021-02-19 16:00 ` Shuah Khan @ 2021-02-20 1:10 ` Tetsuo Handa 2021-02-20 6:58 ` Greg Kroah-Hartman 0 siblings, 1 reply; 31+ messages in thread From: Tetsuo Handa @ 2021-02-20 1:10 UTC (permalink / raw) To: Shuah Khan, Greg Kroah-Hartman Cc: Valentina Manea, Shuah Khan, Arnd Bergmann, linux-usb 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# ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2] usb: usbip: serialize attach/detach operations 2021-02-20 1:10 ` Tetsuo Handa @ 2021-02-20 6:58 ` Greg Kroah-Hartman 2021-02-20 9:51 ` Tetsuo Handa 2021-02-23 1:59 ` [PATCH v3] " Tetsuo Handa 0 siblings, 2 replies; 31+ messages in thread From: Greg Kroah-Hartman @ 2021-02-20 6:58 UTC (permalink / raw) To: Tetsuo Handa Cc: Shuah Khan, Valentina Manea, Shuah Khan, Arnd Bergmann, linux-usb 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2] usb: usbip: serialize attach/detach operations 2021-02-20 6:58 ` Greg Kroah-Hartman @ 2021-02-20 9:51 ` Tetsuo Handa 2021-02-22 15:34 ` Shuah Khan 2021-02-23 1:59 ` [PATCH v3] " Tetsuo Handa 1 sibling, 1 reply; 31+ messages in thread From: Tetsuo Handa @ 2021-02-20 9:51 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Shuah Khan, Valentina Manea, Shuah Khan, Arnd Bergmann, linux-usb 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2] usb: usbip: serialize attach/detach operations 2021-02-20 9:51 ` Tetsuo Handa @ 2021-02-22 15:34 ` Shuah Khan 2021-02-23 1:51 ` Tetsuo Handa 0 siblings, 1 reply; 31+ messages in thread From: Shuah Khan @ 2021-02-22 15:34 UTC (permalink / raw) To: Tetsuo Handa, Greg Kroah-Hartman Cc: Valentina Manea, Shuah Khan, Arnd Bergmann, linux-usb, Shuah Khan 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2] usb: usbip: serialize attach/detach operations 2021-02-22 15:34 ` Shuah Khan @ 2021-02-23 1:51 ` Tetsuo Handa 0 siblings, 0 replies; 31+ messages in thread From: Tetsuo Handa @ 2021-02-23 1:51 UTC (permalink / raw) To: Shuah Khan, Greg Kroah-Hartman Cc: Valentina Manea, Shuah Khan, Arnd Bergmann, linux-usb 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v3] usb: usbip: serialize attach/detach operations 2021-02-20 6:58 ` Greg Kroah-Hartman 2021-02-20 9:51 ` Tetsuo Handa @ 2021-02-23 1:59 ` Tetsuo Handa 2021-02-26 0:00 ` Shuah Khan 1 sibling, 1 reply; 31+ messages in thread From: Tetsuo Handa @ 2021-02-23 1:59 UTC (permalink / raw) To: Valentina Manea, Shuah Khan, Greg Kroah-Hartman Cc: Arnd Bergmann, linux-usb, Tetsuo Handa, Julia Lawall, The kernel test robot 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") --- Changes since v1: Tetsuo Handa found that the PTR_ERR() access in usbip_prepare_threads() in v1 was wrong, and fixed it in v2. Changes since v2: The kernel test robot <lkp@intel.com> found the same error using scripts/coccinelle/tests/odd_ptr_err.cocci and Julia Lawall <julia.lawall@inria.fr> sent us a patch, but nothing changed because that error was already fixed in v2. 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(-) 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..f80098c3dd10 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(tx); + 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); -- 2.25.1 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH v3] usb: usbip: serialize attach/detach operations 2021-02-23 1:59 ` [PATCH v3] " Tetsuo Handa @ 2021-02-26 0:00 ` Shuah Khan 2021-02-26 0:10 ` Tetsuo Handa 0 siblings, 1 reply; 31+ messages in thread From: Shuah Khan @ 2021-02-26 0:00 UTC (permalink / raw) To: Tetsuo Handa, Valentina Manea, Shuah Khan, Greg Kroah-Hartman, Shuah Khan Cc: Arnd Bergmann, linux-usb, Julia Lawall, The kernel test robot On 2/22/21 6:59 PM, 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") > --- > Changes since v1: > > Tetsuo Handa found that the PTR_ERR() access in usbip_prepare_threads() > in v1 was wrong, and fixed it in v2. > > Changes since v2: > > The kernel test robot <lkp@intel.com> found the same error using > scripts/coccinelle/tests/odd_ptr_err.cocci and > Julia Lawall <julia.lawall@inria.fr> sent us a patch, but nothing > changed because that error was already fixed in v2. > > 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(-) > This patch makes changes to 3 different drivers. Please split these patches. Makes it easier to revert or fix them. Patch1 could add common routines and use them in stud_dev and vudc Same for usbip_event_lock_killable() and usbip_event_unlock(). Introduce them in a separate patch. __usbip_sockfd_store() could be made common to stub_dev and vudc similar to usbip_prepare_threads() and usbip_unprepare_threads() It will lot easier to review if this large patch is split into smaller patches. thanks, -- Shuah ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v3] usb: usbip: serialize attach/detach operations 2021-02-26 0:00 ` Shuah Khan @ 2021-02-26 0:10 ` Tetsuo Handa 2021-02-26 15:04 ` Shuah Khan 0 siblings, 1 reply; 31+ messages in thread From: Tetsuo Handa @ 2021-02-26 0:10 UTC (permalink / raw) To: Shuah Khan, Valentina Manea, Shuah Khan, Greg Kroah-Hartman Cc: Arnd Bergmann, linux-usb, Julia Lawall, The kernel test robot On 2021/02/26 9:00, Shuah Khan wrote: > This patch makes changes to 3 different drivers. Please split these > patches. Makes it easier to revert or fix them. > > Patch1 could add common routines and use them in stud_dev and vudc > Same for usbip_event_lock_killable() and usbip_event_unlock(). > Introduce them in a separate patch. > > __usbip_sockfd_store() could be made common to stub_dev and vudc > similar to usbip_prepare_threads() and usbip_unprepare_threads() > > It will lot easier to review if this large patch is split into > smaller patches. Then, can we apply https://lkml.kernel.org/r/20210205135707.4574-1-penguin-kernel@I-love.SAKURA.ne.jp as the first patch? It is very easy to review and apply and backport to stable. ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v3] usb: usbip: serialize attach/detach operations 2021-02-26 0:10 ` Tetsuo Handa @ 2021-02-26 15:04 ` Shuah Khan 2021-03-04 15:24 ` [PATCH v4 00/12] " Tetsuo Handa 0 siblings, 1 reply; 31+ messages in thread From: Shuah Khan @ 2021-02-26 15:04 UTC (permalink / raw) To: Tetsuo Handa, Valentina Manea, Shuah Khan, Greg Kroah-Hartman Cc: Arnd Bergmann, linux-usb, Julia Lawall, The kernel test robot, Shuah Khan On 2/25/21 5:10 PM, Tetsuo Handa wrote: > On 2021/02/26 9:00, Shuah Khan wrote: >> This patch makes changes to 3 different drivers. Please split these >> patches. Makes it easier to revert or fix them. >> >> Patch1 could add common routines and use them in stud_dev and vudc >> Same for usbip_event_lock_killable() and usbip_event_unlock(). >> Introduce them in a separate patch. >> >> __usbip_sockfd_store() could be made common to stub_dev and vudc >> similar to usbip_prepare_threads() and usbip_unprepare_threads() >> >> It will lot easier to review if this large patch is split into >> smaller patches. > > Then, can we apply > https://lkml.kernel.org/r/20210205135707.4574-1-penguin-kernel@I-love.SAKURA.ne.jp > as the first patch? It is very easy to review and apply and backport to stable. > Same concerns as before about error path handling. I have a patch in the works that is simpler and solves the crashes you are seeing and addresses my error path concerns. I will send it out later on today. Thanks for all the heavy lifting. Much appreciated. thanks, -- Shuah ^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v4 00/12] usb: usbip: serialize attach/detach operations 2021-02-26 15:04 ` Shuah Khan @ 2021-03-04 15:24 ` Tetsuo Handa 2021-03-04 15:24 ` [PATCH v4 01/12] usb: usbip: introduce usbip_event_mutex for serialization Tetsuo Handa ` (12 more replies) 0 siblings, 13 replies; 31+ messages in thread From: Tetsuo Handa @ 2021-03-04 15:24 UTC (permalink / raw) To: Valentina Manea, Shuah Khan, Greg Kroah-Hartman Cc: Arnd Bergmann, linux-usb, Tetsuo Handa On 2021/02/27 0:04, Shuah Khan wrote: > I will send it out later on today. Since I haven't received it, here comes an update. Changes since v1: Tetsuo Handa found that the PTR_ERR() access in usbip_prepare_threads() in v1 was wrong, and fixed it in v2. Changes since v2: The kernel test robot <lkp@intel.com> found the same error using scripts/coccinelle/tests/odd_ptr_err.cocci and Julia Lawall <julia.lawall@inria.fr> sent us a patch, but nothing changed because that error was already fixed in v2. Changes since v3: Reduce lines changed in drivers/usb/usbip/stub_dev.c . Split into 12 patches. Tetsuo Handa (12): usb: usbip: introduce usbip_event_mutex for serialization usb: usbip: vhci: serialize attach_store()/detach_store() against event_handler() usb: usbip: vudc: serialize usbip_sockfd_store() against event_handler() usb: usbip: stub: serialize usbip_sockfd_store() against event_handler() usb: usbip: don't reset tcp_socket at vhci_device_reset() usb: usbip: fix error handling of kthread_get_run() usb: usbip: preallocate kernel threads for consistent attach operation usb: usbip: check that stream socket is used usb: usbip: vhci: add automatic recovery to attach_store() usb: usbip: vudc: add automatic recovery to usbip_sockfd_store() usb: usbip: stub: add automatic recovery to usbip_sockfd_store() usb: usbip: remove unused kthread_get_run() drivers/usb/usbip/stub_dev.c | 44 ++++++++++++++----------- 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, 176 insertions(+), 69 deletions(-) -- 2.18.4 ^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v4 01/12] usb: usbip: introduce usbip_event_mutex for serialization 2021-03-04 15:24 ` [PATCH v4 00/12] " Tetsuo Handa @ 2021-03-04 15:24 ` Tetsuo Handa 2021-03-04 15:24 ` [PATCH v4 02/12] usb: usbip: vhci: serialize attach_store()/detach_store() against event_handler() Tetsuo Handa ` (11 subsequent siblings) 12 siblings, 0 replies; 31+ messages in thread From: Tetsuo Handa @ 2021-03-04 15:24 UTC (permalink / raw) To: Valentina Manea, Shuah Khan, Greg Kroah-Hartman Cc: Arnd Bergmann, linux-usb, Tetsuo Handa syzbot is reporting a NULL pointer dereference [1] because usbip modules do not use serialization when attaching/detaching. As a preparation for serializing attach/detach operations, introduce a global mutex, export it via usbip_event_lock_killable()/usbip_event_unlock(), and use it in event_handler(). We don't need to use per a device mutex because event_handler() is processed by a singlethreaded workqueue and ud->tcp_{tx,rx} must not wait for event_handler() to flush. [1] https://syzkaller.appspot.com/bug?extid=95ce4b142579611ef0a9 Reported-by: syzbot <syzbot+95ce4b142579611ef0a9@syzkaller.appspotmail.com> Tested-by: syzbot <syzbot+a93fba6d384346a761e3@syzkaller.appspotmail.com> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> --- drivers/usb/usbip/usbip_common.h | 2 ++ drivers/usb/usbip/usbip_event.c | 15 +++++++++++++++ 2 files changed, 17 insertions(+) diff --git a/drivers/usb/usbip/usbip_common.h b/drivers/usb/usbip/usbip_common.h index a7dd6c66aee5..f8e16c75b154 100644 --- a/drivers/usb/usbip/usbip_common.h +++ b/drivers/usb/usbip/usbip_common.h @@ -327,6 +327,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); } } -- 2.18.4 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v4 02/12] usb: usbip: vhci: serialize attach_store()/detach_store() against event_handler() 2021-03-04 15:24 ` [PATCH v4 00/12] " Tetsuo Handa 2021-03-04 15:24 ` [PATCH v4 01/12] usb: usbip: introduce usbip_event_mutex for serialization Tetsuo Handa @ 2021-03-04 15:24 ` Tetsuo Handa 2021-03-04 15:24 ` [PATCH v4 03/12] usb: usbip: vudc: serialize usbip_sockfd_store() " Tetsuo Handa ` (10 subsequent siblings) 12 siblings, 0 replies; 31+ messages in thread From: Tetsuo Handa @ 2021-03-04 15:24 UTC (permalink / raw) To: Valentina Manea, Shuah Khan, Greg Kroah-Hartman Cc: Arnd Bergmann, linux-usb, Tetsuo Handa syzbot is reporting a NULL pointer dereference at sock_sendmsg() [1], 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. Since I don't want to replace "return" with "goto" in order to call usbip_event_unlock(), wrap whole functions in order to keep changes minimal. [1] https://syzkaller.appspot.com/bug?extid=95ce4b142579611ef0a9 Reported-by: syzbot <syzbot+95ce4b142579611ef0a9@syzkaller.appspotmail.com> Tested-by: syzbot <syzbot+a93fba6d384346a761e3@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> --- drivers/usb/usbip/vhci_sysfs.c | 30 ++++++++++++++++++++++++++---- 1 file changed, 26 insertions(+), 4 deletions(-) diff --git a/drivers/usb/usbip/vhci_sysfs.c b/drivers/usb/usbip/vhci_sysfs.c index 96e5371dc335..031e21aad96a 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,8 +311,8 @@ 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; int sockfd = 0; @@ -396,6 +407,17 @@ static ssize_t attach_store(struct device *dev, struct device_attribute *attr, 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 -- 2.18.4 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v4 03/12] usb: usbip: vudc: serialize usbip_sockfd_store() against event_handler() 2021-03-04 15:24 ` [PATCH v4 00/12] " Tetsuo Handa 2021-03-04 15:24 ` [PATCH v4 01/12] usb: usbip: introduce usbip_event_mutex for serialization Tetsuo Handa 2021-03-04 15:24 ` [PATCH v4 02/12] usb: usbip: vhci: serialize attach_store()/detach_store() against event_handler() Tetsuo Handa @ 2021-03-04 15:24 ` Tetsuo Handa 2021-03-04 15:24 ` [PATCH v4 04/12] usb: usbip: stub: " Tetsuo Handa ` (9 subsequent siblings) 12 siblings, 0 replies; 31+ messages in thread From: Tetsuo Handa @ 2021-03-04 15:24 UTC (permalink / raw) To: Valentina Manea, Shuah Khan, Greg Kroah-Hartman Cc: Arnd Bergmann, linux-usb, Tetsuo Handa usbip_sockfd_store() must perform if (udc->ud.status != SDEV_ST_AVAILABLE) { /* misc assignments for attach operation */ udc->ud.status = SDEV_ST_USED; } under a lock, or multiple ud->tcp_{tx,rx} are created (which will later cause a crash like [1]) and refcount on ud->tcp_socket is leaked when usbip_sockfd_store() is concurrently called. Since I don't want to replace "return" with "goto" in order to call usbip_event_unlock(), wrap whole function in order to keep changes minimal. [1] https://syzkaller.appspot.com/bug?extid=95ce4b142579611ef0a9 Reported-by: syzbot <syzbot+95ce4b142579611ef0a9@syzkaller.appspotmail.com> Tested-by: syzbot <syzbot+a93fba6d384346a761e3@syzkaller.appspotmail.com> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> --- drivers/usb/usbip/vudc_sysfs.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/drivers/usb/usbip/vudc_sysfs.c b/drivers/usb/usbip/vudc_sysfs.c index 100f680c572a..ebf6a127fcc3 100644 --- a/drivers/usb/usbip/vudc_sysfs.c +++ b/drivers/usb/usbip/vudc_sysfs.c @@ -90,8 +90,8 @@ 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; @@ -184,6 +184,17 @@ static ssize_t usbip_sockfd_store(struct device *dev, struct device_attribute *a 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); static ssize_t usbip_status_show(struct device *dev, -- 2.18.4 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v4 04/12] usb: usbip: stub: serialize usbip_sockfd_store() against event_handler() 2021-03-04 15:24 ` [PATCH v4 00/12] " Tetsuo Handa ` (2 preceding siblings ...) 2021-03-04 15:24 ` [PATCH v4 03/12] usb: usbip: vudc: serialize usbip_sockfd_store() " Tetsuo Handa @ 2021-03-04 15:24 ` Tetsuo Handa 2021-03-04 15:24 ` [PATCH v4 05/12] usb: usbip: don't reset tcp_socket at vhci_device_reset() Tetsuo Handa ` (8 subsequent siblings) 12 siblings, 0 replies; 31+ messages in thread From: Tetsuo Handa @ 2021-03-04 15:24 UTC (permalink / raw) To: Valentina Manea, Shuah Khan, Greg Kroah-Hartman Cc: Arnd Bergmann, linux-usb, Tetsuo Handa usbip_sockfd_store() must perform if (sdev->ud.status != SDEV_ST_AVAILABLE) { /* misc assignments for attach operation */ sdev->ud.status = SDEV_ST_USED; } under a lock, or multiple ud->tcp_{tx,rx} are created (which will later cause a crash like [1]) and refcount on ud->tcp_socket is leaked when usbip_sockfd_store() is concurrently called. Since I don't want to replace "return" with "goto" in order to call usbip_event_unlock(), wrap whole function in order to keep changes minimal. [1] https://syzkaller.appspot.com/bug?extid=95ce4b142579611ef0a9 Reported-by: syzbot <syzbot+95ce4b142579611ef0a9@syzkaller.appspotmail.com> Tested-by: syzbot <syzbot+a93fba6d384346a761e3@syzkaller.appspotmail.com> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> --- drivers/usb/usbip/stub_dev.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/drivers/usb/usbip/stub_dev.c b/drivers/usb/usbip/stub_dev.c index 2305d425e6c9..cea5663d1994 100644 --- a/drivers/usb/usbip/stub_dev.c +++ b/drivers/usb/usbip/stub_dev.c @@ -39,8 +39,8 @@ 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; @@ -104,6 +104,17 @@ static ssize_t usbip_sockfd_store(struct device *dev, struct device_attribute *a 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); static struct attribute *usbip_attrs[] = { -- 2.18.4 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v4 05/12] usb: usbip: don't reset tcp_socket at vhci_device_reset() 2021-03-04 15:24 ` [PATCH v4 00/12] " Tetsuo Handa ` (3 preceding siblings ...) 2021-03-04 15:24 ` [PATCH v4 04/12] usb: usbip: stub: " Tetsuo Handa @ 2021-03-04 15:24 ` Tetsuo Handa 2021-03-04 15:24 ` [PATCH v4 06/12] usb: usbip: fix error handling of kthread_get_run() Tetsuo Handa ` (7 subsequent siblings) 12 siblings, 0 replies; 31+ messages in thread From: Tetsuo Handa @ 2021-03-04 15:24 UTC (permalink / raw) To: Valentina Manea, Shuah Khan, Greg Kroah-Hartman Cc: Arnd Bergmann, linux-usb, Tetsuo Handa 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(). And actually, this assignment had been a dead code because any event which involves USBIP_EH_RESET also involves USBIP_EH_SHUTDOWN. Tested-by: syzbot <syzbot+a93fba6d384346a761e3@syzkaller.appspotmail.com> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> --- drivers/usb/usbip/vhci_hcd.c | 6 ------ 1 file changed, 6 deletions(-) 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); -- 2.18.4 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v4 06/12] usb: usbip: fix error handling of kthread_get_run() 2021-03-04 15:24 ` [PATCH v4 00/12] " Tetsuo Handa ` (4 preceding siblings ...) 2021-03-04 15:24 ` [PATCH v4 05/12] usb: usbip: don't reset tcp_socket at vhci_device_reset() Tetsuo Handa @ 2021-03-04 15:24 ` Tetsuo Handa 2021-03-04 15:24 ` [PATCH v4 07/12] usb: usbip: preallocate kernel threads for consistent attach operation Tetsuo Handa ` (6 subsequent siblings) 12 siblings, 0 replies; 31+ messages in thread From: Tetsuo Handa @ 2021-03-04 15:24 UTC (permalink / raw) To: Valentina Manea, Shuah Khan, Greg Kroah-Hartman Cc: Arnd Bergmann, linux-usb, Tetsuo Handa 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. Therefore, make kthread_get_run() return NULL when kthread_create() failed. This patch only fixes error pointer dereference problem, for the code prior to that commit was already providing manual recovery interface. [1] https://syzkaller.appspot.com/bug?extid=a93fba6d384346a761e3 Reported-by: syzbot <syzbot+a93fba6d384346a761e3@syzkaller.appspotmail.com> Reported-by: syzbot <syzbot+bf1a360e305ee719e364@syzkaller.appspotmail.com> Tested-by: syzbot <syzbot+a93fba6d384346a761e3@syzkaller.appspotmail.com> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Fixes: 9720b4bc76a83807 ("staging/usbip: convert to kthread") Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> --- drivers/usb/usbip/usbip_common.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/usb/usbip/usbip_common.h b/drivers/usb/usbip/usbip_common.h index f8e16c75b154..6e2a80b1633d 100644 --- a/drivers/usb/usbip/usbip_common.h +++ b/drivers/usb/usbip/usbip_common.h @@ -291,6 +291,8 @@ struct usbip_device { if (!IS_ERR(__k)) { \ get_task_struct(__k); \ wake_up_process(__k); \ + } else { \ + __k = NULL; \ } \ __k; \ }) -- 2.18.4 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v4 07/12] usb: usbip: preallocate kernel threads for consistent attach operation 2021-03-04 15:24 ` [PATCH v4 00/12] " Tetsuo Handa ` (5 preceding siblings ...) 2021-03-04 15:24 ` [PATCH v4 06/12] usb: usbip: fix error handling of kthread_get_run() Tetsuo Handa @ 2021-03-04 15:24 ` Tetsuo Handa 2021-03-04 15:24 ` [PATCH v4 08/12] usb: usbip: check that stream socket is used Tetsuo Handa ` (5 subsequent siblings) 12 siblings, 0 replies; 31+ messages in thread From: Tetsuo Handa @ 2021-03-04 15:24 UTC (permalink / raw) To: Valentina Manea, Shuah Khan, Greg Kroah-Hartman Cc: Arnd Bergmann, linux-usb, Tetsuo Handa All usbip modules are doing (1) socket lookup for ud->tcp_socket (2) kthread_create() for ud->tcp_{tx,rx} (3) get_task_struct() on ud->tcp_{tx,rx} if kthread_create() succeeded (4) wake_up_process() on ud->tcp_{tx,rx} if kthread_create() succeeded where (1) (2) (3) can be grouped as a common function. Since we can't overwrite ud->tcp_socket and ud->tcp_{tx,rx} without checking ud->status, define usbip_thread_info for holding temporary resources and introduce usbip_prepare_threads()/usbip_unprepare_threads() which respectively allocates/frees these temporary resources. (4) is done by usbip_prepare_threads() callers after checking ud->status. Tested-by: syzbot <syzbot+a93fba6d384346a761e3@syzkaller.appspotmail.com> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> --- drivers/usb/usbip/usbip_common.c | 50 ++++++++++++++++++++++++++++++++ drivers/usb/usbip/usbip_common.h | 12 ++++++++ 2 files changed, 62 insertions(+) diff --git a/drivers/usb/usbip/usbip_common.c b/drivers/usb/usbip/usbip_common.c index 2ab99244bc31..9f677c5a74e8 100644 --- a/drivers/usb/usbip/usbip_common.c +++ b/drivers/usb/usbip/usbip_common.c @@ -748,6 +748,56 @@ 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; + /* 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(tx); + 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 6e2a80b1633d..6e5394dba2ec 100644 --- a/drivers/usb/usbip/usbip_common.h +++ b/drivers/usb/usbip/usbip_common.h @@ -316,6 +316,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); -- 2.18.4 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v4 08/12] usb: usbip: check that stream socket is used 2021-03-04 15:24 ` [PATCH v4 00/12] " Tetsuo Handa ` (6 preceding siblings ...) 2021-03-04 15:24 ` [PATCH v4 07/12] usb: usbip: preallocate kernel threads for consistent attach operation Tetsuo Handa @ 2021-03-04 15:24 ` Tetsuo Handa 2021-03-04 15:24 ` [PATCH v4 09/12] usb: usbip: vhci: add automatic recovery to attach_store() Tetsuo Handa ` (4 subsequent siblings) 12 siblings, 0 replies; 31+ messages in thread From: Tetsuo Handa @ 2021-03-04 15:24 UTC (permalink / raw) To: Valentina Manea, Shuah Khan, Greg Kroah-Hartman Cc: Arnd Bergmann, linux-usb, Tetsuo Handa 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. Tested-by: syzbot <syzbot+a93fba6d384346a761e3@syzkaller.appspotmail.com> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> --- drivers/usb/usbip/usbip_common.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/drivers/usb/usbip/usbip_common.c b/drivers/usb/usbip/usbip_common.c index 9f677c5a74e8..f80098c3dd10 100644 --- a/drivers/usb/usbip/usbip_common.c +++ b/drivers/usb/usbip/usbip_common.c @@ -762,6 +762,11 @@ int usbip_prepare_threads(struct usbip_thread_info *uti, 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)) { -- 2.18.4 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v4 09/12] usb: usbip: vhci: add automatic recovery to attach_store() 2021-03-04 15:24 ` [PATCH v4 00/12] " Tetsuo Handa ` (7 preceding siblings ...) 2021-03-04 15:24 ` [PATCH v4 08/12] usb: usbip: check that stream socket is used Tetsuo Handa @ 2021-03-04 15:24 ` Tetsuo Handa 2021-03-04 15:24 ` [PATCH v4 10/12] usb: usbip: vudc: add automatic recovery to usbip_sockfd_store() Tetsuo Handa ` (3 subsequent siblings) 12 siblings, 0 replies; 31+ messages in thread From: Tetsuo Handa @ 2021-03-04 15:24 UTC (permalink / raw) To: Valentina Manea, Shuah Khan, Greg Kroah-Hartman Cc: Arnd Bergmann, linux-usb, Tetsuo Handa Handle kthread_create() failure by replacing kthread_get_run() with usbip_prepare_threads()/usbip_unprepare_threads(). Tested-by: syzbot <syzbot+a93fba6d384346a761e3@syzkaller.appspotmail.com> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> --- drivers/usb/usbip/vhci_sysfs.c | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/drivers/usb/usbip/vhci_sysfs.c b/drivers/usb/usbip/vhci_sysfs.c index 031e21aad96a..234a133f7333 100644 --- a/drivers/usb/usbip/vhci_sysfs.c +++ b/drivers/usb/usbip/vhci_sysfs.c @@ -314,7 +314,7 @@ static int valid_args(__u32 *pdev_nr, __u32 *rhport, 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; @@ -358,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 */ @@ -374,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); /* @@ -392,7 +392,9 @@ 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; usbip_kcov_handle_init(&vdev->ud); @@ -400,8 +402,8 @@ static ssize_t __attach_store(struct device *dev, struct device_attribute *attr, 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); -- 2.18.4 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v4 10/12] usb: usbip: vudc: add automatic recovery to usbip_sockfd_store() 2021-03-04 15:24 ` [PATCH v4 00/12] " Tetsuo Handa ` (8 preceding siblings ...) 2021-03-04 15:24 ` [PATCH v4 09/12] usb: usbip: vhci: add automatic recovery to attach_store() Tetsuo Handa @ 2021-03-04 15:24 ` Tetsuo Handa 2021-03-04 15:24 ` [PATCH v4 11/12] usb: usbip: stub: " Tetsuo Handa ` (2 subsequent siblings) 12 siblings, 0 replies; 31+ messages in thread From: Tetsuo Handa @ 2021-03-04 15:24 UTC (permalink / raw) To: Valentina Manea, Shuah Khan, Greg Kroah-Hartman Cc: Arnd Bergmann, linux-usb, Tetsuo Handa Handle kthread_create() failure by replacing kthread_get_run() with usbip_prepare_threads()/usbip_unprepare_threads(). Tested-by: syzbot <syzbot+a93fba6d384346a761e3@syzkaller.appspotmail.com> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> --- drivers/usb/usbip/vudc_sysfs.c | 37 ++++++++++++++++------------------ 1 file changed, 17 insertions(+), 20 deletions(-) diff --git a/drivers/usb/usbip/vudc_sysfs.c b/drivers/usb/usbip/vudc_sysfs.c index ebf6a127fcc3..ff3cf225a4fa 100644 --- a/drivers/usb/usbip/vudc_sysfs.c +++ b/drivers/usb/usbip/vudc_sysfs.c @@ -96,8 +96,7 @@ static ssize_t __usbip_sockfd_store(struct device *dev, struct device_attribute 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 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 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,8 @@ static ssize_t __usbip_sockfd_store(struct device *dev, struct device_attribute 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, -- 2.18.4 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v4 11/12] usb: usbip: stub: add automatic recovery to usbip_sockfd_store() 2021-03-04 15:24 ` [PATCH v4 00/12] " Tetsuo Handa ` (9 preceding siblings ...) 2021-03-04 15:24 ` [PATCH v4 10/12] usb: usbip: vudc: add automatic recovery to usbip_sockfd_store() Tetsuo Handa @ 2021-03-04 15:24 ` Tetsuo Handa 2021-03-04 15:24 ` [PATCH v4 12/12] usb: usbip: remove unused kthread_get_run() Tetsuo Handa 2021-03-04 15:52 ` [PATCH v4 00/12] usb: usbip: serialize attach/detach operations Shuah Khan 12 siblings, 0 replies; 31+ messages in thread From: Tetsuo Handa @ 2021-03-04 15:24 UTC (permalink / raw) To: Valentina Manea, Shuah Khan, Greg Kroah-Hartman Cc: Arnd Bergmann, linux-usb, Tetsuo Handa Handle kthread_create() failure by replacing kthread_get_run() with usbip_prepare_threads()/usbip_unprepare_threads(). Tested-by: syzbot <syzbot+a93fba6d384346a761e3@syzkaller.appspotmail.com> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> --- drivers/usb/usbip/stub_dev.c | 29 +++++++++++++---------------- 1 file changed, 13 insertions(+), 16 deletions(-) diff --git a/drivers/usb/usbip/stub_dev.c b/drivers/usb/usbip/stub_dev.c index cea5663d1994..ff7d0cd5a557 100644 --- a/drivers/usb/usbip/stub_dev.c +++ b/drivers/usb/usbip/stub_dev.c @@ -44,7 +44,7 @@ static ssize_t __usbip_sockfd_store(struct device *dev, struct device_attribute { struct stub_device *sdev = dev_get_drvdata(dev); int sockfd = 0; - struct socket *socket; + struct usbip_thread_info uti = { }; int rv; if (!sdev) { @@ -57,7 +57,11 @@ static ssize_t __usbip_sockfd_store(struct device *dev, struct device_attribute return -EINVAL; if (sockfd != -1) { - int err; + 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"); @@ -68,24 +72,15 @@ static ssize_t __usbip_sockfd_store(struct device *dev, struct device_attribute goto err; } - 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"); @@ -102,6 +97,8 @@ static ssize_t __usbip_sockfd_store(struct device *dev, struct device_attribute err: spin_unlock_irq(&sdev->ud.lock); + if (uti.tcp_socket) + usbip_unprepare_threads(&uti); return -EINVAL; } static ssize_t usbip_sockfd_store(struct device *dev, struct device_attribute *attr, -- 2.18.4 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v4 12/12] usb: usbip: remove unused kthread_get_run() 2021-03-04 15:24 ` [PATCH v4 00/12] " Tetsuo Handa ` (10 preceding siblings ...) 2021-03-04 15:24 ` [PATCH v4 11/12] usb: usbip: stub: " Tetsuo Handa @ 2021-03-04 15:24 ` Tetsuo Handa 2021-03-04 15:52 ` [PATCH v4 00/12] usb: usbip: serialize attach/detach operations Shuah Khan 12 siblings, 0 replies; 31+ messages in thread From: Tetsuo Handa @ 2021-03-04 15:24 UTC (permalink / raw) To: Valentina Manea, Shuah Khan, Greg Kroah-Hartman Cc: Arnd Bergmann, linux-usb, Tetsuo Handa All kthread_get_run() users are now using usbip_prepare_threads(). Tested-by: syzbot <syzbot+a93fba6d384346a761e3@syzkaller.appspotmail.com> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> --- drivers/usb/usbip/usbip_common.h | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/drivers/usb/usbip/usbip_common.h b/drivers/usb/usbip/usbip_common.h index 6e5394dba2ec..71dae36c9e3b 100644 --- a/drivers/usb/usbip/usbip_common.h +++ b/drivers/usb/usbip/usbip_common.h @@ -284,19 +284,6 @@ struct usbip_device { #endif }; -#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); \ - } else { \ - __k = NULL; \ - } \ - __k; \ -}) - #define kthread_stop_put(k) \ do { \ kthread_stop(k); \ -- 2.18.4 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH v4 00/12] usb: usbip: serialize attach/detach operations 2021-03-04 15:24 ` [PATCH v4 00/12] " Tetsuo Handa ` (11 preceding siblings ...) 2021-03-04 15:24 ` [PATCH v4 12/12] usb: usbip: remove unused kthread_get_run() Tetsuo Handa @ 2021-03-04 15:52 ` Shuah Khan 2021-03-05 10:06 ` Tetsuo Handa 12 siblings, 1 reply; 31+ messages in thread From: Shuah Khan @ 2021-03-04 15:52 UTC (permalink / raw) To: Tetsuo Handa, Valentina Manea, Shuah Khan, Greg Kroah-Hartman Cc: Arnd Bergmann, linux-usb, Shuah Khan On 3/4/21 8:24 AM, Tetsuo Handa wrote: > On 2021/02/27 0:04, Shuah Khan wrote: >> I will send it out later on today. > > Since I haven't received it, here comes an update. > Thanks for you the update. I have a patch series that works well to go into stables. As I said, it uses a simpler approach and reduces the number of changes. I would rather go with what I am working on. These fixes have to go all the way into Linux 4.4. I will handle this series. thanks, -- Shuah ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v4 00/12] usb: usbip: serialize attach/detach operations 2021-03-04 15:52 ` [PATCH v4 00/12] usb: usbip: serialize attach/detach operations Shuah Khan @ 2021-03-05 10:06 ` Tetsuo Handa 2021-03-05 14:44 ` Shuah Khan 0 siblings, 1 reply; 31+ messages in thread From: Tetsuo Handa @ 2021-03-05 10:06 UTC (permalink / raw) To: Shuah Khan, Valentina Manea, Shuah Khan, Greg Kroah-Hartman Cc: Arnd Bergmann, linux-usb On 2021/03/05 0:52, Shuah Khan wrote: > As I said, it uses a simpler approach and reduces the number > of changes. I would rather go with what I am working on. Will you post it to ML so that people can review? ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v4 00/12] usb: usbip: serialize attach/detach operations 2021-03-05 10:06 ` Tetsuo Handa @ 2021-03-05 14:44 ` Shuah Khan 0 siblings, 0 replies; 31+ messages in thread From: Shuah Khan @ 2021-03-05 14:44 UTC (permalink / raw) To: Tetsuo Handa, Valentina Manea, Shuah Khan, Greg Kroah-Hartman Cc: Arnd Bergmann, linux-usb, Shuah Khan On 3/5/21 3:06 AM, Tetsuo Handa wrote: > On 2021/03/05 0:52, Shuah Khan wrote: >> As I said, it uses a simpler approach and reduces the number >> of changes. I would rather go with what I am working on. > > Will you post it to ML so that people can review? > Yes. All my patches go through mailing lists. :) thanks, -- Shuah ^ permalink raw reply [flat|nested] 31+ messages in thread
end of thread, other threads:[~2021-03-05 14:45 UTC | newest]
Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <000000000000647eff05b3f7e0d4@google.com>
2020-11-13 10:49 ` general protection fault in tomoyo_socket_sendmsg_permission Tetsuo Handa
2021-02-19 0:33 ` [PATCH] usb: usbip: serialize attach/detach operations Tetsuo Handa
2021-02-19 9:47 ` [PATCH (repost)] " Tetsuo Handa
2021-02-19 15:08 ` [PATCH v2] " Tetsuo Handa
2021-02-19 15:53 ` Greg Kroah-Hartman
2021-02-19 16:00 ` Shuah Khan
2021-02-20 1:10 ` Tetsuo Handa
2021-02-20 6:58 ` Greg Kroah-Hartman
2021-02-20 9:51 ` Tetsuo Handa
2021-02-22 15:34 ` Shuah Khan
2021-02-23 1:51 ` Tetsuo Handa
2021-02-23 1:59 ` [PATCH v3] " Tetsuo Handa
2021-02-26 0:00 ` Shuah Khan
2021-02-26 0:10 ` Tetsuo Handa
2021-02-26 15:04 ` Shuah Khan
2021-03-04 15:24 ` [PATCH v4 00/12] " Tetsuo Handa
2021-03-04 15:24 ` [PATCH v4 01/12] usb: usbip: introduce usbip_event_mutex for serialization Tetsuo Handa
2021-03-04 15:24 ` [PATCH v4 02/12] usb: usbip: vhci: serialize attach_store()/detach_store() against event_handler() Tetsuo Handa
2021-03-04 15:24 ` [PATCH v4 03/12] usb: usbip: vudc: serialize usbip_sockfd_store() " Tetsuo Handa
2021-03-04 15:24 ` [PATCH v4 04/12] usb: usbip: stub: " Tetsuo Handa
2021-03-04 15:24 ` [PATCH v4 05/12] usb: usbip: don't reset tcp_socket at vhci_device_reset() Tetsuo Handa
2021-03-04 15:24 ` [PATCH v4 06/12] usb: usbip: fix error handling of kthread_get_run() Tetsuo Handa
2021-03-04 15:24 ` [PATCH v4 07/12] usb: usbip: preallocate kernel threads for consistent attach operation Tetsuo Handa
2021-03-04 15:24 ` [PATCH v4 08/12] usb: usbip: check that stream socket is used Tetsuo Handa
2021-03-04 15:24 ` [PATCH v4 09/12] usb: usbip: vhci: add automatic recovery to attach_store() Tetsuo Handa
2021-03-04 15:24 ` [PATCH v4 10/12] usb: usbip: vudc: add automatic recovery to usbip_sockfd_store() Tetsuo Handa
2021-03-04 15:24 ` [PATCH v4 11/12] usb: usbip: stub: " Tetsuo Handa
2021-03-04 15:24 ` [PATCH v4 12/12] usb: usbip: remove unused kthread_get_run() Tetsuo Handa
2021-03-04 15:52 ` [PATCH v4 00/12] usb: usbip: serialize attach/detach operations Shuah Khan
2021-03-05 10:06 ` Tetsuo Handa
2021-03-05 14:44 ` Shuah Khan
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).