WARNING in ar5523_cmd/usb_submit_urb

[syzbot]

Hello,

syzbot found the following crash on:

HEAD commit: cd234325 usb: gadget: add raw-gadget interface
git tree: https://github.com/google/kasan.git usb-fuzzer
console output: https://syzkaller.appspot.com/x/log.txt?x=146bf3c9e00000
kernel config: https://syzkaller.appspot.com/x/.config?x=bb745005307bc641
dashboard link: https://syzkaller.appspot.com/bug?extid=1bc2c2afd44f820a669f
compiler: gcc (GCC) 9.0.0 20181231 (experimental)
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=1646cf35e00000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=11017735e00000

IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+1bc2c2...@syzkaller.appspotmail.com

usb 1-1: New USB device strings: Mfr=1, Product=2, SerialNumber=3
usb 1-1: Product: syz
usb 1-1: Manufacturer: syz
usb 1-1: SerialNumber: syz
------------[ cut here ]------------
usb 1-1: BOGUS urb xfer, pipe 3 != type 1
WARNING: CPU: 0 PID: 95 at drivers/usb/core/urb.c:478 usb_submit_urb+0x1188/0x1460 drivers/usb/core/urb.c:478
Kernel panic - not syncing: panic_on_warn set ...
CPU: 0 PID: 95 Comm: kworker/0:2 Not tainted 5.5.0-rc7-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Workqueue: usb_hub_wq hub_event
Call Trace:
__dump_stack lib/dump_stack.c:77 [inline]
dump_stack+0xef/0x16e lib/dump_stack.c:118
panic+0x2aa/0x6e1 kernel/panic.c:221
__warn.cold+0x2f/0x30 kernel/panic.c:582
report_bug+0x27b/0x2f0 lib/bug.c:195
fixup_bug arch/x86/kernel/traps.c:174 [inline]
fixup_bug arch/x86/kernel/traps.c:169 [inline]
do_error_trap+0x12b/0x1e0 arch/x86/kernel/traps.c:267
do_invalid_op+0x32/0x40 arch/x86/kernel/traps.c:286
invalid_op+0x23/0x30 arch/x86/entry/entry_64.S:1027
RIP: 0010:usb_submit_urb+0x1188/0x1460 drivers/usb/core/urb.c:478
Code: 4d 85 ed 74 46 e8 28 2d e1 fd 4c 89 f7 e8 d0 87 17 ff 41 89 d8 44 89 e1 4c 89 ea 48 89 c6 48 c7 c7 a0 2b 3b 86 e8 20 13 b6 fd <0f> 0b e9 20 f4 ff ff e8 fc 2c e1 fd 0f 1f 44 00 00 e8 f2 2c e1 fd
RSP: 0018:ffff8881d58cf0d8 EFLAGS: 00010286
RAX: 0000000000000000 RBX: 0000000000000001 RCX: 0000000000000000
RDX: 0000000000000000 RSI: ffffffff81295a0d RDI: ffffed103ab19e0d
RBP: ffff8881cd478050 R08: ffff8881d71ac980 R09: fffffbfff1269cae
R10: fffffbfff1269cad R11: ffffffff8934e56f R12: 0000000000000003
R13: ffff8881d098eee8 R14: ffff8881cda730a0 R15: ffff8881d5583b00
ar5523_cmd+0x438/0x7a0 drivers/net/wireless/ath/ar5523/ar5523.c:271
ar5523_cmd_read drivers/net/wireless/ath/ar5523/ar5523.c:298 [inline]
ar5523_host_available drivers/net/wireless/ath/ar5523/ar5523.c:1372 [inline]
ar5523_probe+0xc11/0x1ad0 drivers/net/wireless/ath/ar5523/ar5523.c:1652
usb_probe_interface+0x310/0x800 drivers/usb/core/driver.c:361
really_probe+0x290/0xad0 drivers/base/dd.c:548
driver_probe_device+0x223/0x350 drivers/base/dd.c:721
__device_attach_driver+0x1d1/0x290 drivers/base/dd.c:828
bus_for_each_drv+0x162/0x1e0 drivers/base/bus.c:430
__device_attach+0x217/0x390 drivers/base/dd.c:894
bus_probe_device+0x1e4/0x290 drivers/base/bus.c:490
device_add+0x1459/0x1bf0 drivers/base/core.c:2487
usb_set_configuration+0xe47/0x17d0 drivers/usb/core/message.c:2023
generic_probe+0x9d/0xd5 drivers/usb/core/generic.c:210
usb_probe_device+0xaf/0x140 drivers/usb/core/driver.c:266
really_probe+0x290/0xad0 drivers/base/dd.c:548
driver_probe_device+0x223/0x350 drivers/base/dd.c:721
__device_attach_driver+0x1d1/0x290 drivers/base/dd.c:828
bus_for_each_drv+0x162/0x1e0 drivers/base/bus.c:430
__device_attach+0x217/0x390 drivers/base/dd.c:894
bus_probe_device+0x1e4/0x290 drivers/base/bus.c:490
device_add+0x1459/0x1bf0 drivers/base/core.c:2487
usb_new_device.cold+0x540/0xcd0 drivers/usb/core/hub.c:2538
hub_port_connect drivers/usb/core/hub.c:5185 [inline]
hub_port_connect_change drivers/usb/core/hub.c:5325 [inline]
port_event drivers/usb/core/hub.c:5471 [inline]
hub_event+0x21cb/0x4300 drivers/usb/core/hub.c:5553
process_one_work+0x945/0x15c0 kernel/workqueue.c:2264
worker_thread+0x96/0xe20 kernel/workqueue.c:2410
kthread+0x318/0x420 kernel/kthread.c:255
ret_from_fork+0x24/0x30 arch/x86/entry/entry_64.S:352
Kernel Offset: disabled
Rebooting in 86400 seconds..


