linux-kernel - Re: ftrace breaks sparc64 build

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:	Mon, 5 Jan 2009 15:05:11 -0500 (EST)
From:	Steven Rostedt <rostedt@...dmis.org>
To:	Sam Ravnborg <sam@...nborg.org>
cc:	LKML <linux-kernel@...r.kernel.org>,
	Steven Rostedt <srostedt@...hat.com>,
	Ingo Molnar <mingo@...e.hu>,
	"David S. Miller" <davem@...emloft.net>,
	sparclinux <sparclinux@...r.kernel.org>
Subject: Re: ftrace breaks sparc64 build

On Mon, 5 Jan 2009, Sam Ravnborg wrote:

> Hi Steven.
> 
> > 
> > Honestly, that code is a little obfuscated, and would be better to write 
> > it as:
> > 
> > 	if (vp->major == 0 && vp->minor=0)
> > 		return ldc_abort(lp);
> > 
> > 	vap = find_by_major(vp->major);
> > 	if (!vap)
> > 		return ldc_abort(lp);
> > 
> > 	[...]
> > 
> > This is much easier to read and we can remove the else statement 
> > altogether. And I bet the warning will go away if we did it this way.
> 
> Fully ageed on the readability.
> I happen to trigger this as an error in the sparc code.
> But I see the same warning also in generic code.
> 
> >From kernel/module.c:
>         /* Suck in entire file: we'll want most of it. */
>         /* vmalloc barfs on "unusual" numbers.  Check here */
>         if (len > 64 * 1024 * 1024 || (hdr = vmalloc(len)) == NULL)
>                 return ERR_PTR(-ENOMEM);
> 
> 
> This gives following warning:
> kernel/module.c: In function `load_module':
> kernel/module.c:1842: warning: 'hdr' might be used uninitialized in this function

Probably the same issue. The problem is that the first use of a variable 
is in the OR section of an if statement that does a return.

	if (x || !(y = init_me())
		return;

	use_me(y);

IMHO I find this sloppy code. When reading the code it can cause reviewers 
trouble, and wasted time, to see that y is indeed initialized. I'm 
impressed that gcc was able to figure it out.

> 
> So this is not a pattern we seen only in sparc code and I wonder if this is
> the first time it is brought up?
> 
> I can fix up the cases in sparc - no problem.
> But it was a suprise to me _why_ these warnings started to creep
> up and then it break my build.

Have you always been compiling with -Werror?

The reason that gcc complains is because you have the "branch_tracer" on 
that converts 'if ()' into a macro (as you saw in your -E compile). This 
makes the if statement more complex, and goes beyond gcc's ability to know 
that the above 'y' is initialized properly. I would work on fixing this in 
the branch tracer, but honestly, I'm kind of glad that gcc barfs on it. 
This will help us point out this kind of sloppy initializations (sorry if 
I'm offending anybody about calling it sloppy). I just believe that it 
makes the code a bit more obfuscated to initialize in an if statement, and 
a second part of a complex if statement at that!

-- Steve

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/