linux-kernel - Re: [PATCH] Fix bad data from non-direct-io read after direct-io write

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: <47226A75.1020008@oracle.com>
Date:	Fri, 26 Oct 2007 15:30:13 -0700
From:	Zach Brown <zach.brown@...cle.com>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
CC:	Karl Schendel <kschendel@...allegro.com>,
	Zach Brown <zach.brown@...cle.com>,
	Benjamin LaHaise <bcrl@...ck.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Nick Piggin <nickpiggin@...oo.com.au>,
	Leonid Ananiev <leonid.i.ananiev@...ux.intel.com>,
	Chris Mason <chris.mason@...cle.com>
Subject: Re: [PATCH] Fix bad data from non-direct-io read after direct-io
 write

Linus Torvalds wrote:
> Hmm. If I read this right, this bug seems to have been introduced by 
> commit 65b8291c4000e5f38fc94fb2ca0cb7e8683c8a1b ("dio: invalidate clean 
> pages before dio write") back in March.

Agreed.  And it's a really dumb bug.  ->direct_io will almost always
return -EIOCBQUEUED for aio dio so it won't be invalidating for aio dio
writes.  (Notice that the testing in that commit mentions two racing
processes, I bet U$1M that I only tested sync dio :/)

I think that test should be changed to

  if (retval < 0 && retval != -EIOCBQUEUED)
	goto out;

> However, with both the old and the new code _and_ with your patch, the 
> return code - in case the invalidate failed - was corrupted. So we may 
> actually end up doing some IO, but then returning the "wrong" error code 
> from the invalidate. Hmm?

If the invalidation fails then the app is left with stale data in the
page cache and current data on disk.  The return code corruption you're
referring to was intended to communicate this scary situation to the app
with EIO.

It sucks.  Does it suck more than returning success for the dio write
when later buffered reads will return stale data?  I dunno.  What does
the peanut gallery think?

> And maybe some day we can all agree that direct_IO is crap and should not 
> be done.

Chris (Mason) and I certainly love the idea of getting rid of
fs/direct-io.c.  Getting O_DIRECT working with the page-granular
buffered locking rules while doing large IOs (and, as far as I know,
potentially sector-granular) without noticeable performance regressions
is a mess.

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