---
This bug 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 syzk...@googlegroups.com.

syzbot will keep track of this bug report. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
syzbot can test patches for this bug, for details see:
https://goo.gl/tpsmEJ#testing-patches

[Dan Carpenter]

We changed this from dev_err() to dev_WARN() in commit 0cb54a3e47cb
("USB: debugging code shouldn't alter control flow").

The difference between dev_WARN() and dev_err() is that dev_WARN()
prints a stack trace and if you have panic on OOPS enabled then it leads
to a panic. The dev_err() function just prints the error message.

Back in the day we didn't have usb emulators fuzz testing the kernel
so dev_WARN() didn't cause a problem for anyone, but these days the
dev_WARN() interferes with syzbot so let's change this to a dev_err().

Reported-by: syzbot+1bc2c2...@syzkaller.appspotmail.com
Signed-off-by: Dan Carpenter <dan.ca...@oracle.com>
---

drivers/usb/core/urb.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c
index da923ec17612..0980c1d2253d 100644
--- a/drivers/usb/core/urb.c
+++ b/drivers/usb/core/urb.c
@@ -475,7 +475,7 @@ int usb_submit_urb(struct urb *urb, gfp_t mem_flags)

/* Check that the pipe's type matches the endpoint's type */
if (usb_urb_ep_type_check(urb))
- dev_WARN(&dev->dev, "BOGUS urb xfer, pipe %x != type %x\n",
+ dev_err(&dev->dev, "BOGUS urb xfer, pipe %x != type %x\n",
usb_pipetype(urb->pipe), pipetypes[xfertype]);

/* Check against a simple/standard policy */
--
2.11.0

[Hillf Danton]

On Fri, 31 Jan 2020 08:06:52 +0300 Dan Carpenter wrote:
> We changed this from dev_err() to dev_WARN() in commit 0cb54a3e47cb
> ("USB: debugging code shouldn't alter control flow").
>
> The difference between dev_WARN() and dev_err() is that dev_WARN()
> prints a stack trace and if you have panic on OOPS enabled then it leads
> to a panic. The dev_err() function just prints the error message.
>
> Back in the day we didn't have usb emulators fuzz testing the kernel
> so dev_WARN() didn't cause a problem for anyone, but these days the

Another free option is perhaps to keep the devoted bot agile if it's
difficult to list anybody who was mauled by its articulate reports.

[Dan Carpenter]

On Fri, Jan 31, 2020 at 05:05:10PM +0800, Hillf Danton wrote:
>
> On Fri, 31 Jan 2020 08:06:52 +0300 Dan Carpenter wrote:
> > We changed this from dev_err() to dev_WARN() in commit 0cb54a3e47cb
> > ("USB: debugging code shouldn't alter control flow").
> >
> > The difference between dev_WARN() and dev_err() is that dev_WARN()
> > prints a stack trace and if you have panic on OOPS enabled then it leads
> > to a panic. The dev_err() function just prints the error message.
> >
> > Back in the day we didn't have usb emulators fuzz testing the kernel
> > so dev_WARN() didn't cause a problem for anyone, but these days the
>
> Another free option is perhaps to keep the devoted bot agile if it's
> difficult to list anybody who was mauled by its articulate reports.

It's difficult to parse this email. I get that you're being sarcastic
but I can't tell what you're being sarcastic about. :P

I think you're basically saying that syzbot should maintain a white
list of ignored Oopses. There are two problems with this: 1) Other
people run syzbot so everyone has to run into this bug and then add it
to their own white list. 2) If the kernel OOpes here then we cannot
test what happens next so it could be hiding bugs.

