[PATCH -next] USB: core: Switch to use kmemdup_nul() helper

[PATCH -next] USB: core: Switch to use kmemdup_nul() helper

[Ruan Jinjie]

Use kmemdup_nul() helper instead of open-coding it to simplify the code.

Signed-off-by: Ruan Jinjie <ruanjinjie@huawei.com>
---
 drivers/usb/core/message.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
index 0d2bfc909019..5762fd04f0d5 100644
--- a/drivers/usb/core/message.c
+++ b/drivers/usb/core/message.c
@@ -1029,10 +1029,9 @@ char *usb_cache_string(struct usb_device *udev, int index)
 	if (buf) {
 		len = usb_string(udev, index, buf, MAX_USB_STRING_SIZE);
 		if (len > 0) {
-			smallbuf = kmalloc(++len, GFP_NOIO);
+			smallbuf = kmemdup_nul(buf, len, GFP_NOIO);
 			if (!smallbuf)
 				return buf;
-			memcpy(smallbuf, buf, len);
 		}
 		kfree(buf);
 	}
-- 
2.34.1

Re: [PATCH -next] USB: core: Switch to use kmemdup_nul() helper

[Greg KH]

On Mon, Aug 07, 2023 at 08:46:10PM +0800, Ruan Jinjie wrote:
> Use kmemdup_nul() helper instead of open-coding it to simplify the code.
> 
> Signed-off-by: Ruan Jinjie <ruanjinjie@huawei.com>
> ---
>  drivers/usb/core/message.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
> index 0d2bfc909019..5762fd04f0d5 100644
> --- a/drivers/usb/core/message.c
> +++ b/drivers/usb/core/message.c
> @@ -1029,10 +1029,9 @@ char *usb_cache_string(struct usb_device *udev, int index)
>  	if (buf) {
>  		len = usb_string(udev, index, buf, MAX_USB_STRING_SIZE);
>  		if (len > 0) {
> -			smallbuf = kmalloc(++len, GFP_NOIO);
> +			smallbuf = kmemdup_nul(buf, len, GFP_NOIO);
>  			if (!smallbuf)
>  				return buf;
> -			memcpy(smallbuf, buf, len);

But you changed the logic here, you now added an extra \0 where the
existing code did not.  Are you sure you mean to do this?  If so, why,
and it needs to be documented in the changelog text.

What this could be is a call to kmemdup() if you really want it, but be
careful about the ++len usage...

Also, does this need to be changed at all?  How was it tested?

thanks,

greg k-h

Re: [PATCH -next] USB: core: Switch to use kmemdup_nul() helper

[Ruan Jinjie]

On 2023/8/8 16:22, Greg KH wrote:
> On Mon, Aug 07, 2023 at 08:46:10PM +0800, Ruan Jinjie wrote:
>> Use kmemdup_nul() helper instead of open-coding it to simplify the code.
>>
>> Signed-off-by: Ruan Jinjie <ruanjinjie@huawei.com>
>> ---
>>  drivers/usb/core/message.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
>> index 0d2bfc909019..5762fd04f0d5 100644
>> --- a/drivers/usb/core/message.c
>> +++ b/drivers/usb/core/message.c
>> @@ -1029,10 +1029,9 @@ char *usb_cache_string(struct usb_device *udev, int index)
>>  	if (buf) {
>>  		len = usb_string(udev, index, buf, MAX_USB_STRING_SIZE);
>>  		if (len > 0) {
>> -			smallbuf = kmalloc(++len, GFP_NOIO);
>> +			smallbuf = kmemdup_nul(buf, len, GFP_NOIO);
>>  			if (!smallbuf)
>>  				return buf;
>> -			memcpy(smallbuf, buf, len);
> 
> But you changed the logic here, you now added an extra \0 where the
> existing code did not.  Are you sure you mean to do this?  If so, why,
> and it needs to be documented in the changelog text.

Right! There is a problem because of the ++len, and the logic has been
changed. Sorry, I'll carefully check the patches issued in the future.

> 
> What this could be is a call to kmemdup() if you really want it, but be
> careful about the ++len usage...
> 
> Also, does this need to be changed at all?  How was it tested?

It's best to keep it as it is. Sorry, just walk through the code.

> 
> thanks,
> 
> greg k-h