[PATCH v0] USB: storage: karma: fix rio_karma_init return

[PATCH v0] USB: storage: karma: fix rio_karma_init return

[Lin Ma]

The function rio_karam_init() should return USB_STOR_TRANSPORT_ERROR
instead of 0 (USB_STOR_TRANSPORT_GOOD) when allocation fails.

Fixes: dfe0d3ba20e8 ("USB Storage: add rio karma eject support")

Signed-off-by: Lin Ma <linma@zju.edu.cn>
---
 drivers/usb/storage/karma.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/storage/karma.c b/drivers/usb/storage/karma.c
index 05cec81dcd3f..b8a4ae1aa22a 100644
--- a/drivers/usb/storage/karma.c
+++ b/drivers/usb/storage/karma.c
@@ -178,19 +178,19 @@ static int rio_karma_init(struct us_data *us)
 	struct karma_data *data = kzalloc(sizeof(struct karma_data), GFP_NOIO);
 
 	if (!data)
-		goto out;
+		return USB_STOR_TRANSPORT_ERROR;
 
 	data->recv = kmalloc(RIO_RECV_LEN, GFP_NOIO);
 	if (!data->recv) {
 		kfree(data);
-		goto out;
+		return USB_STOR_TRANSPORT_ERROR;
 	}
 
 	us->extra = data;
 	us->extra_destructor = rio_karma_destructor;
 	ret = rio_karma_send_command(RIO_ENTER_STORAGE, us);
 	data->in_storage = (ret == 0);
-out:
+
 	return ret;
 }
 
-- 
2.35.1

Re: [PATCH v0] USB: storage: karma: fix rio_karma_init return

[Alan Stern]

On Wed, Apr 06, 2022 at 06:02:59PM +0800, Lin Ma wrote:
> The function rio_karam_init() should return USB_STOR_TRANSPORT_ERROR
> instead of 0 (USB_STOR_TRANSPORT_GOOD) when allocation fails.

Not exactly.  rio_karma_init() is a usb-storage initFunction (see 
the usb_stor_acquire_resources() routine in usb.c), and these functions 
are supposed to return either 0 or a negative error code.

So you should make the routine return -ENOMEM, not 
USB_STOR_TRANSPORT_ERROR.  You can simplify the patch by changing the 
line where ret is defined; initialize it to -ENOMEM rather than to 0.

And don't forget the error code for when the rio_karma_send_command() 
call fails.  In that case the return value should be -EIO.

> Fixes: dfe0d3ba20e8 ("USB Storage: add rio karma eject support")

Shouldn't this also be marked for the stable kernels?

Alan Stern

> Signed-off-by: Lin Ma <linma@zju.edu.cn>
> ---
>  drivers/usb/storage/karma.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/storage/karma.c b/drivers/usb/storage/karma.c
> index 05cec81dcd3f..b8a4ae1aa22a 100644
> --- a/drivers/usb/storage/karma.c
> +++ b/drivers/usb/storage/karma.c
> @@ -178,19 +178,19 @@ static int rio_karma_init(struct us_data *us)
>  	struct karma_data *data = kzalloc(sizeof(struct karma_data), GFP_NOIO);
>  
>  	if (!data)
> -		goto out;
> +		return USB_STOR_TRANSPORT_ERROR;
>  
>  	data->recv = kmalloc(RIO_RECV_LEN, GFP_NOIO);
>  	if (!data->recv) {
>  		kfree(data);
> -		goto out;
> +		return USB_STOR_TRANSPORT_ERROR;
>  	}
>  
>  	us->extra = data;
>  	us->extra_destructor = rio_karma_destructor;
>  	ret = rio_karma_send_command(RIO_ENTER_STORAGE, us);
>  	data->in_storage = (ret == 0);
> -out:
> +
>  	return ret;
>  }
>  
> -- 
> 2.35.1
> 

Re: [PATCH v0] USB: storage: karma: fix rio_karma_init return

[Lin Ma]

Hi Alan,

> Not exactly.  rio_karma_init() is a usb-storage initFunction (see 
> the usb_stor_acquire_resources() routine in usb.c), and these functions 
> are supposed to return either 0 or a negative error code.
> 
> So you should make the routine return -ENOMEM, not 
> USB_STOR_TRANSPORT_ERROR.  You can simplify the patch by changing the 
> line where ret is defined; initialize it to -ENOMEM rather than to 0

Thanks for pointing out that, and as I dig deeper, I found that the use of error code
is "totally a mess" in the usb storage.

Taking the initfunction for example, there are below 6 cases
1 karam: rio_karma_init()
2 realtek_cr: init_realtek_cr()
3 aluda: init_alauda()
4 isd200: isd200_init_info()
5 shuttle_usbat: init_usbat()
6 onetouch: onetouch_connect_input()

The (1) is erroneously return 0 even when the kzalloc if failed, must be fixed.
The (2) and (6) uses -ENOMEM when allocation fails. (2) will also return -EIO when
another kzalloc fails or rts51x_check_status() fails.
The (3) uses USB_STOR_TRANSPORT_ERROR when allocation fails (seems that I learned a
mistake from here).
The (4) uses custom ISD200_ERROR when allocation fails.
The (5) uses constant 1 when allocation fails.

