* [syzbot] INFO: task hung in do_proc_bulk
@ 2021-08-28 15:52 syzbot
2021-08-28 18:03 ` Alan Stern
2021-09-02 20:04 ` [syzbot] INFO: task hung in do_proc_bulk Alan Stern
0 siblings, 2 replies; 17+ messages in thread
From: syzbot @ 2021-08-28 15:52 UTC (permalink / raw)
To: gregkh, johan, linux-kernel, linux-usb, mathias.nyman, stern,
syzkaller-bugs
Hello,
syzbot found the following issue on:
HEAD commit: d5ae8d7f85b7 Revert "media: dvb header files: move some he..
git tree: upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=155fa21e300000
kernel config: https://syzkaller.appspot.com/x/.config?x=2fd902af77ff1e56
dashboard link: https://syzkaller.appspot.com/bug?extid=ada0f7d3d9fd2016d927
compiler: Debian clang version 11.0.1-2, GNU ld (GNU Binutils for Debian) 2.35.1
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=11a1ab0e300000
Bisection is inconclusive: the issue happens on the oldest tested release.
bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=15662ee1300000
final oops: https://syzkaller.appspot.com/x/report.txt?x=17662ee1300000
console output: https://syzkaller.appspot.com/x/log.txt?x=13662ee1300000
IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+ada0f7d3d9fd2016d927@syzkaller.appspotmail.com
INFO: task syz-executor.0:8700 blocked for more than 143 seconds.
Not tainted 5.14.0-rc7-syzkaller #0
"echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
task:syz-executor.0 state:D stack:23192 pid: 8700 ppid: 8455 flags:0x00004004
Call Trace:
context_switch kernel/sched/core.c:4681 [inline]
__schedule+0xc07/0x11f0 kernel/sched/core.c:5938
schedule+0x14b/0x210 kernel/sched/core.c:6017
schedule_timeout+0x98/0x2f0 kernel/time/timer.c:1857
do_wait_for_common+0x2da/0x480 kernel/sched/completion.c:85
__wait_for_common kernel/sched/completion.c:106 [inline]
wait_for_common kernel/sched/completion.c:117 [inline]
wait_for_completion_timeout+0x46/0x60 kernel/sched/completion.c:157
usb_start_wait_urb+0x167/0x550 drivers/usb/core/message.c:63
do_proc_bulk+0x978/0x1080 drivers/usb/core/devio.c:1236
proc_bulk drivers/usb/core/devio.c:1273 [inline]
usbdev_do_ioctl drivers/usb/core/devio.c:2547 [inline]
usbdev_ioctl+0x3441/0x6b10 drivers/usb/core/devio.c:2713
vfs_ioctl fs/ioctl.c:51 [inline]
__do_sys_ioctl fs/ioctl.c:1069 [inline]
__se_sys_ioctl+0xfb/0x170 fs/ioctl.c:1055
do_syscall_x64 arch/x86/entry/common.c:50 [inline]
do_syscall_64+0x3d/0xb0 arch/x86/entry/common.c:80
entry_SYSCALL_64_after_hwframe+0x44/0xae
RIP: 0033:0x4665e9
RSP: 002b:00007f15a90dc188 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
RAX: ffffffffffffffda RBX: 000000000056bf80 RCX: 00000000004665e9
RDX: 0000000020000340 RSI: 00000000c0185502 RDI: 0000000000000004
RBP: 00000000004bfcc4 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 000000000056bf80
R13: 00007ffcc552e1bf R14: 00007f15a90dc300 R15: 0000000000022000
Showing all locks held in the system:
1 lock held by khungtaskd/1610:
#0: ffffffff8c717ec0 (rcu_read_lock){....}-{1:2}, at: rcu_lock_acquire+0x0/0x30 arch/x86/pci/mmconfig_64.c:151
1 lock held by in:imklog/8130:
#0: ffff888035998870 (&f->f_pos_lock){+.+.}-{3:3}, at: __fdget_pos+0x24e/0x2f0 fs/file.c:974
=============================================
NMI backtrace for cpu 1
CPU: 1 PID: 1610 Comm: khungtaskd Not tainted 5.14.0-rc7-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Call Trace:
__dump_stack lib/dump_stack.c:88 [inline]
dump_stack_lvl+0x1d3/0x29f lib/dump_stack.c:105
nmi_cpu_backtrace+0x16c/0x190 lib/nmi_backtrace.c:105
nmi_trigger_cpumask_backtrace+0x191/0x2f0 lib/nmi_backtrace.c:62
trigger_all_cpu_backtrace include/linux/nmi.h:146 [inline]
check_hung_uninterruptible_tasks kernel/hung_task.c:210 [inline]
watchdog+0xd06/0xd50 kernel/hung_task.c:295
kthread+0x453/0x480 kernel/kthread.c:319
ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:295
Sending NMI from CPU 1 to CPUs 0:
NMI backtrace for cpu 0
CPU: 0 PID: 10 Comm: kworker/u4:1 Not tainted 5.14.0-rc7-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Workqueue: bat_events batadv_nc_worker
RIP: 0010:lock_is_held_type+0x1/0x180 kernel/locking/lockdep.c:5653
Code: 74 df 83 3d d8 22 e1 03 00 75 d6 48 c7 c7 80 c3 2e 8a 48 c7 c6 c0 c3 2e 8a 31 c0 e8 c9 8d 72 f7 0f 0b eb bd e8 90 fd ff ff 55 <41> 57 41 56 41 55 41 54 53 48 83 ec 10 65 48 8b 04 25 28 00 00 00
RSP: 0018:ffffc90000cf7990 EFLAGS: 00000202
RAX: 1ffffffff18e3801 RBX: 1ffff9200019ef34 RCX: 0000000080000001
RDX: 0000000000000000 RSI: 00000000ffffffff RDI: ffffffff8c717e20
RBP: ffffc90000cf7a28 R08: dffffc0000000000 R09: fffffbfff1b74ee6
R10: fffffbfff1b74ee6 R11: 0000000000000000 R12: dffffc0000000000
R13: 1ffff9200019ef58 R14: dffffc0000000000 R15: 0000000000000000
FS: 0000000000000000(0000) GS:ffff8880b9c00000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007f0a1e997000 CR3: 000000000c48e000 CR4: 00000000001506f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
lock_is_held include/linux/lockdep.h:283 [inline]
rcu_read_lock_sched_held+0x87/0x110 kernel/rcu/update.c:125
trace_lock_acquire+0x59/0x190 include/trace/events/lock.h:13
lock_acquire+0xa4/0x4a0 kernel/locking/lockdep.c:5596
rcu_lock_acquire+0x2a/0x30 include/linux/rcupdate.h:267
rcu_read_lock include/linux/rcupdate.h:687 [inline]
batadv_nc_purge_orig_hash net/batman-adv/network-coding.c:404 [inline]
batadv_nc_worker+0xc8/0x5b0 net/batman-adv/network-coding.c:715
process_one_work+0x833/0x10c0 kernel/workqueue.c:2276
worker_thread+0xac1/0x1320 kernel/workqueue.c:2422
kthread+0x453/0x480 kernel/kthread.c:319
ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:295
----------------
Code disassembly (best guess):
0: 74 df je 0xffffffe1
2: 83 3d d8 22 e1 03 00 cmpl $0x0,0x3e122d8(%rip) # 0x3e122e1
9: 75 d6 jne 0xffffffe1
b: 48 c7 c7 80 c3 2e 8a mov $0xffffffff8a2ec380,%rdi
12: 48 c7 c6 c0 c3 2e 8a mov $0xffffffff8a2ec3c0,%rsi
19: 31 c0 xor %eax,%eax
1b: e8 c9 8d 72 f7 callq 0xf7728de9
20: 0f 0b ud2
22: eb bd jmp 0xffffffe1
24: e8 90 fd ff ff callq 0xfffffdb9
29: 55 push %rbp
* 2a: 41 57 push %r15 <-- trapping instruction
2c: 41 56 push %r14
2e: 41 55 push %r13
30: 41 54 push %r12
32: 53 push %rbx
33: 48 83 ec 10 sub $0x10,%rsp
37: 65 48 8b 04 25 28 00 mov %gs:0x28,%rax
3e: 00 00
---
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.
For information about bisection process see: https://goo.gl/tpsmEJ#bisection
syzbot can test patches for this issue, for details see:
https://goo.gl/tpsmEJ#testing-patches
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [syzbot] INFO: task hung in do_proc_bulk 2021-08-28 15:52 [syzbot] INFO: task hung in do_proc_bulk syzbot @ 2021-08-28 18:03 ` Alan Stern 2021-08-28 20:05 ` syzbot 2021-09-02 20:04 ` [syzbot] INFO: task hung in do_proc_bulk Alan Stern 1 sibling, 1 reply; 17+ messages in thread From: Alan Stern @ 2021-08-28 18:03 UTC (permalink / raw) To: syzbot Cc: gregkh, johan, linux-kernel, linux-usb, mathias.nyman, syzkaller-bugs On Sat, Aug 28, 2021 at 08:52:17AM -0700, syzbot wrote: > Hello, > > syzbot found the following issue on: > > HEAD commit: d5ae8d7f85b7 Revert "media: dvb header files: move some he.. > git tree: upstream > console output: https://syzkaller.appspot.com/x/log.txt?x=155fa21e300000 > kernel config: https://syzkaller.appspot.com/x/.config?x=2fd902af77ff1e56 > dashboard link: https://syzkaller.appspot.com/bug?extid=ada0f7d3d9fd2016d927 > compiler: Debian clang version 11.0.1-2, GNU ld (GNU Binutils for Debian) 2.35.1 > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=11a1ab0e300000 > > Bisection is inconclusive: the issue happens on the oldest tested release. > > bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=15662ee1300000 > final oops: https://syzkaller.appspot.com/x/report.txt?x=17662ee1300000 > console output: https://syzkaller.appspot.com/x/log.txt?x=13662ee1300000 > > IMPORTANT: if you fix the issue, please add the following tag to the commit: > Reported-by: syzbot+ada0f7d3d9fd2016d927@syzkaller.appspotmail.com > > INFO: task syz-executor.0:8700 blocked for more than 143 seconds. > Not tainted 5.14.0-rc7-syzkaller #0 > "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message. > task:syz-executor.0 state:D stack:23192 pid: 8700 ppid: 8455 flags:0x00004004 > Call Trace: > context_switch kernel/sched/core.c:4681 [inline] > __schedule+0xc07/0x11f0 kernel/sched/core.c:5938 > schedule+0x14b/0x210 kernel/sched/core.c:6017 > schedule_timeout+0x98/0x2f0 kernel/time/timer.c:1857 > do_wait_for_common+0x2da/0x480 kernel/sched/completion.c:85 > __wait_for_common kernel/sched/completion.c:106 [inline] > wait_for_common kernel/sched/completion.c:117 [inline] > wait_for_completion_timeout+0x46/0x60 kernel/sched/completion.c:157 > usb_start_wait_urb+0x167/0x550 drivers/usb/core/message.c:63 > do_proc_bulk+0x978/0x1080 drivers/usb/core/devio.c:1236 Looks like the wait needs to be interruptible. Alan Stern #syz test: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git d5ae8d7f85b7 Index: usb-devel/drivers/usb/core/message.c =================================================================== --- usb-devel.orig/drivers/usb/core/message.c +++ usb-devel/drivers/usb/core/message.c @@ -60,12 +60,18 @@ static int usb_start_wait_urb(struct urb goto out; expire = timeout ? msecs_to_jiffies(timeout) : MAX_SCHEDULE_TIMEOUT; - if (!wait_for_completion_timeout(&ctx.done, expire)) { + retval = wait_for_completion_interruptible_timeout(&ctx.done, expire); + if (retval <= 0) { usb_kill_urb(urb); - retval = (ctx.status == -ENOENT ? -ETIMEDOUT : ctx.status); + if (retval < 0) + retval = -EINTR; + else if (ctx.status == -ENOENT) + retval = -ETIMEDOUT; + else + retval = ctx.status; dev_dbg(&urb->dev->dev, - "%s timed out on ep%d%s len=%u/%u\n", + "%s timed out or interrupted on ep%d%s len=%u/%u\n", current->comm, usb_endpoint_num(&urb->ep->desc), usb_urb_dir_in(urb) ? "in" : "out", ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [syzbot] INFO: task hung in do_proc_bulk 2021-08-28 18:03 ` Alan Stern @ 2021-08-28 20:05 ` syzbot 2021-08-29 1:58 ` [PATCH] USB: core: Make usb_start_wait_urb() interruptible Alan Stern 0 siblings, 1 reply; 17+ messages in thread From: syzbot @ 2021-08-28 20:05 UTC (permalink / raw) To: gregkh, johan, linux-kernel, linux-usb, mathias.nyman, stern, syzkaller-bugs Hello, syzbot has tested the proposed patch and the reproducer did not trigger any issue: Reported-and-tested-by: syzbot+ada0f7d3d9fd2016d927@syzkaller.appspotmail.com Tested on: commit: d5ae8d7f Revert "media: dvb header files: move some he.. git tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git kernel config: https://syzkaller.appspot.com/x/.config?x=2fd902af77ff1e56 dashboard link: https://syzkaller.appspot.com/bug?extid=ada0f7d3d9fd2016d927 compiler: Debian clang version 11.0.1-2, GNU ld (GNU Binutils for Debian) 2.35.1 patch: https://syzkaller.appspot.com/x/patch.diff?x=16c2799d300000 Note: testing is done by a robot and is best-effort only. ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH] USB: core: Make usb_start_wait_urb() interruptible 2021-08-28 20:05 ` syzbot @ 2021-08-29 1:58 ` Alan Stern 2021-08-30 7:56 ` Johan Hovold 0 siblings, 1 reply; 17+ messages in thread From: Alan Stern @ 2021-08-29 1:58 UTC (permalink / raw) To: Greg KH; +Cc: syzbot, syzkaller-bugs, USB mailing list usb_start_wait_urb() can be called from user processes by means of the USBDEVFS_BULK and USBDEVFS_CONTROL ioctls in usbfs. Consequently it should not contain an uninterruptible wait of arbitrarily long length (the timeout value is specified here by the user, so it can be practically anything). Doing so leads the kernel to complain about "Task X blocked for more than N seconds", as found in testing by syzbot: INFO: task syz-executor.0:8700 blocked for more than 143 seconds. Not tainted 5.14.0-rc7-syzkaller #0 "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message. task:syz-executor.0 state:D stack:23192 pid: 8700 ppid: 8455 flags:0x00004004 Call Trace: context_switch kernel/sched/core.c:4681 [inline] __schedule+0xc07/0x11f0 kernel/sched/core.c:5938 schedule+0x14b/0x210 kernel/sched/core.c:6017 schedule_timeout+0x98/0x2f0 kernel/time/timer.c:1857 do_wait_for_common+0x2da/0x480 kernel/sched/completion.c:85 __wait_for_common kernel/sched/completion.c:106 [inline] wait_for_common kernel/sched/completion.c:117 [inline] wait_for_completion_timeout+0x46/0x60 kernel/sched/completion.c:157 usb_start_wait_urb+0x167/0x550 drivers/usb/core/message.c:63 do_proc_bulk+0x978/0x1080 drivers/usb/core/devio.c:1236 proc_bulk drivers/usb/core/devio.c:1273 [inline] usbdev_do_ioctl drivers/usb/core/devio.c:2547 [inline] usbdev_ioctl+0x3441/0x6b10 drivers/usb/core/devio.c:2713 ... This patch fixes the problem by converting the uninterruptible wait to an interruptible one. For the most part this won't affect calls to usb_start_wait_urb(), because they are made by kernel threads and so can't receive most signals. But in some cases such calls may occur in user threads in contexts other than usbfs ioctls. A signal in these circumstances could cause a USB transfer to fail when otherwise it wouldn't. The outcome wouldn't be too dreadful, since USB transfers can fail at any time and the system is prepared to handle these failures gracefully. In theory, for example, a signal might cause a driver's probe routine to fail; in practice if the user doesn't want a probe to fail then he shouldn't send interrupt signals to the probing process. Overall, then, making these delays interruptible seems to be an acceptable risk. Signed-off-by: Alan Stern <stern@rowland.harvard.edu> Reported-and-tested-by: syzbot+ada0f7d3d9fd2016d927@syzkaller.appspotmail.com CC: stable@vger.kernel.org --- [as1964] drivers/usb/core/message.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) Index: usb-devel/drivers/usb/core/message.c =================================================================== --- usb-devel.orig/drivers/usb/core/message.c +++ usb-devel/drivers/usb/core/message.c @@ -60,12 +60,18 @@ static int usb_start_wait_urb(struct urb goto out; expire = timeout ? msecs_to_jiffies(timeout) : MAX_SCHEDULE_TIMEOUT; - if (!wait_for_completion_timeout(&ctx.done, expire)) { + retval = wait_for_completion_interruptible_timeout(&ctx.done, expire); + if (retval <= 0) { usb_kill_urb(urb); - retval = (ctx.status == -ENOENT ? -ETIMEDOUT : ctx.status); + if (ctx.status != -ENOENT) /* URB already completed */ + retval = ctx.status; + else if (retval == 0) + retval = -ETIMEDOUT; + else + retval = -EINTR; dev_dbg(&urb->dev->dev, - "%s timed out on ep%d%s len=%u/%u\n", + "%s timed out or interrupted on ep%d%s len=%u/%u\n", current->comm, usb_endpoint_num(&urb->ep->desc), usb_urb_dir_in(urb) ? "in" : "out", ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] USB: core: Make usb_start_wait_urb() interruptible 2021-08-29 1:58 ` [PATCH] USB: core: Make usb_start_wait_urb() interruptible Alan Stern @ 2021-08-30 7:56 ` Johan Hovold 2021-08-30 14:46 ` Alan Stern 0 siblings, 1 reply; 17+ messages in thread From: Johan Hovold @ 2021-08-30 7:56 UTC (permalink / raw) To: Alan Stern; +Cc: Greg KH, syzbot, syzkaller-bugs, USB mailing list On Sat, Aug 28, 2021 at 09:58:25PM -0400, Alan Stern wrote: > usb_start_wait_urb() can be called from user processes by means of the > USBDEVFS_BULK and USBDEVFS_CONTROL ioctls in usbfs. Consequently it > should not contain an uninterruptible wait of arbitrarily long length > (the timeout value is specified here by the user, so it can be > practically anything). Doing so leads the kernel to complain about > "Task X blocked for more than N seconds", as found in testing by > syzbot: > > INFO: task syz-executor.0:8700 blocked for more than 143 seconds. > Not tainted 5.14.0-rc7-syzkaller #0 > "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message. > task:syz-executor.0 state:D stack:23192 pid: 8700 ppid: 8455 flags:0x00004004 > Call Trace: > context_switch kernel/sched/core.c:4681 [inline] > __schedule+0xc07/0x11f0 kernel/sched/core.c:5938 > schedule+0x14b/0x210 kernel/sched/core.c:6017 > schedule_timeout+0x98/0x2f0 kernel/time/timer.c:1857 > do_wait_for_common+0x2da/0x480 kernel/sched/completion.c:85 > __wait_for_common kernel/sched/completion.c:106 [inline] > wait_for_common kernel/sched/completion.c:117 [inline] > wait_for_completion_timeout+0x46/0x60 kernel/sched/completion.c:157 > usb_start_wait_urb+0x167/0x550 drivers/usb/core/message.c:63 > do_proc_bulk+0x978/0x1080 drivers/usb/core/devio.c:1236 > proc_bulk drivers/usb/core/devio.c:1273 [inline] > usbdev_do_ioctl drivers/usb/core/devio.c:2547 [inline] > usbdev_ioctl+0x3441/0x6b10 drivers/usb/core/devio.c:2713 > ... > > This patch fixes the problem by converting the uninterruptible wait to > an interruptible one. For the most part this won't affect calls to > usb_start_wait_urb(), because they are made by kernel threads and so > can't receive most signals. > > But in some cases such calls may occur in user threads in contexts > other than usbfs ioctls. A signal in these circumstances could cause > a USB transfer to fail when otherwise it wouldn't. The outcome > wouldn't be too dreadful, since USB transfers can fail at any time and > the system is prepared to handle these failures gracefully. In > theory, for example, a signal might cause a driver's probe routine to > fail; in practice if the user doesn't want a probe to fail then he > shouldn't send interrupt signals to the probing process. While probe() triggered through sysfs or by module loading is one example, the USB msg helpers are also called in a lot of other user-thread contexts such as open() calls etc. It might even be that the majority of these calls can be done from user threads (post enumeration). > Overall, then, making these delays interruptible seems to be an > acceptable risk. Possibly, but changing the API like this to fix the usbfs ioctls seems like using a bit of a too big hammer to me, especially when backporting to stable. > Signed-off-by: Alan Stern <stern@rowland.harvard.edu> > Reported-and-tested-by: syzbot+ada0f7d3d9fd2016d927@syzkaller.appspotmail.com > CC: stable@vger.kernel.org Johan ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] USB: core: Make usb_start_wait_urb() interruptible 2021-08-30 7:56 ` Johan Hovold @ 2021-08-30 14:46 ` Alan Stern 2021-08-30 15:11 ` Oliver Neukum 2021-08-31 9:13 ` Johan Hovold 0 siblings, 2 replies; 17+ messages in thread From: Alan Stern @ 2021-08-30 14:46 UTC (permalink / raw) To: Johan Hovold; +Cc: Greg KH, syzbot, syzkaller-bugs, USB mailing list On Mon, Aug 30, 2021 at 09:56:50AM +0200, Johan Hovold wrote: > On Sat, Aug 28, 2021 at 09:58:25PM -0400, Alan Stern wrote: > > This patch fixes the problem by converting the uninterruptible wait to > > an interruptible one. For the most part this won't affect calls to > > usb_start_wait_urb(), because they are made by kernel threads and so > > can't receive most signals. > > > > But in some cases such calls may occur in user threads in contexts > > other than usbfs ioctls. A signal in these circumstances could cause > > a USB transfer to fail when otherwise it wouldn't. The outcome > > wouldn't be too dreadful, since USB transfers can fail at any time and > > the system is prepared to handle these failures gracefully. In > > theory, for example, a signal might cause a driver's probe routine to > > fail; in practice if the user doesn't want a probe to fail then he > > shouldn't send interrupt signals to the probing process. > > While probe() triggered through sysfs or by module loading is one > example, the USB msg helpers are also called in a lot of other > user-thread contexts such as open() calls etc. It might even be that the > majority of these calls can be done from user threads (post > enumeration). Could be. It's not a well defined matter; it depends on what drivers are in use and how they are used. Consider that a control message in a driver is likely to use the default USB_CTRL_[GS]ET_TIMEOUT value of 5 seconds. Does it make sense to allow uninterruptible wait states to last as long as that? And to what extent does it matter if we make these delays interruptible? A signal delivered during a system call will be fielded when the call returns if not earlier; the only difference will be that now some USB messages may be aborted. For things like SIGINT or SIGTERM this seems reasonable. (I'm not so sure about things like SIGALRM, SIGIO, or SIGSTOP, though.) > > Overall, then, making these delays interruptible seems to be an > > acceptable risk. > > Possibly, but changing the API like this to fix the usbfs ioctls seems > like using a bit of a too big hammer to me, especially when backporting > to stable. Perhaps the stable backport could be delayed for a while (say, one release cycle). Do you have alternative suggestions? I don't think we want special interruptible versions of usb_control_msg() and usb_bulk_msg() just for use by usbfs. Alan Stern ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] USB: core: Make usb_start_wait_urb() interruptible 2021-08-30 14:46 ` Alan Stern @ 2021-08-30 15:11 ` Oliver Neukum 2021-08-30 16:09 ` Alan Stern 2021-08-31 9:13 ` Johan Hovold 1 sibling, 1 reply; 17+ messages in thread From: Oliver Neukum @ 2021-08-30 15:11 UTC (permalink / raw) To: Alan Stern, Johan Hovold Cc: Greg KH, syzbot, syzkaller-bugs, USB mailing list On 30.08.21 16:46, Alan Stern wrote: > Do you have alternative suggestions? I don't think we want special > interruptible versions of usb_control_msg() and usb_bulk_msg() just for > use by usbfs. Hi, why not just pass a flag? Regards Oliver ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] USB: core: Make usb_start_wait_urb() interruptible 2021-08-30 15:11 ` Oliver Neukum @ 2021-08-30 16:09 ` Alan Stern 2021-08-31 8:52 ` Oliver Neukum 0 siblings, 1 reply; 17+ messages in thread From: Alan Stern @ 2021-08-30 16:09 UTC (permalink / raw) To: Oliver Neukum Cc: Johan Hovold, Greg KH, syzbot, syzkaller-bugs, USB mailing list On Mon, Aug 30, 2021 at 05:11:53PM +0200, Oliver Neukum wrote: > > On 30.08.21 16:46, Alan Stern wrote: > > Do you have alternative suggestions? I don't think we want special > > interruptible versions of usb_control_msg() and usb_bulk_msg() just for > > use by usbfs. > > Hi, > > why not just pass a flag? We could. But you're ignoring the question I asked earlier in that email: Is a 5-second uninterruptible delay (the default USB control timeout) acceptable in general? Alan Stern ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] USB: core: Make usb_start_wait_urb() interruptible 2021-08-30 16:09 ` Alan Stern @ 2021-08-31 8:52 ` Oliver Neukum 0 siblings, 0 replies; 17+ messages in thread From: Oliver Neukum @ 2021-08-31 8:52 UTC (permalink / raw) To: Alan Stern Cc: Johan Hovold, Greg KH, syzbot, syzkaller-bugs, USB mailing list On 30.08.21 18:09, Alan Stern wrote: > On Mon, Aug 30, 2021 at 05:11:53PM +0200, Oliver Neukum wrote: >> On 30.08.21 16:46, Alan Stern wrote: >>> Do you have alternative suggestions? I don't think we want special >>> interruptible versions of usb_control_msg() and usb_bulk_msg() just for >>> use by usbfs. >> Hi, >> >> why not just pass a flag? > We could. But you're ignoring the question I asked earlier in that > email: Is a 5-second uninterruptible delay (the default USB control > timeout) acceptable in general? We cannot change the five seconds, so this boils down to whether we can always handle signals when we need to send control messages. The answer to that is negative. Suspend/resume, block IO, probe, ioctl(). This list will be rather long. Regards Oliver ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] USB: core: Make usb_start_wait_urb() interruptible 2021-08-30 14:46 ` Alan Stern 2021-08-30 15:11 ` Oliver Neukum @ 2021-08-31 9:13 ` Johan Hovold 2021-08-31 10:47 ` Oliver Neukum ` (2 more replies) 1 sibling, 3 replies; 17+ messages in thread From: Johan Hovold @ 2021-08-31 9:13 UTC (permalink / raw) To: Alan Stern; +Cc: Greg KH, syzbot, syzkaller-bugs, USB mailing list On Mon, Aug 30, 2021 at 10:46:13AM -0400, Alan Stern wrote: > On Mon, Aug 30, 2021 at 09:56:50AM +0200, Johan Hovold wrote: > > On Sat, Aug 28, 2021 at 09:58:25PM -0400, Alan Stern wrote: > > > This patch fixes the problem by converting the uninterruptible wait to > > > an interruptible one. For the most part this won't affect calls to > > > usb_start_wait_urb(), because they are made by kernel threads and so > > > can't receive most signals. > > > > > > But in some cases such calls may occur in user threads in contexts > > > other than usbfs ioctls. A signal in these circumstances could cause > > > a USB transfer to fail when otherwise it wouldn't. The outcome > > > wouldn't be too dreadful, since USB transfers can fail at any time and > > > the system is prepared to handle these failures gracefully. In > > > theory, for example, a signal might cause a driver's probe routine to > > > fail; in practice if the user doesn't want a probe to fail then he > > > shouldn't send interrupt signals to the probing process. > > > > While probe() triggered through sysfs or by module loading is one > > example, the USB msg helpers are also called in a lot of other > > user-thread contexts such as open() calls etc. It might even be that the > > majority of these calls can be done from user threads (post > > enumeration). > > Could be. It's not a well defined matter; it depends on what drivers > are in use and how they are used. Right, but the commit message seemed to suggest that these helpers being run from interruptible threads was the exception. > Consider that a control message in a driver is likely to use the > default USB_CTRL_[GS]ET_TIMEOUT value of 5 seconds. Does it make sense > to allow uninterruptible wait states to last as long as that? Perhaps sometimes? I don't have a use case at hand, but I haven't reviewed all drivers either. The comment above usb_start_wait_urb() (which also needs to be updated, by the way) even suggests that drivers should "implement their own interruptible routines" so perhaps this has just gone unnoticed for 20 odd years. And the question then becomes, why didn't we use interruptible sleep from the start? And trying to answer that I find that that's precisely what we did, but for some reason it was changed to uninterruptible sleep in v2.4.11 without a motivation (that I could easily find spelled out). > And to what extent does it matter if we make these delays > interruptible? A signal delivered during a system call will be fielded > when the call returns if not earlier; the only difference will be that > now some USB messages may be aborted. For things like SIGINT or > SIGTERM this seems reasonable. (I'm not so sure about things like > SIGALRM, SIGIO, or SIGSTOP, though.) I'm not saying I'm necessarily against the change. It just seemed a bit rushed to change the (stable) API like this while claiming it won't affect most call sites. > > > Overall, then, making these delays interruptible seems to be an > > > acceptable risk. > > > > Possibly, but changing the API like this to fix the usbfs ioctls seems > > like using a bit of a too big hammer to me, especially when backporting > > to stable. > > Perhaps the stable backport could be delayed for a while (say, one > release cycle). That might work. > Do you have alternative suggestions? I don't think we want special > interruptible versions of usb_control_msg() and usb_bulk_msg() just for > use by usbfs. usbfs could carry a temporary local implementation as the documentation for usb_start_wait_urb() currently suggests. I assume we can't limit the usbfs timeouts. Johan ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] USB: core: Make usb_start_wait_urb() interruptible 2021-08-31 9:13 ` Johan Hovold @ 2021-08-31 10:47 ` Oliver Neukum 2021-08-31 11:02 ` Oliver Neukum 2021-08-31 11:10 ` Johan Hovold 2 siblings, 0 replies; 17+ messages in thread From: Oliver Neukum @ 2021-08-31 10:47 UTC (permalink / raw) To: Johan Hovold, Alan Stern Cc: Greg KH, syzbot, syzkaller-bugs, USB mailing list On 31.08.21 11:13, Johan Hovold wrote: > The comment above usb_start_wait_urb() (which also needs to be updated, > by the way) even suggests that drivers should "implement their own > interruptible routines" so perhaps this has just gone unnoticed for 20 > odd years. And the question then becomes, why didn't we use > interruptible sleep from the start? > > And trying to answer that I find that that's precisely what we did, but > for some reason it was changed to uninterruptible sleep in v2.4.11 > without a motivation (that I could easily find spelled out). I must admit that I do not remember. But there are a lot of situations requiring control messages that do not allow signal delivery. Take for example a device that is HID and storage. We are doing HID error handling, which can involve a device reset. You absolutely cannot deliver a signal right now, as you have a device that is in an undefined state in the block layer. It looks to me very much like we need both versions and as a rule of thumb, while you would use GFP_NOIO you must also use the uninterruptible messaging. Regards Oliver ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] USB: core: Make usb_start_wait_urb() interruptible 2021-08-31 9:13 ` Johan Hovold 2021-08-31 10:47 ` Oliver Neukum @ 2021-08-31 11:02 ` Oliver Neukum 2021-08-31 11:10 ` Johan Hovold 2 siblings, 0 replies; 17+ messages in thread From: Oliver Neukum @ 2021-08-31 11:02 UTC (permalink / raw) To: Johan Hovold, Alan Stern Cc: Greg KH, syzbot, syzkaller-bugs, USB mailing list On 31.08.21 11:13, Johan Hovold wrote: > On Mon, Aug 30, 2021 at 10:46:13AM -0400, Alan Stern wrote: >> Perhaps the stable backport could be delayed for a while (say, one >> release cycle). > That might work. > >> Do you have alternative suggestions? I don't think we want special >> interruptible versions of usb_control_msg() and usb_bulk_msg() just for >> use by usbfs. > usbfs could carry a temporary local implementation as the documentation > for usb_start_wait_urb() currently suggests. I assume we can't limit the > usbfs timeouts. Upon further considerations forcing user space to handle signals also breaks the API, albeit in a more subtle manner. I'd suggest that we use wait_event_killable_timeout(). And do it the way Alan initially disliked, that is code a version for use by usbfs. Thus we'd avoid the issue of having an unkillable process, but we do not impose a need to handle signals. Regards Oliver ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] USB: core: Make usb_start_wait_urb() interruptible 2021-08-31 9:13 ` Johan Hovold 2021-08-31 10:47 ` Oliver Neukum 2021-08-31 11:02 ` Oliver Neukum @ 2021-08-31 11:10 ` Johan Hovold 2021-08-31 17:03 ` Alan Stern 2 siblings, 1 reply; 17+ messages in thread From: Johan Hovold @ 2021-08-31 11:10 UTC (permalink / raw) To: Alan Stern; +Cc: Greg KH, syzbot, syzkaller-bugs, USB mailing list On Tue, Aug 31, 2021 at 11:13:59AM +0200, Johan Hovold wrote: > On Mon, Aug 30, 2021 at 10:46:13AM -0400, Alan Stern wrote: > > Consider that a control message in a driver is likely to use the > > default USB_CTRL_[GS]ET_TIMEOUT value of 5 seconds. Does it make sense > > to allow uninterruptible wait states to last as long as that? > > Perhaps sometimes? I don't have a use case at hand, but I haven't > reviewed all drivers either. > > The comment above usb_start_wait_urb() (which also needs to be updated, > by the way) even suggests that drivers should "implement their own > interruptible routines" so perhaps this has just gone unnoticed for 20 > odd years. And the question then becomes, why didn't we use > interruptible sleep from the start? > > And trying to answer that I find that that's precisely what we did, but > for some reason it was changed to uninterruptible sleep in v2.4.11 > without a motivation (that I could easily find spelled out). Here it is: https://lore.kernel.org/lkml/20010818013101.A7058@devserv.devel.redhat.com/ It's rationale does not seem valid anymore (i.e. the NULL deref), but the example is still instructive. If you close for example a v4l application you still expect the device to be shut down orderly but with interruptible sleep all control requests during shutdown will be aborted immediately. Similar for USB serial devices which would for example fail to deassert DTR/RTS, etc. (I just verified with the patch applied.) Johan ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] USB: core: Make usb_start_wait_urb() interruptible 2021-08-31 11:10 ` Johan Hovold @ 2021-08-31 17:03 ` Alan Stern 2021-09-01 8:16 ` Oliver Neukum 0 siblings, 1 reply; 17+ messages in thread From: Alan Stern @ 2021-08-31 17:03 UTC (permalink / raw) To: Johan Hovold, Oliver Neukum Cc: Greg KH, syzbot, syzkaller-bugs, USB mailing list On Tue, Aug 31, 2021 at 01:10:32PM +0200, Johan Hovold wrote: > On Tue, Aug 31, 2021 at 11:13:59AM +0200, Johan Hovold wrote: > > On Mon, Aug 30, 2021 at 10:46:13AM -0400, Alan Stern wrote: > > > > Consider that a control message in a driver is likely to use the > > > default USB_CTRL_[GS]ET_TIMEOUT value of 5 seconds. Does it make sense > > > to allow uninterruptible wait states to last as long as that? > > > > Perhaps sometimes? I don't have a use case at hand, but I haven't > > reviewed all drivers either. > > > > The comment above usb_start_wait_urb() (which also needs to be updated, > > by the way) even suggests that drivers should "implement their own > > interruptible routines" so perhaps this has just gone unnoticed for 20 > > odd years. And the question then becomes, why didn't we use > > interruptible sleep from the start? > > > > And trying to answer that I find that that's precisely what we did, but > > for some reason it was changed to uninterruptible sleep in v2.4.11 > > without a motivation (that I could easily find spelled out). > > Here it is: > > https://lore.kernel.org/lkml/20010818013101.A7058@devserv.devel.redhat.com/ > > It's rationale does not seem valid anymore (i.e. the NULL deref), but > the example is still instructive. > > If you close for example a v4l application you still expect the device > to be shut down orderly but with interruptible sleep all control > requests during shutdown will be aborted immediately. > > Similar for USB serial devices which would for example fail to deassert > DTR/RTS, etc. (I just verified with the patch applied.) On Tue, Aug 31, 2021 at 01:02:11PM +0200, Oliver Neukum wrote: > Upon further considerations forcing user space to handle signals also > breaks the API, albeit in a more subtle manner. I'd suggest that we use > wait_event_killable_timeout(). And do it the way Alan initially disliked, > that is code a version for use by usbfs. > > Thus we'd avoid the issue of having an unkillable process, but we do > not impose a need to handle signals. Okay, I'll play it safe and rewrite the patch, adding special-purpose routines just for usbfs. Will wait_event_killable_timeout() prevent complaints about tasks being blocked for too long (the reason syzbot reported this in the first place)? Alan Stern ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] USB: core: Make usb_start_wait_urb() interruptible 2021-08-31 17:03 ` Alan Stern @ 2021-09-01 8:16 ` Oliver Neukum 0 siblings, 0 replies; 17+ messages in thread From: Oliver Neukum @ 2021-09-01 8:16 UTC (permalink / raw) To: Alan Stern, Johan Hovold Cc: Greg KH, syzbot, syzkaller-bugs, USB mailing list On 31.08.21 19:03, Alan Stern wrote: > > Will wait_event_killable_timeout() prevent complaints about tasks being > blocked for too long (the reason syzbot reported this in the first > place)? Very good question. TASK_KILLABLE is its own task state, so I'd say if it doesn't the test suite needs to be fixed. Regards Oliver ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [syzbot] INFO: task hung in do_proc_bulk 2021-08-28 15:52 [syzbot] INFO: task hung in do_proc_bulk syzbot 2021-08-28 18:03 ` Alan Stern @ 2021-09-02 20:04 ` Alan Stern 2021-09-02 20:46 ` syzbot 1 sibling, 1 reply; 17+ messages in thread From: Alan Stern @ 2021-09-02 20:04 UTC (permalink / raw) To: syzbot; +Cc: linux-kernel, linux-usb, syzkaller-bugs Let's see if making the wait killable rather than interruptible fixes the issue. This patch avoids modifying the regular usb_start_wait_urb(), creating a separate usbfs_start_wait_urb() just for this purpose. Alan Stern #syz test: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git d5ae8d7f85b7 Index: usb-devel/drivers/usb/core/devio.c =================================================================== --- usb-devel.orig/drivers/usb/core/devio.c +++ usb-devel/drivers/usb/core/devio.c @@ -32,6 +32,7 @@ #include <linux/usb.h> #include <linux/usbdevice_fs.h> #include <linux/usb/hcd.h> /* for usbcore internals */ +#include <linux/usb/quirks.h> #include <linux/cdev.h> #include <linux/notifier.h> #include <linux/security.h> @@ -1102,14 +1103,55 @@ static int usbdev_release(struct inode * return 0; } +static void usbfs_blocking_completion(struct urb *urb) +{ + complete((struct completion *) urb->context); +} + +/* + * Much like usb_start_wait_urb, but returns status separately from + * actual_length and uses a killable wait. + */ +static int usbfs_start_wait_urb(struct urb *urb, int timeout, + unsigned int *actlen) +{ + DECLARE_COMPLETION_ONSTACK(ctx); + unsigned long expire; + int rc; + + urb->context = &ctx; + urb->complete = usbfs_blocking_completion; + *actlen = 0; + rc = usb_submit_urb(urb, GFP_KERNEL); + if (unlikely(rc)) + return rc; + + expire = (timeout ? msecs_to_jiffies(timeout) : MAX_SCHEDULE_TIMEOUT); + rc = wait_for_completion_killable_timeout(&ctx, expire); + if (rc <= 0) { + usb_kill_urb(urb); + *actlen = urb->actual_length; + if (urb->status != -ENOENT) + ; /* Completed before it was killed */ + else if (rc < 0) + return -EINTR; + else + return -ETIMEDOUT; + } + *actlen = urb->actual_length; + return urb->status; +} + static int do_proc_control(struct usb_dev_state *ps, struct usbdevfs_ctrltransfer *ctrl) { struct usb_device *dev = ps->dev; unsigned int tmo; unsigned char *tbuf; - unsigned wLength; + unsigned wLength, actlen; int i, pipe, ret; + struct urb *urb = NULL; + struct usb_ctrlrequest *dr = NULL; ret = check_ctrlrecip(ps, ctrl->bRequestType, ctrl->bRequest, ctrl->wIndex); @@ -1122,51 +1164,63 @@ static int do_proc_control(struct usb_de sizeof(struct usb_ctrlrequest)); if (ret) return ret; + + ret = -ENOMEM; tbuf = (unsigned char *)__get_free_page(GFP_KERNEL); - if (!tbuf) { - ret = -ENOMEM; + if (!tbuf) goto done; - } + urb = usb_alloc_urb(0, GFP_NOIO); + if (!urb) + goto done; + dr = kmalloc(sizeof(struct usb_ctrlrequest), GFP_NOIO); + if (!dr) + goto done; + + dr->bRequestType = ctrl->bRequestType; + dr->bRequest = ctrl->bRequest; + dr->wValue = cpu_to_le16(ctrl->wValue); + dr->wIndex = cpu_to_le16(ctrl->wIndex); + dr->wLength = cpu_to_le16(ctrl->wLength); + tmo = ctrl->timeout; snoop(&dev->dev, "control urb: bRequestType=%02x " "bRequest=%02x wValue=%04x " "wIndex=%04x wLength=%04x\n", ctrl->bRequestType, ctrl->bRequest, ctrl->wValue, ctrl->wIndex, ctrl->wLength); - if ((ctrl->bRequestType & USB_DIR_IN) && ctrl->wLength) { + + if ((ctrl->bRequestType & USB_DIR_IN) && wLength) { pipe = usb_rcvctrlpipe(dev, 0); - snoop_urb(dev, NULL, pipe, ctrl->wLength, tmo, SUBMIT, NULL, 0); + usb_fill_control_urb(urb, dev, pipe, (unsigned char *) dr, tbuf, + wLength, NULL, NULL); + snoop_urb(dev, NULL, pipe, wLength, tmo, SUBMIT, NULL, 0); usb_unlock_device(dev); - i = usb_control_msg(dev, pipe, ctrl->bRequest, - ctrl->bRequestType, ctrl->wValue, ctrl->wIndex, - tbuf, ctrl->wLength, tmo); + i = usbfs_start_wait_urb(urb, tmo, &actlen); usb_lock_device(dev); - snoop_urb(dev, NULL, pipe, max(i, 0), min(i, 0), COMPLETE, - tbuf, max(i, 0)); - if ((i > 0) && ctrl->wLength) { - if (copy_to_user(ctrl->data, tbuf, i)) { + snoop_urb(dev, NULL, pipe, actlen, i, COMPLETE, tbuf, actlen); + if (!i && actlen) { + if (copy_to_user(ctrl->data, tbuf, actlen)) { ret = -EFAULT; - goto done; + goto recv_fault; } } } else { - if (ctrl->wLength) { - if (copy_from_user(tbuf, ctrl->data, ctrl->wLength)) { + if (wLength) { + if (copy_from_user(tbuf, ctrl->data, wLength)) { ret = -EFAULT; goto done; } } pipe = usb_sndctrlpipe(dev, 0); - snoop_urb(dev, NULL, pipe, ctrl->wLength, tmo, SUBMIT, - tbuf, ctrl->wLength); + usb_fill_control_urb(urb, dev, pipe, (unsigned char *) dr, tbuf, + wLength, NULL, NULL); + snoop_urb(dev, NULL, pipe, wLength, tmo, SUBMIT, tbuf, wLength); usb_unlock_device(dev); - i = usb_control_msg(dev, pipe, ctrl->bRequest, - ctrl->bRequestType, ctrl->wValue, ctrl->wIndex, - tbuf, ctrl->wLength, tmo); + i = usbfs_start_wait_urb(urb, tmo, &actlen); usb_lock_device(dev); - snoop_urb(dev, NULL, pipe, max(i, 0), min(i, 0), COMPLETE, NULL, 0); + snoop_urb(dev, NULL, pipe, actlen, i, COMPLETE, NULL, 0); } if (i < 0 && i != -EPIPE) { dev_printk(KERN_DEBUG, &dev->dev, "usbfs: USBDEVFS_CONTROL " @@ -1174,8 +1228,15 @@ static int do_proc_control(struct usb_de current->comm, ctrl->bRequestType, ctrl->bRequest, ctrl->wLength, i); } - ret = i; + ret = (i < 0 ? i : actlen); + + recv_fault: + /* Linger a bit, prior to the next control message. */ + if (dev->quirks & USB_QUIRK_DELAY_CTRL_MSG) + msleep(200); done: + kfree(dr); + usb_free_urb(urb); free_page((unsigned long) tbuf); usbfs_decrease_memory_usage(PAGE_SIZE + sizeof(struct urb) + sizeof(struct usb_ctrlrequest)); @@ -1195,10 +1256,11 @@ static int do_proc_bulk(struct usb_dev_s struct usbdevfs_bulktransfer *bulk) { struct usb_device *dev = ps->dev; - unsigned int tmo, len1, pipe; - int len2; + unsigned int tmo, len1, len2, pipe; unsigned char *tbuf; int i, ret; + struct urb *urb = NULL; + struct usb_host_endpoint *ep; ret = findintfep(ps->dev, bulk->ep); if (ret < 0) @@ -1206,14 +1268,17 @@ static int do_proc_bulk(struct usb_dev_s ret = checkintf(ps, ret); if (ret) return ret; + + len1 = bulk->len; + if (len1 < 0 || len1 >= (INT_MAX - sizeof(struct urb))) + return -EINVAL; + if (bulk->ep & USB_DIR_IN) pipe = usb_rcvbulkpipe(dev, bulk->ep & 0x7f); else pipe = usb_sndbulkpipe(dev, bulk->ep & 0x7f); - if (!usb_maxpacket(dev, pipe, !(bulk->ep & USB_DIR_IN))) - return -EINVAL; - len1 = bulk->len; - if (len1 >= (INT_MAX - sizeof(struct urb))) + ep = usb_pipe_endpoint(dev, pipe); + if (!ep || !usb_endpoint_maxp(&ep->desc)) return -EINVAL; ret = usbfs_increase_memory_usage(len1 + sizeof(struct urb)); if (ret) @@ -1223,17 +1288,29 @@ static int do_proc_bulk(struct usb_dev_s * len1 can be almost arbitrarily large. Don't WARN if it's * too big, just fail the request. */ + ret = -ENOMEM; tbuf = kmalloc(len1, GFP_KERNEL | __GFP_NOWARN); - if (!tbuf) { - ret = -ENOMEM; + if (!tbuf) goto done; + urb = usb_alloc_urb(0, GFP_KERNEL); + if (!urb) + goto done; + + if ((ep->desc.bmAttributes & USB_ENDPOINT_XFERTYPE_MASK) == + USB_ENDPOINT_XFER_INT) { + pipe = (pipe & ~(3 << 30)) | (PIPE_INTERRUPT << 30); + usb_fill_int_urb(urb, dev, pipe, tbuf, len1, + NULL, NULL, ep->desc.bInterval); + } else { + usb_fill_bulk_urb(urb, dev, pipe, tbuf, len1, NULL, NULL); } + tmo = bulk->timeout; if (bulk->ep & 0x80) { snoop_urb(dev, NULL, pipe, len1, tmo, SUBMIT, NULL, 0); usb_unlock_device(dev); - i = usb_bulk_msg(dev, pipe, tbuf, len1, &len2, tmo); + i = usbfs_start_wait_urb(urb, tmo, &len2); usb_lock_device(dev); snoop_urb(dev, NULL, pipe, len2, i, COMPLETE, tbuf, len2); @@ -1253,12 +1330,13 @@ static int do_proc_bulk(struct usb_dev_s snoop_urb(dev, NULL, pipe, len1, tmo, SUBMIT, tbuf, len1); usb_unlock_device(dev); - i = usb_bulk_msg(dev, pipe, tbuf, len1, &len2, tmo); + i = usbfs_start_wait_urb(urb, tmo, &len2); usb_lock_device(dev); snoop_urb(dev, NULL, pipe, len2, i, COMPLETE, NULL, 0); } ret = (i < 0 ? i : len2); done: + usb_free_urb(urb); kfree(tbuf); usbfs_decrease_memory_usage(len1 + sizeof(struct urb)); return ret; ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [syzbot] INFO: task hung in do_proc_bulk 2021-09-02 20:04 ` [syzbot] INFO: task hung in do_proc_bulk Alan Stern @ 2021-09-02 20:46 ` syzbot 0 siblings, 0 replies; 17+ messages in thread From: syzbot @ 2021-09-02 20:46 UTC (permalink / raw) To: linux-kernel, linux-usb, stern, syzkaller-bugs Hello, syzbot has tested the proposed patch and the reproducer did not trigger any issue: Reported-and-tested-by: syzbot+ada0f7d3d9fd2016d927@syzkaller.appspotmail.com Tested on: commit: d5ae8d7f Revert "media: dvb header files: move some he.. git tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git kernel config: https://syzkaller.appspot.com/x/.config?x=2fd902af77ff1e56 dashboard link: https://syzkaller.appspot.com/bug?extid=ada0f7d3d9fd2016d927 compiler: Debian clang version 11.0.1-2, GNU ld (GNU Binutils for Debian) 2.35.1 patch: https://syzkaller.appspot.com/x/patch.diff?x=112280a3300000 Note: testing is done by a robot and is best-effort only. ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2021-09-02 20:46 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-08-28 15:52 [syzbot] INFO: task hung in do_proc_bulk syzbot 2021-08-28 18:03 ` Alan Stern 2021-08-28 20:05 ` syzbot 2021-08-29 1:58 ` [PATCH] USB: core: Make usb_start_wait_urb() interruptible Alan Stern 2021-08-30 7:56 ` Johan Hovold 2021-08-30 14:46 ` Alan Stern 2021-08-30 15:11 ` Oliver Neukum 2021-08-30 16:09 ` Alan Stern 2021-08-31 8:52 ` Oliver Neukum 2021-08-31 9:13 ` Johan Hovold 2021-08-31 10:47 ` Oliver Neukum 2021-08-31 11:02 ` Oliver Neukum 2021-08-31 11:10 ` Johan Hovold 2021-08-31 17:03 ` Alan Stern 2021-09-01 8:16 ` Oliver Neukum 2021-09-02 20:04 ` [syzbot] INFO: task hung in do_proc_bulk Alan Stern 2021-09-02 20:46 ` syzbot
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).