[PATCH] staging: most: usb: fix exception handling

[PATCH] staging: most: usb: fix exception handling

[Christian Gromm]

This patch fixes error handling on function parameters.

Signed-off-by: Christian Gromm <christian.gromm@microchip.com>
---
 drivers/staging/most/usb/usb.c | 33 +++++++++++++++++----------------
 1 file changed, 17 insertions(+), 16 deletions(-)

diff --git a/drivers/staging/most/usb/usb.c b/drivers/staging/most/usb/usb.c
index e8c5a8c..e5276524 100644
--- a/drivers/staging/most/usb/usb.c
+++ b/drivers/staging/most/usb/usb.c
@@ -229,14 +229,14 @@ static unsigned int get_stream_frame_size(struct most_channel_config *cfg)
  */
 static int hdm_poison_channel(struct most_interface *iface, int channel)
 {
-	struct most_dev *mdev = to_mdev(iface);
+	struct most_dev *mdev;
 	unsigned long flags;
 	spinlock_t *lock; /* temp. lock */
 
 	if (unlikely(!iface)) {
-		dev_warn(&mdev->usb_device->dev, "Poison: Bad interface.\n");
-		return -EIO;
+		return -EFAULT;
 	}
+	mdev = to_mdev(iface);
 	if (unlikely(channel < 0 || channel >= iface->num_channels)) {
 		dev_warn(&mdev->usb_device->dev, "Channel ID out of range.\n");
 		return -ECHRNG;
@@ -278,13 +278,13 @@ static int hdm_add_padding(struct most_dev *mdev, int channel, struct mbo *mbo)
 	unsigned int j, num_frames;
 
 	if (!frame_size)
-		return -EIO;
+		return -EFAULT;
 	num_frames = mbo->buffer_length / frame_size;
 
 	if (num_frames < 1) {
 		dev_err(&mdev->usb_device->dev,
 			"Missed minimal transfer unit.\n");
-		return -EIO;
+		return -EINVAL;
 	}
 
 	for (j = num_frames - 1; j > 0; j--)
@@ -312,7 +312,7 @@ static int hdm_remove_padding(struct most_dev *mdev, int channel,
 	unsigned int j, num_frames;
 
 	if (!frame_size)
-		return -EIO;
+		return -EFAULT;
 	num_frames = mbo->processed_length / USB_MTU;
 
 	for (j = 1; j < num_frames; j++)
@@ -560,7 +560,7 @@ static int hdm_enqueue(struct most_interface *iface, int channel,
 	void *virt_address;
 
 	if (unlikely(!iface || !mbo))
-		return -EIO;
+		return -EFAULT;
 	if (unlikely(iface->num_channels <= channel || channel < 0))
 		return -ECHRNG;
 
@@ -666,18 +666,18 @@ static int hdm_configure_channel(struct most_interface *iface, int channel,
 {
 	unsigned int num_frames;
 	unsigned int frame_size;
-	struct most_dev *mdev = to_mdev(iface);
-	struct device *dev = &mdev->usb_device->dev;
+	struct most_dev *mdev;
+	struct device *dev;
 
+	if (unlikely(!iface || !conf))
+		return -EFAULT;
+	mdev = to_mdev(iface);
+	dev = &mdev->usb_device->dev;
 	mdev->is_channel_healthy[channel] = true;
 	mdev->clear_work[channel].channel = channel;
 	mdev->clear_work[channel].mdev = mdev;
 	INIT_WORK(&mdev->clear_work[channel].ws, wq_clear_halt);
 
-	if (unlikely(!iface || !conf)) {
-		dev_err(dev, "Bad interface or config pointer.\n");
-		return -EINVAL;
-	}
 	if (unlikely(channel < 0 || channel >= iface->num_channels)) {
 		dev_err(dev, "Channel ID out of range.\n");
 		return -EINVAL;
@@ -747,11 +747,12 @@ static void hdm_request_netinfo(struct most_interface *iface, int channel,
 {
 	struct most_dev *mdev;
 
-	BUG_ON(!iface);
+	if (!iface || !on_netinfo) {
+		WARN_ON(1);
+		return;
+	}
 	mdev = to_mdev(iface);
 	mdev->on_netinfo = on_netinfo;
-	if (!on_netinfo)
-		return;
 
 	mdev->link_stat_timer.expires = jiffies + HZ;
 	mod_timer(&mdev->link_stat_timer, mdev->link_stat_timer.expires);
-- 
2.7.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: most: usb: fix exception handling

[Greg KH]

On Mon, May 04, 2020 at 03:44:00PM +0200, Christian Gromm wrote:
> This patch fixes error handling on function parameters.

What does that mean?  If I don't understand it, I think it needs to be
made a lot more explicit as to why you are making these changes :) 

> Signed-off-by: Christian Gromm <christian.gromm@microchip.com>

Any "Fixes:" tag for this?

SHould it go to stable if it really resolves issues?

thanks,

greg k-h
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: most: usb: fix exception handling

[Greg KH]

On Mon, May 04, 2020 at 03:44:00PM +0200, Christian Gromm wrote:
> This patch fixes error handling on function parameters.
> 
> Signed-off-by: Christian Gromm <christian.gromm@microchip.com>
> ---
>  drivers/staging/most/usb/usb.c | 33 +++++++++++++++++----------------
>  1 file changed, 17 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/staging/most/usb/usb.c b/drivers/staging/most/usb/usb.c
> index e8c5a8c..e5276524 100644
> --- a/drivers/staging/most/usb/usb.c
> +++ b/drivers/staging/most/usb/usb.c
> @@ -229,14 +229,14 @@ static unsigned int get_stream_frame_size(struct most_channel_config *cfg)
>   */
>  static int hdm_poison_channel(struct most_interface *iface, int channel)
>  {
> -	struct most_dev *mdev = to_mdev(iface);
> +	struct most_dev *mdev;
>  	unsigned long flags;
>  	spinlock_t *lock; /* temp. lock */
>  
>  	if (unlikely(!iface)) {
> -		dev_warn(&mdev->usb_device->dev, "Poison: Bad interface.\n");
> -		return -EIO;
> +		return -EFAULT;

-EFAULT is ONLY for when you have an error with copying memory to/from
userspace.

This should just be -EINVAL, right?

And how can iface ever be NULL?

And why unlikely() there, can you measure the difference with/without
it?  If not, please drop as the compiler/CPU can do it faster than you
ever can.

thanks,

greg k-h
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: most: usb: fix exception handling

[Christian.Gromm]

On Mon, 2020-05-04 at 15:54 +0200, Greg KH wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> On Mon, May 04, 2020 at 03:44:00PM +0200, Christian Gromm wrote:
> > This patch fixes error handling on function parameters.
> 
> What does that mean?  If I don't understand it, I think it needs to
> be
> made a lot more explicit as to why you are making these changes :)
> 
> > Signed-off-by: Christian Gromm <christian.gromm@microchip.com>
> 
> Any "Fixes:" tag for this?

No. Just wanted to fix some obvious things, before adding
it to stable, as discussed in our last email thread.
> 
> SHould it go to stable if it really resolves issues?

No. Once you accept this, I'll add it to stable anyway.

thanks,
Chris

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: most: usb: fix exception handling

[Christian.Gromm]

On Mon, 2020-05-04 at 15:55 +0200, Greg KH wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> On Mon, May 04, 2020 at 03:44:00PM +0200, Christian Gromm wrote:
> > This patch fixes error handling on function parameters.
> > 
> > Signed-off-by: Christian Gromm <christian.gromm@microchip.com>
> > ---
> >  drivers/staging/most/usb/usb.c | 33 +++++++++++++++++-------------
> > ---
> >  1 file changed, 17 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/staging/most/usb/usb.c
> > b/drivers/staging/most/usb/usb.c
> > index e8c5a8c..e5276524 100644
> > --- a/drivers/staging/most/usb/usb.c
> > +++ b/drivers/staging/most/usb/usb.c
> > @@ -229,14 +229,14 @@ static unsigned int
> > get_stream_frame_size(struct most_channel_config *cfg)
> >   */
> >  static int hdm_poison_channel(struct most_interface *iface, int
> > channel)
> >  {
> > -     struct most_dev *mdev = to_mdev(iface);
> > +     struct most_dev *mdev;
> >       unsigned long flags;
> >       spinlock_t *lock; /* temp. lock */
> > 
> >       if (unlikely(!iface)) {
> > -             dev_warn(&mdev->usb_device->dev, "Poison: Bad
> > interface.\n");
> > -             return -EIO;
> > +             return -EFAULT;
> 
> -EFAULT is ONLY for when you have an error with copying memory
> to/from
> userspace.

Ok.

> 
> This should just be -EINVAL, right?
> 
> And how can iface ever be NULL?

It should never become NULL. But you never know, right?
Too paranoid?

> 
> And why unlikely() there, can you measure the difference with/without
> it?  If not, please drop as the compiler/CPU can do it faster than
> you
> ever can.
> 
> thanks,
> 
> greg k-h
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: most: usb: fix exception handling

[Greg KH]

On Mon, May 04, 2020 at 03:22:08PM +0000, Christian.Gromm@microchip.com wrote:
> On Mon, 2020-05-04 at 15:55 +0200, Greg KH wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you
> > know the content is safe
> > 
> > On Mon, May 04, 2020 at 03:44:00PM +0200, Christian Gromm wrote:
> > > This patch fixes error handling on function parameters.
> > > 
> > > Signed-off-by: Christian Gromm <christian.gromm@microchip.com>
> > > ---
> > >  drivers/staging/most/usb/usb.c | 33 +++++++++++++++++-------------
> > > ---
> > >  1 file changed, 17 insertions(+), 16 deletions(-)
> > > 
> > > diff --git a/drivers/staging/most/usb/usb.c
> > > b/drivers/staging/most/usb/usb.c
> > > index e8c5a8c..e5276524 100644
> > > --- a/drivers/staging/most/usb/usb.c
> > > +++ b/drivers/staging/most/usb/usb.c
> > > @@ -229,14 +229,14 @@ static unsigned int
> > > get_stream_frame_size(struct most_channel_config *cfg)
> > >   */
> > >  static int hdm_poison_channel(struct most_interface *iface, int
> > > channel)
> > >  {
> > > -     struct most_dev *mdev = to_mdev(iface);
> > > +     struct most_dev *mdev;
> > >       unsigned long flags;
> > >       spinlock_t *lock; /* temp. lock */
> > > 
> > >       if (unlikely(!iface)) {
> > > -             dev_warn(&mdev->usb_device->dev, "Poison: Bad
> > > interface.\n");
> > > -             return -EIO;
> > > +             return -EFAULT;
> > 
> > -EFAULT is ONLY for when you have an error with copying memory
> > to/from
> > userspace.
> 
> Ok.
> 
> > 
> > This should just be -EINVAL, right?
> > 
> > And how can iface ever be NULL?
> 
> It should never become NULL. But you never know, right?
> Too paranoid?

Yes, never check things that can not happen otherwise all functions
would be nothing but checking parameters all the time.

So just drop this check entirely.

thanks,

greg k-h
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: most: usb: fix exception handling

[Greg KH]

On Mon, May 04, 2020 at 03:17:53PM +0000, Christian.Gromm@microchip.com wrote:
> On Mon, 2020-05-04 at 15:54 +0200, Greg KH wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you
> > know the content is safe
> > 
> > On Mon, May 04, 2020 at 03:44:00PM +0200, Christian Gromm wrote:
> > > This patch fixes error handling on function parameters.
> > 
> > What does that mean?  If I don't understand it, I think it needs to
> > be
> > made a lot more explicit as to why you are making these changes :)
> > 
> > > Signed-off-by: Christian Gromm <christian.gromm@microchip.com>
> > 
> > Any "Fixes:" tag for this?
> 
> No. Just wanted to fix some obvious things, before adding
> it to stable, as discussed in our last email thread.

Remember, no one has context when reading a git commit log, please spell
it out :)

> > SHould it go to stable if it really resolves issues?
> 
> No. Once you accept this, I'll add it to stable anyway.

How?  Put the cc: stable on a patch if it fixes a real bug.  I don't see
what this "fixes" still...

thanks,

greg k-h
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: most: usb: fix exception handling

[Christian.Gromm]

On Mon, 2020-05-04 at 17:39 +0200, Greg KH wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> On Mon, May 04, 2020 at 03:17:53PM +0000, 
> Christian.Gromm@microchip.com wrote:
> > On Mon, 2020-05-04 at 15:54 +0200, Greg KH wrote:
> > > EXTERNAL EMAIL: Do not click links or open attachments unless you
> > > know the content is safe
> > > 
> > > On Mon, May 04, 2020 at 03:44:00PM +0200, Christian Gromm wrote:
> > > > This patch fixes error handling on function parameters.
> > > 
> > > What does that mean?  If I don't understand it, I think it needs
> > > to
> > > be
> > > made a lot more explicit as to why you are making these changes
> > > :)
> > > 
> > > > Signed-off-by: Christian Gromm <christian.gromm@microchip.com>
> > > 
> > > Any "Fixes:" tag for this?
> > 
> > No. Just wanted to fix some obvious things, before adding
> > it to stable, as discussed in our last email thread.
> 
> Remember, no one has context when reading a git commit log, please
> spell
> it out :)
> 
> > > SHould it go to stable if it really resolves issues?
> > 
> > No. Once you accept this, I'll add it to stable anyway.
> 
> How?  Put the cc: stable on a patch if it fixes a real bug.  I don't
> see
> what this "fixes" still...

The interface pointer provided as a function parameter has already
been used before it has been checked against NULL. :(

Once I have the unnecessary parameter checking removed, the problem 
will be removed too.

Can you please drop this patch and I'll send a new one. V2 does not
make very much sense, as the patch will be doing somethig differnt 
now.

thanks,
Chris
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel