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:	Sun, 14 Sep 2014 15:25:45 +0300
From:	Boaz Harrosh <openosd@...il.com>
To:	Dave Chinner <david@...morbit.com>
CC:	Matthew Wilcox <matthew.r.wilcox@...el.com>,
	linux-fsdevel@...r.kernel.org, linux-mm@...ck.org,
	linux-kernel@...r.kernel.org,
	Ross Zwisler <ross.zwisler@...ux.intel.com>,
	willy@...ux.intel.com
Subject: Re: [PATCH v10 20/21] ext4: Add DAX functionality

On 09/11/2014 07:38 AM, Dave Chinner wrote:
<>
> 
> And so ext4 is buggy, because what ext4 does ....
> 
> ... is not a retry - it falls back to a fundamentally different
> code path. i.e:
> 
> sys_write()
> ....
> 	new_sync_write
> 	  ext4_file_write_iter
> 	    __generic_file_write_iter(O_DIRECT)
> 	      written = generic_file_direct_write()
> 	      if (error || complete write)
> 	        return
> 	      /* short write! do buffered IO to finish! */
> 	      generic_perform_write()
> 	        loop {
> 			ext4_write_begin
> 			ext4_write_end
> 		}
> 
> and so we allocate pages in the page cache and do buffered IO into
> them because DAX doesn't hook ->writebegin/write_end as we are
> supposed to intercept all buffered IO at a higher level.
> 
> This causes data corruption when tested at ENOSPC on DAX enabled
> ext4 filesystems. I think that it's an oversight and hence a bug
> that needs to be fixed but I'm first asking Willy to see if it was
> intentional or not because maybe I missed sometihng in the past 4
> months since I've paid really close attention to the DAX code.
> 
> And in saying that, Boaz, I'd suggest you spend some time looking at
> the history of the DAX patchset. Pay careful note to who came up
> with the original idea and architecture that led to the IO path you
> are so stridently defending.....
> 

Yes! you are completely right, and I have not seen this bug. The same bug
exist with ext2 as well. I think this is a bug in patch:
	[PATCH v10 07/21] Replace XIP read and write with DAX I/O

It needs a:
@@ -2584,7 +2584,7 @@ ssize_t __generic_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
 		loff_t endbyte;
 
 		written = generic_file_direct_write(iocb, from, pos);
-		if (written < 0 || written == count)
+		if (written < 0 || written == count || IS_DAX(inode))
 			goto out;
 
 		/*

Or something like that. Is that what you meant?

(You have commented on the ext4 patch but this is already earlier in ext2
 so I did not see it, sorry. "If you explain slow I finally get it ;-)" )

> Cheers,
> Dave.

Yes I agree this is a very bad data corruption bug. I also think that the
read path should not be allowed to fall back to buffered IO just the same
for the same reason. We must not allow any real data in page_cache for a
DAX file.

Thanks for explaining
Boaz


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

Powered by blists - more mailing lists