It's worth pointed out that (except (1)), the others though not following the standard,
it won't cause bad thing because the callsite of these initFunction just check whether the
return is zero.

/*
 * Just before we start our control thread, initialize
 * the device if it needs initialization
 */
 if (us->unusual_dev->initFunction) {
     p = us->unusual_dev->initFunction(us);
     if (p)
         return p;
 }

I will then send patches to make sure all initFunction follow the standard.

> 
> And don't forget the error code for when the rio_karma_send_command() 
> call fails.  In that case the return value should be -EIO.
> 

Okay, will add this in next version of this patch.

> 
> Shouldn't this also be marked for the stable kernels?
> 

Sorry, I didn't get it, do you mean add another tag like "Cc: stable@vger.kernel.org"?
Or I just need to CC the mail to stable mail list?

Regards
Lin

Re: [PATCH v0] USB: storage: karma: fix rio_karma_init return

[Alan Stern]

On Thu, Apr 07, 2022 at 09:11:02AM +0800, Lin Ma wrote:
> Hi Alan,
> 
> > Not exactly.  rio_karma_init() is a usb-storage initFunction (see 
> > the usb_stor_acquire_resources() routine in usb.c), and these functions 
> > are supposed to return either 0 or a negative error code.
> > 
> > So you should make the routine return -ENOMEM, not 
> > USB_STOR_TRANSPORT_ERROR.  You can simplify the patch by changing the 
> > line where ret is defined; initialize it to -ENOMEM rather than to 0
> 
> Thanks for pointing out that, and as I dig deeper, I found that the use of error code
> is "totally a mess" in the usb storage.
> 
> Taking the initfunction for example, there are below 6 cases
> 1 karam: rio_karma_init()
> 2 realtek_cr: init_realtek_cr()
> 3 aluda: init_alauda()
> 4 isd200: isd200_init_info()
> 5 shuttle_usbat: init_usbat()
> 6 onetouch: onetouch_connect_input()
> 
> The (1) is erroneously return 0 even when the kzalloc if failed, must be fixed.
> The (2) and (6) uses -ENOMEM when allocation fails. (2) will also return -EIO when
> another kzalloc fails or rts51x_check_status() fails.
> The (3) uses USB_STOR_TRANSPORT_ERROR when allocation fails (seems that I learned a
> mistake from here).
> The (4) uses custom ISD200_ERROR when allocation fails.
> The (5) uses constant 1 when allocation fails.
> 
> It's worth pointed out that (except (1)), the others though not following the standard,
> it won't cause bad thing because the callsite of these initFunction just check whether the
> return is zero.

That isn't true.  Look again at the code:

> /*
>  * Just before we start our control thread, initialize
>  * the device if it needs initialization
>  */
>  if (us->unusual_dev->initFunction) {
>      p = us->unusual_dev->initFunction(us);
>      if (p)
>          return p;

If p isn't zero then this function uses p as its return value.  Thus it 
does more with p than just check whether it is zero.

>  }
> 
> I will then send patches to make sure all initFunction follow the standard.
> 
> > 
> > And don't forget the error code for when the rio_karma_send_command() 
> > call fails.  In that case the return value should be -EIO.
> > 
> 
> Okay, will add this in next version of this patch.
> 
> > 
> > Shouldn't this also be marked for the stable kernels?
> > 
> 
> Sorry, I didn't get it, do you mean add another tag like "Cc: stable@vger.kernel.org"?
> Or I just need to CC the mail to stable mail list?

I mean add another tag.  See the description of Option 1 in 
Documentation/process/stable-kernel-rules.rst.

Alan Stern

> 
> Regards
> Lin

Re: [PATCH v0] USB: storage: karma: fix rio_karma_init return

[Lin Ma]

Hello Alan,

Sorry for the delay. I've sent another version of this patch.

> 
> That isn't true.  Look again at the code:
> 
> > /*
> >  * Just before we start our control thread, initialize
> >  * the device if it needs initialization
> >  */
> >  if (us->unusual_dev->initFunction) {
> >      p = us->unusual_dev->initFunction(us);
> >      if (p)
> >          return p;
> 
> If p isn't zero then this function uses p as its return value.  Thus it 
> does more with p than just check whether it is zero.
> 

Yeah, you are right. Well what I mean about the "bad thing" is that something wrong but the usb core continue to install this driver, just like what the karam driver is doing.

> >  }
> > 
> > I will then send patches to make sure all initFunction follow the standard.
> > 
> > > 
> > > And don't forget the error code for when the rio_karma_send_command() 
> > > call fails.  In that case the return value should be -EIO.
> > > 
> > 
> > Okay, will add this in next version of this patch.
> > 
> > > 
> > > Shouldn't this also be marked for the stable kernels?
> > > 
> > 
> > Sorry, I didn't get it, do you mean add another tag like "Cc: stable@vger.kernel.org"?
> > Or I just need to CC the mail to stable mail list?
> 
> I mean add another tag.  See the description of Option 1 in 
> Documentation/process/stable-kernel-rules.rst.
> 

Cool, have added this one.
Link: https://marc.info/?l=linux-usb&m=164965695228314&w=2

Thanks
Lin Ma

> Alan Stern
> 
> > 
> > Regards
> > Lin