Re: [PATCH v3 1/2] vsprintf: Add %p[mM]U for uppercase MAC address

[Peter Seiderer <ps.report@gmx.net>]

Hello Rasmus, Konrad, *,

On Thu, 27 Apr 2023 14:35:19 +0200, Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:

> On 27/04/2023 13.51, Konrad Gräfe wrote:
> > The CDC-ECM specification requires an USB gadget to send the host MAC
> > address as uppercase hex string. This change adds the appropriate
> > modifier.
> > 
> > Cc: stable@vger.kernel.org  
> 
> Why cc stable?
> 
> > Signed-off-by: Konrad Gräfe <k.graefe@gateware.de>
> > ---
> > Added in v3
> > 
> >  lib/vsprintf.c | 18 +++++++++++++++---
> >  1 file changed, 15 insertions(+), 3 deletions(-)  
> 
> The diffstat here, or for some other patch in the same series,
> definitely ought to mention lib/test_printf.c.
> 
> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > index be71a03c936a..8aee1caabd9e 100644
> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > @@ -1269,9 +1269,10 @@ char *mac_address_string(char *buf, char *end, u8 *addr,
> >  {
> >  	char mac_addr[sizeof("xx:xx:xx:xx:xx:xx")];
> >  	char *p = mac_addr;
> > -	int i;
> > +	int i, pos;
> >  	char separator;
> >  	bool reversed = false;
> > +	bool uppercase = false;
> >  
> >  	if (check_pointer(&buf, end, addr, spec))
> >  		return buf;
> > @@ -1281,6 +1282,10 @@ char *mac_address_string(char *buf, char *end, u8 *addr,
> >  		separator = '-';
> >  		break;
> >  
> > +	case 'U':
> > +		uppercase = true;
> > +		break;
> > +
> >  	case 'R':
> >  		reversed = true;
> >  		fallthrough;  
> 
> This seems broken, and I'm surprised the compiler doesn't warn about
> separator possibly being uninitialized further down. I'm also surprised
> your testing hasn't caught this. For reference, the full switch
> statement is currently

Compiler (gcc) does not warn because of Makefile:

  1038 # Enabled with W=2, disabled by default as noisy
  1039 ifdef CONFIG_CC_IS_GCC
  1040 KBUILD_CFLAGS += -Wno-maybe-uninitialized
  1041 endif

With this commented:

  lib/vsprintf.c: In function ‘mac_address_string’:
  lib/vsprintf.c:1310:30: warning: ‘separator’ may be used uninitialized [-Wmaybe-uninitialized]
   1310 |                         *p++ = separator;
        |                         ~~~~~^~~~~~~~~~~
  lib/vsprintf.c:1273:14: note: ‘separator’ was declared here
   1273 |         char separator;
        |              ^~~~~~~~~

Regards,
Peter

> 
>         switch (fmt[1]) {
>         case 'F':
>                 separator = '-';
>                 break;
> 
>         case 'R':
>                 reversed = true;
>                 fallthrough;
> 
>         default:
>                 separator = ':';
>                 break;
>         }
> 
> > @@ -1292,9 +1297,14 @@ char *mac_address_string(char *buf, char *end, u8 *addr,
> >  
> >  	for (i = 0; i < 6; i++) {
> >  		if (reversed)
> > -			p = hex_byte_pack(p, addr[5 - i]);
> > +			pos = 5 - i;
> > +		else
> > +			pos = i;
> > +
> > +		if (uppercase)
> > +			p = hex_byte_pack_upper(p, addr[pos]);
> >  		else
> > -			p = hex_byte_pack(p, addr[i]);
> > +			p = hex_byte_pack(p, addr[pos]);  
> 
> I think this becomes quite hard to follow. We have string_upper() in
> linux/string_helpers.h, so I'd rather just leave this loop alone and do
> 
>   if (uppercase)
>     string_upper(mac_addr, mac_addr);
> 
> after the nul-termination.
> 
> Rasmus
>