linux-kernel - Re: [PATCH 1/1] Fix Compilation Warning in kernel/trace/trace.c

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] [day] [month] [year] [list]

Message-ID: <1324039259.23971.127.camel@gandalf.stny.rr.com>
Date:	Fri, 16 Dec 2011 07:40:59 -0500
From:	Steven Rostedt <rostedt@...dmis.org>
To:	Devendra Naga <devendra.aaru@...il.com>
Cc:	fweisbec@...il.com, mingo@...hat.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/1] Fix Compilation Warning in kernel/trace/trace.c

On Fri, 2011-12-16 at 02:15 -0500, Devendra Naga wrote:
> there was a compilation warning saying
> kernel/trace/trace.c:3644:8: warning: ‘page2’ may be used uninitialized in this function

Nacked-by: Steven Rostedt <rostedt@...dmis.org>

And I also nacked this same fix here:

https://lkml.org/lkml/2011/11/28/287


> 
> The page2 pointer is used only when nr_pages = 2.
> 
> Assigned a null value to the page2 to get rid of warning.


Please please please, do not just fix warnings without actually looking
at the code.

1) this is a bug in the compiler, which happens to be fixed in newer
compilers (which does not produce this warning). It just happens that
gcc couldn't tell the simple usage of:

	if (nr_pages == 2)
		page2 = kmap_atomic(pages[1]);

and later:

	if (nr_pages == 2) {
		len = PAGE_SIZE - offset;
		memcpy(&entry->buf, page1 + offset, len);
		memcpy(&entry->buf[len], page2, cnt - len);

and finally:

	if (nr_pages == 2)
		kunmap_atomic(page2);

nr_pages never changes during this code. Gcc probably got confused
because on error exit, it does change (to unmap everything).

2) But your fix is still wrong!

Initializing it to NULL is wrong! Because it would hide a kernel bug if
page2 was really used uninitialized. As passing NULL as the source of
memcpy will panic the kernel.

Again, do not just randomly post fixes to gcc warnings without
understanding the real fix.

Thanks,

-- Steve



> 
> Signed-off-by: Devendra Naga <devendra.aaru@...il.com>
> ---
>  kernel/trace/trace.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index f2bd275..80dfd92 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -3641,7 +3641,7 @@ tracing_mark_write(struct file *filp, const char __user *ubuf,
>  	int nr_pages = 1;
>  	ssize_t written;
>  	void *page1;
> -	void *page2;
> +	void *page2 = NULL;
>  	int offset;
>  	int size;
>  	int len;


--
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/