One idea is that there could be a kernel function which generates a
stack trace but is not an Oops.

regards,
dan carpenter

[Dmitry Vyukov]

Yes, this is needed for any kernel testing: not just syzbot, and not
just syzkaller, and not just fuzzing, literally for any kernel
testing.

We need a way to easily distinguish between kernel bugs and not bugs
in a black-and-white manner. Otherwise, of practical options testing
systems can either ignore lots of kernel bugs during testing
(unfortunately I see this happening, if it does not panic the system,
it's being ignored); or attach a human expert in each system to read
logs of each test run to sort kernel messages into bugs and non-bugs.
Both of these are not good options.

This is not about stack traces. There is already a function that does
this (print_stack() or something), and it's fine to print stacks (if
necessary, it produces lots of output so should not be taken lightly).
The way syzkaller detects these now is by "WARNING:" prefix.

This is also not about the exact way these are spelled out. We could
make "WARNING:" mean an invalid user input and use something else for
kernel bugs. But it just seems that WARNING===kernel bug is a really
good starting point (lots of debugging tools use it). Especially
taking into account the general recommendation of "don't panic kernel
if it can proceed", as the result lots of BUGs (in the sense that
these are kernel bugs) were turned into WARNINGs (or written out as
WARNINGs initially). Otherwise BUG would be a good marking for bugs,
except that it's not recommended to use in most cases.

I see lots of people also mention panic_on_warn in the context of
these reports. panic_on_warn here is only a red herring. It really
does not change anything. We could remove it, but still report
WARNINGs. But syzkaller also reports some things that don't panic
anyway. This is really about the criteria for kernel bug vs non-bug
(something that needs to be reported or not).

I would assume this criteria is also important for kernel developers
(people reading/writing code), especially for new subsystems. If I see
a WARNING in code (or just any kind of assertion), it's useful to know
whether it's something that can't happen under any circumstances and
if it happens it's really a serious logical flow somewhere; versus
just invalid input/rare/unexpected runtime condition. The first
category can be used as a basis for building my understanding of the
code. Not being able to understand type of assertion confuses you both
ways: if you think it's something that can't happen but at the same
time you see a way it can be violated, you question your understanding
of the code. Or the other way around, you are trying to figure out the
way how condition can become true, but you can only conclude that it's
always false, you also question your understanding of the code.

There were proposals to add a parallel set of macros, say, NOTICE
(name is obviously discussable). That would print something different
from WARNING: and optionally with a stack trace. Maybe also a parallel
set of _once and _ratelimited versions (esp since these can be
triggered). These would very unambiguously mean "this can be triggered
by users/devices; on low-memory, etc". But there was not enough
interest at the time and the discussion quickly died. Maybe it's time
to revive it.
You can imagine some value-added features on top: e.g. command
line/sysctl option to disable that output entirely, or reduce it to 1
line (no stack), if there are too many of these (we know these can be
triggered!), or if nobody is simply looking for them anyway (true for
most users).

+Eric, Steve, who got these WARNING-not-a-kernel-bug reports recently too

[Johan Hovold]

On Fri, Jan 31, 2020 at 08:06:52AM +0300, Dan Carpenter wrote:
> We changed this from dev_err() to dev_WARN() in commit 0cb54a3e47cb
> ("USB: debugging code shouldn't alter control flow").
>
> The difference between dev_WARN() and dev_err() is that dev_WARN()
> prints a stack trace and if you have panic on OOPS enabled then it leads
> to a panic. The dev_err() function just prints the error message.
>
> Back in the day we didn't have usb emulators fuzz testing the kernel
> so dev_WARN() didn't cause a problem for anyone, but these days the
> dev_WARN() interferes with syzbot so let's change this to a dev_err().

The commit you refer to did more than just change dev_err() to
dev_WARN(); it also stopped returning an error in case a driver
submitted an URB for an endpoint of the wrong type. At that point in
time all this was dependent on CONFIG_USB_DEBUG however.

