| Message ID | 20200131050651.hlq27kehtir3agf2@kili.mountain (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: core: urb: change a dev_WARN() to dev_err() for syzbot | expand |
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
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
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
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
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
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
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 */
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(-)