* WARNING in ar5523_cmd/usb_submit_urb @ 2020-01-29 12:27 syzbot 2020-01-31 5:06 ` [PATCH] usb: core: urb: change a dev_WARN() to dev_err() for syzbot Dan Carpenter [not found] ` <20200131090510.7112-1-hdanton@sina.com> 0 siblings, 2 replies; 12+ messages in thread From: syzbot @ 2020-01-29 12:27 UTC (permalink / raw) To: andreyknvl, gregkh, ingrassia, linux-kernel, linux-usb, syzkaller-bugs 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+1bc2c2afd44f820a669f@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 syzkaller@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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] usb: core: urb: change a dev_WARN() to dev_err() for syzbot 2020-01-29 12:27 WARNING in ar5523_cmd/usb_submit_urb syzbot @ 2020-01-31 5:06 ` Dan Carpenter 2020-01-31 13:30 ` Johan Hovold 2020-02-10 19:04 ` Greg KH [not found] ` <20200131090510.7112-1-hdanton@sina.com> 1 sibling, 2 replies; 12+ messages in thread From: Dan Carpenter @ 2020-01-31 5:06 UTC (permalink / raw) To: gregkh, Alan Stern Cc: syzbot, andreyknvl, ingrassia, linux-kernel, linux-usb, syzkaller-bugs 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+1bc2c2afd44f820a669f@syzkaller.appspotmail.com Signed-off-by: Dan Carpenter <dan.carpenter@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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] usb: core: urb: change a dev_WARN() to dev_err() for syzbot 2020-01-31 5:06 ` [PATCH] usb: core: urb: change a dev_WARN() to dev_err() for syzbot Dan Carpenter @ 2020-01-31 13:30 ` Johan Hovold 2020-01-31 13:39 ` Dan Carpenter 2020-02-10 19:04 ` Greg KH 1 sibling, 1 reply; 12+ messages in thread From: Johan Hovold @ 2020-01-31 13:30 UTC (permalink / raw) To: Dan Carpenter Cc: gregkh, Alan Stern, syzbot, andreyknvl, ingrassia, linux-kernel, linux-usb, syzkaller-bugs 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+1bc2c2afd44f820a669f@syzkaller.appspotmail.com > Signed-off-by: Dan Carpenter <dan.carpenter@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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] usb: core: urb: change a dev_WARN() to dev_err() for syzbot 2020-01-31 13:30 ` Johan Hovold @ 2020-01-31 13:39 ` Dan Carpenter 0 siblings, 0 replies; 12+ messages in thread From: Dan Carpenter @ 2020-01-31 13:39 UTC (permalink / raw) To: Johan Hovold Cc: gregkh, Alan Stern, syzbot, andreyknvl, ingrassia, linux-kernel, linux-usb, syzkaller-bugs On Fri, Jan 31, 2020 at 02:30:04PM +0100, Johan Hovold wrote: > > Reported-by: syzbot+1bc2c2afd44f820a669f@syzkaller.appspotmail.com > > Signed-off-by: Dan Carpenter <dan.carpenter@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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] usb: core: urb: change a dev_WARN() to dev_err() for syzbot 2020-01-31 5:06 ` [PATCH] usb: core: urb: change a dev_WARN() to dev_err() for syzbot Dan Carpenter 2020-01-31 13:30 ` Johan Hovold @ 2020-02-10 19:04 ` Greg KH 2020-02-10 21:11 ` Alan Stern 2020-02-11 6:03 ` Dan Carpenter 1 sibling, 2 replies; 12+ messages in thread From: Greg KH @ 2020-02-10 19:04 UTC (permalink / raw) To: Dan Carpenter Cc: Alan Stern, syzbot, andreyknvl, ingrassia, linux-kernel, linux-usb, syzkaller-bugs 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(). > > Reported-by: syzbot+1bc2c2afd44f820a669f@syzkaller.appspotmail.com > Signed-off-by: Dan Carpenter <dan.carpenter@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]); 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] usb: core: urb: change a dev_WARN() to dev_err() for syzbot 2020-02-10 19:04 ` Greg KH @ 2020-02-10 21:11 ` Alan Stern 2020-02-10 21:50 ` Greg KH 2020-02-11 6:03 ` Dan Carpenter 1 sibling, 1 reply; 12+ messages in thread From: Alan Stern @ 2020-02-10 21:11 UTC (permalink / raw) To: Greg KH Cc: Dan Carpenter, syzbot, andreyknvl, ingrassia, linux-kernel, linux-usb, syzkaller-bugs On Mon, 10 Feb 2020, Greg KH wrote: > 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(). > > > > Reported-by: syzbot+1bc2c2afd44f820a669f@syzkaller.appspotmail.com > > Signed-off-by: Dan Carpenter <dan.carpenter@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]); > > Like others said, we should have the stack trace here. So can you > change this to dev_warn() and a stacktrace? 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] usb: core: urb: change a dev_WARN() to dev_err() for syzbot 2020-02-10 21:11 ` Alan Stern @ 2020-02-10 21:50 ` Greg KH 0 siblings, 0 replies; 12+ messages in thread From: Greg KH @ 2020-02-10 21:50 UTC (permalink / raw) To: Alan Stern Cc: Dan Carpenter, syzbot, andreyknvl, ingrassia, linux-kernel, linux-usb, syzkaller-bugs On Mon, Feb 10, 2020 at 04:11:10PM -0500, Alan Stern wrote: > On Mon, 10 Feb 2020, Greg KH wrote: > > > 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(). > > > > > > Reported-by: syzbot+1bc2c2afd44f820a669f@syzkaller.appspotmail.com > > > Signed-off-by: Dan Carpenter <dan.carpenter@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]); > > > > Like others said, we should have the stack trace here. So can you > > change this to dev_warn() and a stacktrace? > > 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. Ok, nevermind, you are right we should fix up the driver if that happens. greg k-h ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] usb: core: urb: change a dev_WARN() to dev_err() for syzbot 2020-02-10 19:04 ` Greg KH 2020-02-10 21:11 ` Alan Stern @ 2020-02-11 6:03 ` Dan Carpenter 1 sibling, 0 replies; 12+ messages in thread From: Dan Carpenter @ 2020-02-11 6:03 UTC (permalink / raw) To: Greg KH Cc: Alan Stern, syzbot, andreyknvl, ingrassia, linux-kernel, linux-usb, syzkaller-bugs On Mon, Feb 10, 2020 at 11:04:19AM -0800, Greg KH wrote: > 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(). > > > > Reported-by: syzbot+1bc2c2afd44f820a669f@syzkaller.appspotmail.com > > Signed-off-by: Dan Carpenter <dan.carpenter@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]); > > Like others said, we should have the stack trace here. So can you > change this to dev_warn() and a stacktrace? > Let's just fix the driver instead. That was the message I got from the thread. regards, dan carpenter ^ permalink raw reply [flat|nested] 12+ messages in thread
[parent not found: <20200131090510.7112-1-hdanton@sina.com>]
* Re: [PATCH] usb: core: urb: change a dev_WARN() to dev_err() for syzbot [not found] ` <20200131090510.7112-1-hdanton@sina.com> @ 2020-01-31 10:16 ` Dan Carpenter 2020-01-31 11:19 ` Dmitry Vyukov 0 siblings, 1 reply; 12+ messages in thread From: Dan Carpenter @ 2020-01-31 10:16 UTC (permalink / raw) To: Hillf Danton Cc: gregkh, Alan Stern, syzbot, andreyknvl, ingrassia, linux-kernel, linux-usb, syzkaller-bugs 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] usb: core: urb: change a dev_WARN() to dev_err() for syzbot 2020-01-31 10:16 ` Dan Carpenter @ 2020-01-31 11:19 ` Dmitry Vyukov 2020-01-31 13:37 ` Johan Hovold 2020-01-31 14:25 ` Steven Rostedt 0 siblings, 2 replies; 12+ messages in thread From: Dmitry Vyukov @ 2020-01-31 11:19 UTC (permalink / raw) To: Dan Carpenter, syzkaller, Eric Dumazet, Steven Rostedt Cc: Hillf Danton, Greg Kroah-Hartman, Alan Stern, syzbot, Andrey Konovalov, ingrassia, LKML, USB list, syzkaller-bugs On Fri, Jan 31, 2020 at 11:17 AM Dan Carpenter <dan.carpenter@oracle.com> wrote: > > 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. 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] usb: core: urb: change a dev_WARN() to dev_err() for syzbot 2020-01-31 11:19 ` Dmitry Vyukov @ 2020-01-31 13:37 ` Johan Hovold 2020-01-31 14:25 ` Steven Rostedt 1 sibling, 0 replies; 12+ messages in thread From: Johan Hovold @ 2020-01-31 13:37 UTC (permalink / raw) To: Dmitry Vyukov Cc: Dan Carpenter, syzkaller, Eric Dumazet, Steven Rostedt, Hillf Danton, Greg Kroah-Hartman, Alan Stern, syzbot, Andrey Konovalov, ingrassia, LKML, USB list, syzkaller-bugs 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] usb: core: urb: change a dev_WARN() to dev_err() for syzbot 2020-01-31 11:19 ` Dmitry Vyukov 2020-01-31 13:37 ` Johan Hovold @ 2020-01-31 14:25 ` Steven Rostedt 1 sibling, 0 replies; 12+ messages in thread From: Steven Rostedt @ 2020-01-31 14:25 UTC (permalink / raw) To: Dmitry Vyukov Cc: Dan Carpenter, syzkaller, Eric Dumazet, Hillf Danton, Greg Kroah-Hartman, Alan Stern, syzbot, Andrey Konovalov, ingrassia, LKML, USB list, syzkaller-bugs On Fri, 31 Jan 2020 12:19:39 +0100 Dmitry Vyukov <dvyukov@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 ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2020-02-11 6:04 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-01-29 12:27 WARNING in ar5523_cmd/usb_submit_urb syzbot 2020-01-31 5:06 ` [PATCH] usb: core: urb: change a dev_WARN() to dev_err() for syzbot Dan Carpenter 2020-01-31 13:30 ` Johan Hovold 2020-01-31 13:39 ` Dan Carpenter 2020-02-10 19:04 ` Greg KH 2020-02-10 21:11 ` Alan Stern 2020-02-10 21:50 ` Greg KH 2020-02-11 6:03 ` Dan Carpenter [not found] ` <20200131090510.7112-1-hdanton@sina.com> 2020-01-31 10:16 ` Dan Carpenter 2020-01-31 11:19 ` Dmitry Vyukov 2020-01-31 13:37 ` Johan Hovold 2020-01-31 14:25 ` Steven Rostedt
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).