> Reported-by: syzbot+1bc2c2...@syzkaller.appspotmail.com
> Signed-off-by: Dan Carpenter <dan.ca...@oracle.com>
> ---
>
> drivers/usb/core/urb.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c
> index da923ec17612..0980c1d2253d 100644
> --- a/drivers/usb/core/urb.c
> +++ b/drivers/usb/core/urb.c
> @@ -475,7 +475,7 @@ int usb_submit_urb(struct urb *urb, gfp_t mem_flags)
>
> /* Check that the pipe's type matches the endpoint's type */
> if (usb_urb_ep_type_check(urb))
> - dev_WARN(&dev->dev, "BOGUS urb xfer, pipe %x != type %x\n",
> + dev_err(&dev->dev, "BOGUS urb xfer, pipe %x != type %x\n",
> usb_pipetype(urb->pipe), pipetypes[xfertype]);
>
> /* Check against a simple/standard policy */

It seems this change would just be papering over these driver bugs. The
dev_WARN() is there in the first place to allow us to catch them.

Even if it takes some work, it should be doable to track down and add
the missing sanity checks to the drivers that lack them. Some have
already been fixed, and I have some more pending patches to fix or add
helpers to simplify fixing the remaining ones.

Johan

[Johan Hovold]

On Fri, Jan 31, 2020 at 12:19:39PM +0100, Dmitry Vyukov wrote:

> I see lots of people also mention panic_on_warn in the context of
> these reports. panic_on_warn here is only a red herring. It really
> does not change anything. We could remove it, but still report
> WARNINGs. But syzkaller also reports some things that don't panic
> anyway. This is really about the criteria for kernel bug vs non-bug
> (something that needs to be reported or not).

Mentioning panic_on_warn is relevant to determine whether a fix needs to
be backported or not. Some of the bugs in question are mostly benign in
the sense that they are unlikely to crash your machine, but we'd still
want them in in stable due to panic_on_warn and automatic testing.

Johan

[Dan Carpenter]

On Fri, Jan 31, 2020 at 02:30:04PM +0100, Johan Hovold wrote:
> > Reported-by: syzbot+1bc2c2...@syzkaller.appspotmail.com
> > Signed-off-by: Dan Carpenter <dan.ca...@oracle.com>
> > ---
> >
> > drivers/usb/core/urb.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c
> > index da923ec17612..0980c1d2253d 100644
> > --- a/drivers/usb/core/urb.c
> > +++ b/drivers/usb/core/urb.c
> > @@ -475,7 +475,7 @@ int usb_submit_urb(struct urb *urb, gfp_t mem_flags)
> >
> > /* Check that the pipe's type matches the endpoint's type */
> > if (usb_urb_ep_type_check(urb))
> > - dev_WARN(&dev->dev, "BOGUS urb xfer, pipe %x != type %x\n",
> > + dev_err(&dev->dev, "BOGUS urb xfer, pipe %x != type %x\n",
> > usb_pipetype(urb->pipe), pipetypes[xfertype]);
> >
> > /* Check against a simple/standard policy */
>
> It seems this change would just be papering over these driver bugs. The
> dev_WARN() is there in the first place to allow us to catch them.
>
> Even if it takes some work, it should be doable to track down and add
> the missing sanity checks to the drivers that lack them. Some have
> already been fixed, and I have some more pending patches to fix or add
> helpers to simplify fixing the remaining ones.

Ah, fine. I misunderstood what the warning message was about.

regards,
dan carpenter

[Steven Rostedt]

On Fri, 31 Jan 2020 12:19:39 +0100
Dmitry Vyukov <dvy...@google.com> wrote:

> +Eric, Steve, who got these WARNING-not-a-kernel-bug reports recently too

I've been trying to convert all WARN_ON() in my code to be only
triggered if something happens where I don't expect it to happen, and
there's either a bug in the code, or I missed something in the design
of the code. That is, if a WARN_ON() triggers, it means I need to have
a good look at the code to figure out why.

I like the idea of a NOTICE() that can be used for hardware bugs or bad
user input. Something to say, "the kernel code is fine, but we received
something we did not expect", which to me is different than a bug in
the kernel. Although, it could lead to finding a bug.

-- Steve

[Greg KH]

On Fri, Jan 31, 2020 at 08:06:52AM +0300, Dan Carpenter wrote:

Like others said, we should have the stack trace here. So can you
change this to dev_warn() and a stacktrace?

thanks,

greg k-h

[Alan Stern]

In fact we want both a stack trace and a syzbot notification, because
this particular error indicates a bug in a kernel driver. Therefore
dev_WARN is appropriate.

Alan Stern

> thanks,
>
> greg k-h

[Greg KH]

Ok, nevermind, you are right we should fix up the driver if that
happens.

greg k-h

[Dan Carpenter]

Let's just fix the driver instead. That was the message I got from the
thread.

regards,
dan carpenter