[2/2] USB: usbdevfs: restore warning for nonsensical flags

[2/2] USB: usbdevfs: restore warning for nonsensical flags

[Oliver Neukum]

On Mi, 2018-09-05 at 15:07 +0200, Greg KH wrote:
> On Wed, Sep 05, 2018 at 03:02:48PM +0200, Oliver Neukum wrote:
> > On Mi, 2018-09-05 at 14:19 +0200, Greg KH wrote:
> > > On Wed, Sep 05, 2018 at 12:07:03PM +0200, Oliver Neukum wrote:

> > > > +	if (!allow_short && uurb->flags & USBDEVFS_URB_SHORT_NOT_OK)
> > > > +		dev_warn(&ps->dev->dev, "Requested nonsensical USBDEVFS_URB_SHORT_NOT_OK.\n");
> > > > +	if (!allow_zero && uurb->flags & USBDEVFS_URB_ZERO_PACKET)
> > > > +		dev_warn(&ps->dev->dev, "Requested nonsensical USBDEVFS_URB_ZERO_PACKET.\n");
> > > 
> > > We should not make it trivial for userspace to spam the kernel log if at
> > > all possible.  Returning an error is probably the better thing to do
> > > here, not just silently fix it up or ignore it.
> > 
> > That means a change in the API in a way that makes orking systems fail.
> 
> Ah, good point.

Well, but do we want to do this in the next major release even if we
cannot do it in a stable release?

>   I guess they were hitting the same dev_WARN() messages
> today anyway, right?

Yes. And for a kernel problem you really want the stack traces.
Still, that does not tell us that we want to print a message if
user space messes up. So dev_warn() or nothing?

> > Do you want an extra version for stable?
> 
> No, but why was this patch not marked for stable?

I was under the impression that it was. This is a separate
patch because you could argue that it is unnecessary or that stable
and the next release should diverge on whether to take it.

	Regards
		Oliver

[2/2] USB: usbdevfs: restore warning for nonsensical flags

[Greg Kroah-Hartman]

On Thu, Sep 06, 2018 at 11:34:04AM -0400, Alan Stern wrote:
> On Thu, 6 Sep 2018, Oliver Neukum wrote:
> 
> > On Mi, 2018-09-05 at 15:07 +0200, Greg KH wrote:
> > > On Wed, Sep 05, 2018 at 03:02:48PM +0200, Oliver Neukum wrote:
> > > > On Mi, 2018-09-05 at 14:19 +0200, Greg KH wrote:
> > > > > On Wed, Sep 05, 2018 at 12:07:03PM +0200, Oliver Neukum wrote:
> > 
> > > > > > +	if (!allow_short && uurb->flags & USBDEVFS_URB_SHORT_NOT_OK)
> > > > > > +		dev_warn(&ps->dev->dev, "Requested nonsensical USBDEVFS_URB_SHORT_NOT_OK.\n");
> > > > > > +	if (!allow_zero && uurb->flags & USBDEVFS_URB_ZERO_PACKET)
> > > > > > +		dev_warn(&ps->dev->dev, "Requested nonsensical USBDEVFS_URB_ZERO_PACKET.\n");
> > > > > 
> > > > > We should not make it trivial for userspace to spam the kernel log if at
> > > > > all possible.  Returning an error is probably the better thing to do
> > > > > here, not just silently fix it up or ignore it.
> > > > 
> > > > That means a change in the API in a way that makes orking systems fail.
> > > 
> > > Ah, good point.
> > 
> > Well, but do we want to do this in the next major release even if we
> > cannot do it in a stable release?
> > 
> > >   I guess they were hitting the same dev_WARN() messages
> > > today anyway, right?
> > 
> > Yes. And for a kernel problem you really want the stack traces.
> > Still, that does not tell us that we want to print a message if
> > user space messes up. So dev_warn() or nothing?
> 
> An alternative is for usbfs to silently fix the flags when they are
> wrong.  Would that be any better?

Probably not.  I'll take the original patches now and see if there is
any complaints by users.

thanks,

greg k-h

[2/2] USB: usbdevfs: restore warning for nonsensical flags

[Alan Stern]

On Thu, 6 Sep 2018, Oliver Neukum wrote:

> On Mi, 2018-09-05 at 15:07 +0200, Greg KH wrote:
> > On Wed, Sep 05, 2018 at 03:02:48PM +0200, Oliver Neukum wrote:
> > > On Mi, 2018-09-05 at 14:19 +0200, Greg KH wrote:
> > > > On Wed, Sep 05, 2018 at 12:07:03PM +0200, Oliver Neukum wrote:
> 
> > > > > +	if (!allow_short && uurb->flags & USBDEVFS_URB_SHORT_NOT_OK)
> > > > > +		dev_warn(&ps->dev->dev, "Requested nonsensical USBDEVFS_URB_SHORT_NOT_OK.\n");
> > > > > +	if (!allow_zero && uurb->flags & USBDEVFS_URB_ZERO_PACKET)
> > > > > +		dev_warn(&ps->dev->dev, "Requested nonsensical USBDEVFS_URB_ZERO_PACKET.\n");
> > > > 
> > > > We should not make it trivial for userspace to spam the kernel log if at
> > > > all possible.  Returning an error is probably the better thing to do
> > > > here, not just silently fix it up or ignore it.
> > > 
> > > That means a change in the API in a way that makes orking systems fail.
> > 
> > Ah, good point.
> 
> Well, but do we want to do this in the next major release even if we
> cannot do it in a stable release?
> 
> >   I guess they were hitting the same dev_WARN() messages
> > today anyway, right?
> 
> Yes. And for a kernel problem you really want the stack traces.
> Still, that does not tell us that we want to print a message if
> user space messes up. So dev_warn() or nothing?

An alternative is for usbfs to silently fix the flags when they are
wrong.  Would that be any better?

Alan Stern

> > > Do you want an extra version for stable?
> > 
> > No, but why was this patch not marked for stable?
> 
> I was under the impression that it was. This is a separate
> patch because you could argue that it is unnecessary or that stable
> and the next release should diverge on whether to take it.
> 
> 	Regards
> 		Oliver

[2/2] USB: usbdevfs: restore warning for nonsensical flags

[Greg Kroah-Hartman]

