Re: [PATCH] USB: core: Use `krealloc()` in `usb_cache_string()`

[Bence Csókás]

Hi Greg,

On 3/12/26 06:02, Greg Kroah-Hartman wrote:
> On Thu, Mar 12, 2026 at 12:06:35AM +0100, Bence Csókás via B4 Relay wrote:
> > From: Bence Csókás <bence98@xxxxxxxxxx>
> >
> > Instead of "shrinking" the allocation by `kmalloc()`ing a new, smaller
> > buffer, utilize `krealloc()` to shrink the existing allocation. This saves
> > a `memcpy()`, as well as guards against `smallbuf` allocation failure.
> >
> > Signed-off-by: Bence Csókás <bence98@xxxxxxxxxx>
> > ---
> > Using `krealloc()` makes this code from 2005 more readable as well as
> > robust. Nested `if`s were also unrolled.
>
> How is it more "robust" now?

My understanding was (at least from reading mm/slub.c, and also by 
analogue to libc `realloc()`), that krealloc-ing an allocation to be 
smaller (without changing alignment or NUMA requirements) just shrinks 
it in-place, instead of allocating a new, smaller buffer (which is what 
the code was doing before, essentially "by hand"). Under memory 
pressure, this smaller allocation might fail, even though by the end, 
more memory will have been freed than what was initially allocated.

> > ---
> >   drivers/usb/core/message.c | 20 ++++++++++----------
> >   1 file changed, 10 insertions(+), 10 deletions(-)
>
> Same number of lines. Well, not quite, because I'm going to ask you to
> remove the ?: stuff below...
>
> > diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
> > index ea970ddf8879..dfe61d8b913b 100644
> > --- a/drivers/usb/core/message.c
> > +++ b/drivers/usb/core/message.c
> > @@ -1005,7 +1005,7 @@ int usb_string(struct usb_device *dev, int index, char *buf, size_t size)
> >   }
> >   EXPORT_SYMBOL_GPL(usb_string);
> >   
> > -/* one UTF-8-encoded 16-bit character has at most three bytes */
> > +/* one 16-bit character, when UTF-8-encoded, has at most three bytes */
>
> Why change this?

Right. While I was mentally parsing `usb_cache_string()` I came across 
this comment and found it very confusingly written. How can "one [...] 
16-bit character" be anything else than two bytes (assuming 8-bit bytes; 
let's ignore historical architectures like the PDP-10)? The answer is 
that the UTF-8 *encoding* has <= 3 bytes, not the 16-bit UCS-2 character 
it encodes.

> >   #define MAX_USB_STRING_SIZE (127 * 3 + 1)
> >   
> >   /**
> > @@ -1026,17 +1026,17 @@ char *usb_cache_string(struct usb_device *udev, int index)
> >   		return NULL;
> >   
> >   	buf = kmalloc(MAX_USB_STRING_SIZE, GFP_NOIO);
> > -	if (buf) {
> > -		len = usb_string(udev, index, buf, MAX_USB_STRING_SIZE);
> > -		if (len > 0) {
> > -			smallbuf = kmalloc(++len, GFP_NOIO);
> > -			if (!smallbuf)
> > -				return buf;
> > -			memcpy(smallbuf, buf, len);
> > -		}
> > +	if (!buf)
> > +		return NULL;
> > +
> > +	len = usb_string(udev, index, buf, MAX_USB_STRING_SIZE);
> > +	if (len <= 0) {
> >   		kfree(buf);
> > +		return NULL;
> >   	}
> > -	return smallbuf;
> > +
> > +	smallbuf = krealloc(buf, len + 1, GFP_NOIO);
> > +	return smallbuf ? : buf;
>
> I hate ? : except where it can only be used (i.e. in function
> arguments), so please spell it out exactly what you are doing here.

Sure.

> Also, how was this tested?

I just compiled and booted it on my Arch box (with the original vendor 
config), an AthlonII X2 PC. I'm now typing this mail on a USB keyboard 
and mouse under Plasma, running this kernel :) I also plugged in a 
pendrive, mounted it, `ls`'d the mount, unmounted, unplugged, and did 
this 2 more times.

I realize I should probably put this info under the dashes. I'll prepare 
a v2.

> thanks,
>
> greg k-h

Bence