linux-kernel - Re: [PATCH v2] byteswap: try to avoid __builtin_constant

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: <3864016.Cn7XXDBM38@wuerfel>
Date:	Wed, 22 Jun 2016 11:59:30 +0200
From:	Arnd Bergmann <arnd@...db.de>
To:	Tomas Winkler <tomasw@...il.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	mm-commits@...r.kernel.org,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	linux-scsi@...r.kernel.org, jpoimboe@...hat.com,
	Martin Jambor <mjambor@...e.cz>,
	"Martin K. Petersen" <martin.petersen@...cle.com>,
	James Bottomley <James.Bottomley@...senpartnership.com>,
	Denys Vlasenko <dvlasenk@...hat.com>,
	Thomas Graf <tgraf@...g.ch>,
	Peter Zijlstra <peterz@...radead.org>,
	David Rientjes <rientjes@...gle.com>,
	Ingo Molnar <mingo@...nel.org>,
	Himanshu Madhani <himanshu.madhani@...gic.com>,
	Dept-Eng QLA2xxx Upstream <qla2xxx-upstream@...gic.com>,
	Jan Hubicka <hubicka@....cz>,
	"Amir (Jer)" <amir.jer.levy@...el.com>
Subject: Re: [PATCH v2] byteswap: try to avoid __builtin_constant_p gcc bug

On Wednesday, June 22, 2016 11:24:50 AM CEST Tomas Winkler wrote:
> On Tue, Jun 21, 2016 at 12:02 PM, Tomas Winkler <tomasw@...il.com> wrote:
> > On Tue, May 3, 2016 at 9:36 AM, Arnd Bergmann <arnd@...db.de> wrote:
> >> On Monday 02 May 2016 16:32:25 Andrew Morton wrote:

> >>  #ifdef __HAVE_BUILTIN_BSWAP32__
> >> -#define __swab32(x) __builtin_bswap32((__u32)(x))
> >> +#define __swab32(x) (__u32)__builtin_bswap32((__u32)(x))
> >>  #else
> >>  #define __swab32(x)                            \
> >>         (__builtin_constant_p((__u32)(x)) ?     \
> >
> >>
> >
> > I wonder if this doesn't break switch statement that requires a
> > constant expression, there few cases like this over the kernel.
> >
> > switch(val) {
> > case cpu_to_le32(IXGBE_RXDADV_STAT_FCSTAT_FCPRSP):
> >
> > http://lxr.free-electrons.com/source/drivers/net/ethernet/intel/ixgbe/ixgbe_fcoe.c#L458
> >
> 
> I'm asking because sparse and checkpatch doesn't agree on that ping
> sparse issues
> 'error: bad constant expression'
> When changing to __constant_cpu_to_le32 sparse is happy but
> checkpatch.ps is complaining
> __constant_cpu_to_le32 should be cpu_to_le32
> 

That is an interesting problem, as both seem to be reasonable:
sparse probably doesn't understand __builtin_constant_p() enough,
while avoiding __constant_cpu_to_le32() is a good recommendation
in general.

How many instances of this do you see in the kernel? If ixgbe is the
only one, I'd just move the byteswap up into the switch statement:

	switch (le32_to_cpu(val)) {
	case IXGBE_RXDADV_STAT_FCSTAT_FCPRSP:

which may cost one or two cycles for the non-constant byteswap,
but is also easier to read than the current code.

	Arnd