linux-kernel - Re: [PATCH] direct-io: squelch maybe-uninitialized warning in do_direct

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: <1404245087.2010.15.camel@x220>
Date:	Tue, 01 Jul 2014 22:04:47 +0200
From:	Paul Bolle <pebolle@...cali.nl>
To:	Markus Mayer <markus.mayer@...aro.org>
Cc:	Alexander Viro <viro@...iv.linux.org.uk>,
	Jason Cooper <jason@...edaemon.net>,
	Linux Filesystem Mailing List <linux-fsdevel@...r.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] direct-io: squelch maybe-uninitialized warning in
 do_direct_IO()

On Mon, 2014-06-23 at 12:54 -0700, Markus Mayer wrote:
> I did a bit of digging on this and I am wondering if the initialization
> of "to" and "from" to 0 should instead be done in dio_get_page().
> 
> The warning is caused by the return in dio_get_page():
> 
> 		ret = dio_refill_pages(dio, sdio);
> 		if (ret)
> 			return ERR_PTR(ret);
> 
> This code path would leave "to" and "from" uninitialized (even though the
> caller currently never uses the variables in that case).
> 
> If both variables are initialized to 0 in dio_get_page(), it would
> guarantee that any future callers of dio_get_page() also get to take
> advantage of the initialization. If, however, the variables are only
> initialized in do_direct_IO(), other callers would have to take care of
> this themselves (or the same warnings will pop up there).
> 
> There aren't currently any other callers to dio_get_page(), so the
> choice is probably not a terribly critical one right now, but I wanted
> to at least point this out.

I peeked at this a bit too.

Maybe it's better to move initializing "to" and "from" out of
dio_get_page(). That _might_ make it easier for both the the reader and
the compiler to understand what's going on. Something like this:

diff --git a/fs/direct-io.c b/fs/direct-io.c
index 98040ba388ac..2f024fc16a07 100644
--- a/fs/direct-io.c
+++ b/fs/direct-io.c
@@ -198,9 +198,8 @@ static inline int dio_refill_pages(struct dio *dio, struct dio_submit *sdio)
  * L1 cache.
  */
 static inline struct page *dio_get_page(struct dio *dio,
-		struct dio_submit *sdio, size_t *from, size_t *to)
+		struct dio_submit *sdio)
 {
-	int n;
 	if (dio_pages_present(sdio) == 0) {
 		int ret;
 
@@ -209,10 +208,7 @@ static inline struct page *dio_get_page(struct dio *dio,
 			return ERR_PTR(ret);
 		BUG_ON(dio_pages_present(sdio) == 0);
 	}
-	n = sdio->head++;
-	*from = n ? 0 : sdio->from;
-	*to = (n == sdio->tail - 1) ? sdio->to : PAGE_SIZE;
-	return dio->pages[n];
+	return dio->pages[sdio->head];
 }
 
 /**
@@ -911,11 +907,15 @@ static int do_direct_IO(struct dio *dio, struct dio_submit *sdio,
 	while (sdio->block_in_file < sdio->final_block_in_request) {
 		struct page *page;
 		size_t from, to;
-		page = dio_get_page(dio, sdio, &from, &to);
+
+		page = dio_get_page(dio, sdio);
 		if (IS_ERR(page)) {
 			ret = PTR_ERR(page);
 			goto out;
 		}
+		from = sdio->head ? 0 : sdio->from;
+		to = (sdio->head == sdio->tail - 1) ? sdio->to : PAGE_SIZE;
+		sdio->head++;
 
 		while (from < to) {
 			unsigned this_chunk_bytes;	/* # of bytes mapped */

This at least silences GCC.

But do_direct_IO() and friends are complicated enough that I'll have to
look at them for another day or two (or many, many more) before I'm
confident that this doesn't actually change anything.


Paul Bolle

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