On Wed, Sep 05, 2018 at 03:02:48PM +0200, Oliver Neukum wrote:
> On Mi, 2018-09-05 at 14:19 +0200, Greg KH wrote:
> > On Wed, Sep 05, 2018 at 12:07:03PM +0200, Oliver Neukum wrote:
> > > If we filter flags before they reach the core we need to generate our
> > > own warnings.
> > > 
> > > Signed-off-by: Oliver Neukum <oneukum@suse.com>
> > > Fixes: 0cb54a3e47cb ("USB: debugging code shouldn't alter control flow")
> > > ---
> > >  drivers/usb/core/devio.c | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > > 
> > > diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
> > > index 263dd2f309fb..244417d0dfd1 100644
> > > --- a/drivers/usb/core/devio.c
> > > +++ b/drivers/usb/core/devio.c
> > > @@ -1697,6 +1697,11 @@ static int proc_do_submiturb(struct usb_dev_state *ps, struct usbdevfs_urb *uurb
> > >  		u |= URB_NO_INTERRUPT;
> > >  	as->urb->transfer_flags = u;
> > >  
> > > +	if (!allow_short && uurb->flags & USBDEVFS_URB_SHORT_NOT_OK)
> > > +		dev_warn(&ps->dev->dev, "Requested nonsensical USBDEVFS_URB_SHORT_NOT_OK.\n");
> > > +	if (!allow_zero && uurb->flags & USBDEVFS_URB_ZERO_PACKET)
> > > +		dev_warn(&ps->dev->dev, "Requested nonsensical USBDEVFS_URB_ZERO_PACKET.\n");
> > 
> > We should not make it trivial for userspace to spam the kernel log if at
> > all possible.  Returning an error is probably the better thing to do
> > here, not just silently fix it up or ignore it.
> 
> That means a change in the API in a way that makes orking systems fail.

Ah, good point.  I guess they were hitting the same dev_WARN() messages
today anyway, right?

> Do you want an extra version for stable?

No, but why was this patch not marked for stable?

thanks,

greg k-h

[2/2] USB: usbdevfs: restore warning for nonsensical flags

[Oliver Neukum]

On Mi, 2018-09-05 at 14:19 +0200, Greg KH wrote:
> On Wed, Sep 05, 2018 at 12:07:03PM +0200, Oliver Neukum wrote:
> > If we filter flags before they reach the core we need to generate our
> > own warnings.
> > 
> > Signed-off-by: Oliver Neukum <oneukum@suse.com>
> > Fixes: 0cb54a3e47cb ("USB: debugging code shouldn't alter control flow")
> > ---
> >  drivers/usb/core/devio.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
> > index 263dd2f309fb..244417d0dfd1 100644
> > --- a/drivers/usb/core/devio.c
> > +++ b/drivers/usb/core/devio.c
> > @@ -1697,6 +1697,11 @@ static int proc_do_submiturb(struct usb_dev_state *ps, struct usbdevfs_urb *uurb
> >  		u |= URB_NO_INTERRUPT;
> >  	as->urb->transfer_flags = u;
> >  
> > +	if (!allow_short && uurb->flags & USBDEVFS_URB_SHORT_NOT_OK)
> > +		dev_warn(&ps->dev->dev, "Requested nonsensical USBDEVFS_URB_SHORT_NOT_OK.\n");
> > +	if (!allow_zero && uurb->flags & USBDEVFS_URB_ZERO_PACKET)
> > +		dev_warn(&ps->dev->dev, "Requested nonsensical USBDEVFS_URB_ZERO_PACKET.\n");
> 
> We should not make it trivial for userspace to spam the kernel log if at
> all possible.  Returning an error is probably the better thing to do
> here, not just silently fix it up or ignore it.

That means a change in the API in a way that makes orking systems fail.
Do you want an extra version for stable?

	Regards
		Oliver

[2/2] USB: usbdevfs: restore warning for nonsensical flags

[Greg Kroah-Hartman]

On Wed, Sep 05, 2018 at 12:07:03PM +0200, Oliver Neukum wrote:
> If we filter flags before they reach the core we need to generate our
> own warnings.
> 
> Signed-off-by: Oliver Neukum <oneukum@suse.com>
> Fixes: 0cb54a3e47cb ("USB: debugging code shouldn't alter control flow")
> ---
>  drivers/usb/core/devio.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
> index 263dd2f309fb..244417d0dfd1 100644
> --- a/drivers/usb/core/devio.c
> +++ b/drivers/usb/core/devio.c
> @@ -1697,6 +1697,11 @@ static int proc_do_submiturb(struct usb_dev_state *ps, struct usbdevfs_urb *uurb
>  		u |= URB_NO_INTERRUPT;
>  	as->urb->transfer_flags = u;
>  
> +	if (!allow_short && uurb->flags & USBDEVFS_URB_SHORT_NOT_OK)
> +		dev_warn(&ps->dev->dev, "Requested nonsensical USBDEVFS_URB_SHORT_NOT_OK.\n");
> +	if (!allow_zero && uurb->flags & USBDEVFS_URB_ZERO_PACKET)
> +		dev_warn(&ps->dev->dev, "Requested nonsensical USBDEVFS_URB_ZERO_PACKET.\n");

We should not make it trivial for userspace to spam the kernel log if at
all possible.  Returning an error is probably the better thing to do
here, not just silently fix it up or ignore it.

thanks,

greg k-h

[2/2] USB: usbdevfs: restore warning for nonsensical flags

[Oliver Neukum]

If we filter flags before they reach the core we need to generate our
own warnings.

Signed-off-by: Oliver Neukum <oneukum@suse.com>
Fixes: 0cb54a3e47cb ("USB: debugging code shouldn't alter control flow")
---
 drivers/usb/core/devio.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
index 263dd2f309fb..244417d0dfd1 100644
--- a/drivers/usb/core/devio.c
+++ b/drivers/usb/core/devio.c
@@ -1697,6 +1697,11 @@ static int proc_do_submiturb(struct usb_dev_state *ps, struct usbdevfs_urb *uurb
 		u |= URB_NO_INTERRUPT;
 	as->urb->transfer_flags = u;
 
+	if (!allow_short && uurb->flags & USBDEVFS_URB_SHORT_NOT_OK)
+		dev_warn(&ps->dev->dev, "Requested nonsensical USBDEVFS_URB_SHORT_NOT_OK.\n");
+	if (!allow_zero && uurb->flags & USBDEVFS_URB_ZERO_PACKET)
+		dev_warn(&ps->dev->dev, "Requested nonsensical USBDEVFS_URB_ZERO_PACKET.\n");
+
 	as->urb->transfer_buffer_length = uurb->buffer_length;
 	as->urb->setup_packet = (unsigned char *)dr;
 	dr = NULL;