netdev - RE: [PATCH net-next 5/5] net: sctp: fix incorrect type in gfp initializer

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]

Date:	Thu, 12 Jun 2014 08:46:44 +0000
From:	David Laight <David.Laight@...LAB.COM>
To:	'Alexei Starovoitov' <alexei.starovoitov@...il.com>,
	Daniel Borkmann <dborkman@...hat.com>
CC:	"davem@...emloft.net" <davem@...emloft.net>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"linux-sctp@...r.kernel.org" <linux-sctp@...r.kernel.org>,
	Tejun Heo <tj@...nel.org>
Subject: RE: [PATCH net-next 5/5] net: sctp: fix incorrect type in gfp
 initializer

From: Alexei Starovoitov
> On Wed, Jun 11, 2014 at 8:09 AM, Daniel Borkmann <dborkman@...hat.com> wrote:
> > On 06/11/2014 04:55 PM, David Laight wrote:
> >>
> >> From: Daniel Borkmann
...
> >>> --- a/net/sctp/associola.c
> >>> +++ b/net/sctp/associola.c
> >>> @@ -1591,7 +1591,7 @@ int sctp_assoc_lookup_laddr(struct sctp_association
> >>> *asoc,
> >>>   /* Set an association id for a given association */
> >>>   int sctp_assoc_set_id(struct sctp_association *asoc, gfp_t gfp)
> >>>   {
> >>> -       bool preload = gfp & __GFP_WAIT;
> >>> +       bool preload = !!(gfp & __GFP_WAIT);
> >>>         int ret;
> >>>
> >>>         /* If the id is already assigned, keep it. */
> >>> --
> >>
> >>
> >> I was wondering if the compiler still manages to optimise this in a
> >> manner that avoids actually calculating the boolean value...
> >>
> >> So I disassembled the compilation I just did of the old code (gcc 4.7.3).
> >> The object code looks strange.
> 
> I'm not sure where you see this.
> Just tried with gcc 4.7.2 on x64 and assembler code is exactly the
> same before/after this change.

I was slightly worried it might generate the boolean value - something
that you really don't want it to do.

I only looked at the output for the old version.
The compiler seemed to have converted:
	if (preload)
		x();
	y;
	if (preload)
		z();
into:
	if (preload) {
		x(); y; z();
	} else {
		y;
	}
and then found out that z() was empty, leaving two copies of y().

	David

> >> I think that idr_preload_end() must be an empty inline function.
> >
> >
> > Cc'ing Tejun. ;-)
> >
> >
> >> The compiler has duplicated the code between the two 'if (preload)'
> >> clauses (to avoid the conditional test), and the failed to tail
> >> merge everything in the latter stages.
> >> I suspect that an empty '#define' would generate smaller code.
> >>
> >>         David