Skip to content

Commit 9ecd10b

Browse files
guaneryutorvalds
authored andcommitted
direct-io: fix direct write stale data exposure from concurrent buffered read
Currently direct writes inside i_size on a DIO_SKIP_HOLES filesystem are not allowed to allocate blocks(get_more_blocks() sets 'create' to 0 before calling get_block() callback), if it's a sparse file, direct writes fall back to buffered writes to avoid stale data exposure from concurrent buffered read. But there're two cases that can result in stale data exposure are not correctly detected. 1. The detection for "writing inside i_size" is not sufficient, writes can be treated as "extending writes" wrongly. For example, direct write 1FSB (file system block) to a 1FSB sparse file on ext2/3/4, starting from offset 0, in this case it's writing inside i_size, but 'create' is non-zero, because 'block_in_file' and '(i_size_read(inode) >> blkbits' are both zero. 2. Direct writes starting from or beyong i_size (not inside i_size) also could trigger block allocation and expose stale data. For example, consider a sparse file with i_size of 2k, and a write to offset 2k or 3k into the file, with a filesystem block size of 4k. (Thanks to Jeff Moyer for pointing this case out in his review.) The first problem can be demostrated by running ltp-aiodio test ADSP045 many times. When testing on extN filesystems, I see test failures occasionally, buffered read could read non-zero (stale) data. ADSP045: dio_sparse -a 4k -w 4k -s 2k -n 1 dio_sparse 0 TINFO : Dirtying free blocks dio_sparse 0 TINFO : Starting I/O tests non zero buffer at buf[0] => 0xffffffaa,ffffffaa,ffffffaa,ffffffaa non-zero read at offset 0 dio_sparse 0 TINFO : Killing childrens(s) dio_sparse 1 TFAIL : dio_sparse.c:191: 1 children(s) exited abnormally The second problem can also be reproduced easily by a hacked dio_sparse program, which accepts an option to specify the write offset. What we should really do is to disable block allocation for writes that could result in filling holes inside i_size. Link: http://lkml.kernel.org/r/[email protected] Reviewed-by: Jan Kara <[email protected]> Signed-off-by: Eryu Guan <[email protected]> Cc: Al Viro <[email protected]> Signed-off-by: Andrew Morton <[email protected]> Signed-off-by: Linus Torvalds <[email protected]>
1 parent 38b52ef commit 9ecd10b

File tree

1 file changed

+7
-7
lines changed

1 file changed

+7
-7
lines changed

fs/direct-io.c

+7-7
Original file line numberDiff line numberDiff line change
@@ -628,20 +628,20 @@ static int get_more_blocks(struct dio *dio, struct dio_submit *sdio,
628628
map_bh->b_size = fs_count << i_blkbits;
629629

630630
/*
631-
* For writes inside i_size on a DIO_SKIP_HOLES filesystem we
632-
* forbid block creations: only overwrites are permitted.
633-
* We will return early to the caller once we see an
634-
* unmapped buffer head returned, and the caller will fall
635-
* back to buffered I/O.
631+
* For writes that could fill holes inside i_size on a
632+
* DIO_SKIP_HOLES filesystem we forbid block creations: only
633+
* overwrites are permitted. We will return early to the caller
634+
* once we see an unmapped buffer head returned, and the caller
635+
* will fall back to buffered I/O.
636636
*
637637
* Otherwise the decision is left to the get_blocks method,
638638
* which may decide to handle it or also return an unmapped
639639
* buffer head.
640640
*/
641641
create = dio->rw & WRITE;
642642
if (dio->flags & DIO_SKIP_HOLES) {
643-
if (sdio->block_in_file < (i_size_read(dio->inode) >>
644-
sdio->blkbits))
643+
if (fs_startblk <= ((i_size_read(dio->inode) - 1) >>
644+
i_blkbits))
645645
create = 0;
646646
}
647647

0 commit comments

Comments
 (0)