linux-kernel - Re: byteorder: cpu_to_le32_array vs cpu_to_be32

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <AM0PR0502MB366879157E871B1E757E491ABA6E0@AM0PR0502MB3668.eurprd05.prod.outlook.com>
Date:   Sun, 20 Oct 2019 20:23:33 +0000
From:   Anatol Belski <weltling@...look.de>
To:     Joe Perches <joe@...ches.com>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        Mika Westerberg <mika.westerberg@...ux.intel.com>
CC:     linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: byteorder: cpu_to_le32_array vs cpu_to_be32_array function API
 differences

On Sun, 2019-10-20 at 12:40 -0700, Joe Perches wrote:
> On Sun, 2019-10-20 at 19:28 +0000, Anatol Belski wrote:
> > Hi,
> 
> Hello.
> 
> > On Sun, 2019-10-20 at 12:02 -0700, Joe Perches wrote:
> > > There's an argument inconsistency between these 4 functions
> > > in include/linux/byteorder/generic.h
> > > 
> > > It'd be more a consistent API with one form and not two.
> > > 
> > >    static inline void le32_to_cpu_array(u32 *buf, unsigned int
> > > words)
> > >    {
> > >    	while (words--) {
> > >    		__le32_to_cpus(buf);
> > >    		buf++;
> > >    	}
> > >    }
> > > 
> > >    static inline void cpu_to_le32_array(u32 *buf, unsigned int
> > > words)
> > >    {
> > >    	while (words--) {
> > >    		__cpu_to_le32s(buf);
> > >    		buf++;
> > >    	}
> > >    }
> > > 
> > > vs
> > > 
> > >    static inline void cpu_to_be32_array(__be32 *dst, const u32
> > > *src,
> > > size_t len)
> > >    {
> > >    	int i;
> > > 
> > >    	for (i = 0; i < len; i++)
> > >    		dst[i] = cpu_to_be32(src[i]);
> > >    }
> > > 
> > >    static inline void be32_to_cpu_array(u32 *dst, const __be32
> > > *src,
> > > size_t len)
> > >    {
> > >    	int i;
> > > 
> > >    	for (i = 0; i < len; i++)
> > >    		dst[i] = be32_to_cpu(src[i]);
> > >    }
> > > 
> > > 
> > 
> > size_t is the right choice for this, as it'll generate more correct
> > binary depending on 32/64 bit. I've sent a patch in
> > 'include/linux/byteorder/generic.h: fix signed/unsigned warnings'
> > before, but only touched the place where i've seen warnings. My
> > very
> > bet is, that changing between size_t/unsigned, while it would be
> > consistent, wouldn't change the functionality. It'd probably make
> > sense
> > to extend the aforementioned patch to move unsigned -> size_t.
> 
> True, but my point was the le versions have 2 arguments and
> convert the buf input, and the be versions have 3 arguments
> and convert src to dst.

Thanks for checking. Yes, that's another point of the inconsistency. I
could imagine fixing it by adapting all the signatures to be


static inline void _cpu_to_le32_array(__le32 *dst, const u32 *src,
size_t len)
static inline void _le32_to_cpu_array(u32 *dst, const __le32 *src,
size_t len)
static inline void cpu_to_be32_array(__be32 *dst, const u32 *src,
size_t len)
static inline void be32_to_cpu_array(u32 *dst, const __be32 *src,
size_t len)

and do the necessary implementation change in the le variants, plus
introduce some macros to ensure the backward compatibility.

#define le32_to_cpu_array(dst, len) _cpu_to_le32_array(dst, dst, len)
#define cpu_to_le32_array(dst, len) _le32_to_cpu_array(dst, dst, len)

But it is a bad situation for the backward compatibility in any case,
as the new API would have to be named somehow. That would be bad for
the backport. If breaching this is ok for master, then the fix would be
as easy as changing the signatures and adapting the implementation.


Thanks

Anatol