linux-kernel - Re: [PATCH] TRACING: Fix a copmile warning

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: <4E2EC1D6.4020102@grupopie.com>
Date:	Tue, 26 Jul 2011 14:32:06 +0100
From:	Paulo Marques <pmarques@...popie.com>
To:	Jesper Juhl <jj@...osbits.net>
CC:	Steven Rostedt <rostedt@...dmis.org>, stufever@...il.com,
	linux-kernel@...r.kernel.org,
	Wang Shaoyan <wangshaoyan.pt@...bao.com>,
	Frederic Weisbecker <fweisbec@...il.com>,
	Ingo Molnar <mingo@...hat.com>
Subject: Re: [PATCH] TRACING: Fix a copmile warning

Jesper Juhl wrote:
> On Tue, 26 Jul 2011, Paulo Marques wrote:
> 
>> Steven Rostedt wrote:
>>> On Mon, 2011-07-18 at 17:40 +0800, stufever@...il.com wrote:
>>>> From: Wang Shaoyan <wangshaoyan.pt@...bao.com>
>>>>
>>>> It's harmless but annyoing.
>>>>   kernel/trace/trace_printk.c: In function 'module_trace_bprintk_format_notify':
>>>>   kernel/trace/trace_printk.c:52: warning: 'fmt' may be used uninitialized in this function
>>> I prefer not to add this patch. Fix gcc. Actually some gcc's do not warn
>>> on this, others do. Here's the code that confuses gcc:
>>>
>>> 		tb_fmt = kmalloc(sizeof(*tb_fmt), GFP_KERNEL);
>>> 		if (tb_fmt)
>>> 			fmt = kmalloc(strlen(*iter) + 1, GFP_KERNEL);
>>> 		if (tb_fmt && fmt) {
>>> 			list_add_tail(&tb_fmt->list, &trace_bprintk_fmt_list);
>>> 			strcpy(fmt, *iter);
>>> 			tb_fmt->fmt = fmt;
>>> 			*iter = tb_fmt->fmt;
>>>
>>>
>>> fmt will never be looked at if tb_fmt is NULL, and fmt is initialized if
>>> tb_fmt is not NULL.
>> Yes, changing code just to please gcc is not nice. In this case,
>> changing the code to the more straightforward / naive implementation
>> might make it more readable (IMHO) and maybe even improve code
>> generation. I.e., something like this:
>>
>> tb_fmt = kmalloc(sizeof(*tb_fmt), GFP_KERNEL);
>> if (tb_fmt) {
>>         fmt = kmalloc(strlen(*iter) + 1, GFP_KERNEL);
>> 	if (fmt) {
>>         	list_add_tail(&tb_fmt->list, &trace_bprintk_fmt_list);
>>         	strcpy(fmt, *iter);
>>         	tb_fmt->fmt = fmt;
>>         	*iter = tb_fmt->fmt;
>>         } else {
>> 	        kfree(tb_fmt);
>>         	*iter = NULL;
>>         }
>> } else {
>>         *iter = NULL;
>> }
>>
>> The downside is that the "*iter = NULL" gets repeated twice...
>>
> 
> You could avoid that like this:
> 
> *iter = NULL;
> tb_fmt = kmalloc(sizeof(*tb_fmt), GFP_KERNEL);
> if (tb_fmt) {
>       fmt = kmalloc(strlen(*iter) + 1, GFP_KERNEL);
>       if (fmt) {
>               list_add_tail(&tb_fmt->list, &trace_bprintk_fmt_list);
>               strcpy(fmt, *iter);
>               tb_fmt->fmt = fmt;
>               *iter = tb_fmt->fmt;
>         } else {
>               kfree(tb_fmt);
>         }
> }

Yes, but this way you always set *iter to NULL, whereas in the previous
version that was the very unlikely case (kmalloc returning NULL).

Probably gcc is smart enough to generate the same code for both
versions, to avoid setting *iter twice for the likely case (and even if
it doesn't, then the cache will be hot and probably not written back
yet, yadda, yadda)...

-- 
Paulo Marques - www.grupopie.com

"I used to be indecisive, but now I'm not so sure."
--
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/