All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] btrfs: don't fallback to buffered read if we don't need to
@ 2020-10-22 14:05 Johannes Thumshirn
  2020-10-22 15:00 ` Goldwyn Rodrigues
  0 siblings, 1 reply; 3+ messages in thread
From: Johannes Thumshirn @ 2020-10-22 14:05 UTC (permalink / raw)
  To: David Sterba; +Cc: Goldwyn Rodrigues, linux-btrfs, Johannes Thumshirn

Since we switched to the iomap infrastructure in b5ff9f1a96e8f ("btrfs:
switch to iomap for direct IO") we're calling generic_file_buffered_read()
directly and not via generic_file_read_iter() anymore.

If the read could read everything there is no need to bother calling
generic_file_buffered_read(), like it is handled in
generic_file_read_iter().

If we call generic_file_buffered_read() in this case we can hit a
situation where we do an invalid readahead and cause this UBSAN splat:
johannes@redsun60:linux(btrfs-misc-next)$ kasan_symbolize.py < ubsan.txt
rapido1:/home/johannes/src/xfstests-dev# cat results/generic/091.dmesg
run fstests generic/091 at 2020-10-21 10:52:32
================================================================================
UBSAN: shift-out-of-bounds in ./include/linux/log2.h:57:13
shift exponent 64 is too large for 64-bit type 'long unsigned int'
CPU: 0 PID: 656 Comm: fsx Not tainted 5.9.0-rc7+ #821
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.13.0-0-gf21b5a4-rebuilt.opensuse.org 04/01/2014
Call Trace:
 __dump_stack lib/dump_stack.c:77
 dump_stack+0x57/0x70 lib/dump_stack.c:118
 ubsan_epilogue+0x5/0x40 lib/ubsan.c:148
 __ubsan_handle_shift_out_of_bounds.cold+0x61/0xe9 lib/ubsan.c:395
 __roundup_pow_of_two ./include/linux/log2.h:57
 get_init_ra_size mm/readahead.c:318
 ondemand_readahead.cold+0x16/0x2c mm/readahead.c:530
 generic_file_buffered_read+0x3ac/0x840 mm/filemap.c:2199
 call_read_iter ./include/linux/fs.h:1876
 new_sync_read+0x102/0x180 fs/read_write.c:415
 vfs_read+0x11c/0x1a0 fs/read_write.c:481
 ksys_read+0x4f/0xc0 fs/read_write.c:615
 do_syscall_64+0x33/0x40 arch/x86/entry/common.c:46
 entry_SYSCALL_64_after_hwframe+0x44/0xa9 arch/x86/entry/entry_64.S:118
RIP: 0033:0x7fe87fee992e
Code: 0f 1f 40 00 48 8b 15 a1 96 00 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb ba 0f 1f 00 64 8b 04 25 18 00 00 00 85 c0 75 14 0f 05 <48> 3d 00 f0 ff ff 77 5a c3 66 0f 1f 84 00 00 00 00 00 48 83 ec 28
RSP: 002b:00007ffe01605278 EFLAGS: 00000246 ORIG_RAX: 0000000000000000
RAX: ffffffffffffffda RBX: 000000000004f000 RCX: 00007fe87fee992e
RDX: 0000000000004000 RSI: 0000000001677000 RDI: 0000000000000003
RBP: 000000000004f000 R08: 0000000000004000 R09: 000000000004f000
R10: 0000000000053000 R11: 0000000000000246 R12: 0000000000004000
R13: 0000000000000000 R14: 000000000007a120 R15: 0000000000000000
================================================================================
BTRFS info (device nullb0): has skinny extents
BTRFS info (device nullb0): ZONED mode enabled, zone size 268435456 B
BTRFS info (device nullb0): enabling ssd optimizations

Fixes: b5ff9f1a96e8f ("btrfs: switch to iomap for direct IO")
Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
---
Changes to v1:
- add check for read beyond EOF (Goldwyn)
- Polish subject a bit
---
 fs/btrfs/file.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index 6f5ecba74f54..1c97e559aefb 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -3612,7 +3612,8 @@ static ssize_t btrfs_file_read_iter(struct kiocb *iocb, struct iov_iter *to)
 		inode_lock_shared(inode);
 		ret = btrfs_direct_IO(iocb, to);
 		inode_unlock_shared(inode);
-		if (ret < 0)
+		if (ret < 0 || !iov_iter_count(to) ||
+		    iocb->ki_pos >= i_size_read(file_inode(iocb->ki_filp)))
 			return ret;
 	}
 
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [PATCH v2] btrfs: don't fallback to buffered read if we don't need to
  2020-10-22 14:05 [PATCH v2] btrfs: don't fallback to buffered read if we don't need to Johannes Thumshirn
@ 2020-10-22 15:00 ` Goldwyn Rodrigues
  2020-10-23 16:05   ` David Sterba
  0 siblings, 1 reply; 3+ messages in thread
From: Goldwyn Rodrigues @ 2020-10-22 15:00 UTC (permalink / raw)
  To: Johannes Thumshirn; +Cc: David Sterba, linux-btrfs

On 23:05 22/10, Johannes Thumshirn wrote:
> Since we switched to the iomap infrastructure in b5ff9f1a96e8f ("btrfs:
> switch to iomap for direct IO") we're calling generic_file_buffered_read()
> directly and not via generic_file_read_iter() anymore.
> 
> If the read could read everything there is no need to bother calling

If the DIO read

> generic_file_buffered_read(), like it is handled in
> generic_file_read_iter().
> 
> If we call generic_file_buffered_read() in this case we can hit a
> situation where we do an invalid readahead and cause this UBSAN splat:
> johannes@redsun60:linux(btrfs-misc-next)$ kasan_symbolize.py < ubsan.txt
> rapido1:/home/johannes/src/xfstests-dev# cat results/generic/091.dmesg
> run fstests generic/091 at 2020-10-21 10:52:32
> ================================================================================
> UBSAN: shift-out-of-bounds in ./include/linux/log2.h:57:13
> shift exponent 64 is too large for 64-bit type 'long unsigned int'
> CPU: 0 PID: 656 Comm: fsx Not tainted 5.9.0-rc7+ #821
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.13.0-0-gf21b5a4-rebuilt.opensuse.org 04/01/2014
> Call Trace:
>  __dump_stack lib/dump_stack.c:77
>  dump_stack+0x57/0x70 lib/dump_stack.c:118
>  ubsan_epilogue+0x5/0x40 lib/ubsan.c:148
>  __ubsan_handle_shift_out_of_bounds.cold+0x61/0xe9 lib/ubsan.c:395
>  __roundup_pow_of_two ./include/linux/log2.h:57
>  get_init_ra_size mm/readahead.c:318
>  ondemand_readahead.cold+0x16/0x2c mm/readahead.c:530
>  generic_file_buffered_read+0x3ac/0x840 mm/filemap.c:2199
>  call_read_iter ./include/linux/fs.h:1876
>  new_sync_read+0x102/0x180 fs/read_write.c:415
>  vfs_read+0x11c/0x1a0 fs/read_write.c:481
>  ksys_read+0x4f/0xc0 fs/read_write.c:615
>  do_syscall_64+0x33/0x40 arch/x86/entry/common.c:46
>  entry_SYSCALL_64_after_hwframe+0x44/0xa9 arch/x86/entry/entry_64.S:118
> RIP: 0033:0x7fe87fee992e
> Code: 0f 1f 40 00 48 8b 15 a1 96 00 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb ba 0f 1f 00 64 8b 04 25 18 00 00 00 85 c0 75 14 0f 05 <48> 3d 00 f0 ff ff 77 5a c3 66 0f 1f 84 00 00 00 00 00 48 83 ec 28
> RSP: 002b:00007ffe01605278 EFLAGS: 00000246 ORIG_RAX: 0000000000000000
> RAX: ffffffffffffffda RBX: 000000000004f000 RCX: 00007fe87fee992e
> RDX: 0000000000004000 RSI: 0000000001677000 RDI: 0000000000000003
> RBP: 000000000004f000 R08: 0000000000004000 R09: 000000000004f000
> R10: 0000000000053000 R11: 0000000000000246 R12: 0000000000004000
> R13: 0000000000000000 R14: 000000000007a120 R15: 0000000000000000
> ================================================================================
> BTRFS info (device nullb0): has skinny extents
> BTRFS info (device nullb0): ZONED mode enabled, zone size 268435456 B
> BTRFS info (device nullb0): enabling ssd optimizations
> 
> Fixes: b5ff9f1a96e8f ("btrfs: switch to iomap for direct IO")
> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Reviewed-by: Goldwyn Rodrigues <rgoldwyn@suse.com>

> ---
> Changes to v1:
> - add check for read beyond EOF (Goldwyn)
> - Polish subject a bit
> ---
>  fs/btrfs/file.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> index 6f5ecba74f54..1c97e559aefb 100644
> --- a/fs/btrfs/file.c
> +++ b/fs/btrfs/file.c
> @@ -3612,7 +3612,8 @@ static ssize_t btrfs_file_read_iter(struct kiocb *iocb, struct iov_iter *to)
>  		inode_lock_shared(inode);
>  		ret = btrfs_direct_IO(iocb, to);
>  		inode_unlock_shared(inode);
> -		if (ret < 0)
> +		if (ret < 0 || !iov_iter_count(to) ||
> +		    iocb->ki_pos >= i_size_read(file_inode(iocb->ki_filp)))
>  			return ret;
>  	}
>  
> -- 
> 2.26.2
> 

-- 
Goldwyn

^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [PATCH v2] btrfs: don't fallback to buffered read if we don't need to
  2020-10-22 15:00 ` Goldwyn Rodrigues
@ 2020-10-23 16:05   ` David Sterba
  0 siblings, 0 replies; 3+ messages in thread
From: David Sterba @ 2020-10-23 16:05 UTC (permalink / raw)
  To: Goldwyn Rodrigues; +Cc: Johannes Thumshirn, David Sterba, linux-btrfs

On Thu, Oct 22, 2020 at 10:00:33AM -0500, Goldwyn Rodrigues wrote:
> > diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> > index 6f5ecba74f54..1c97e559aefb 100644
> > --- a/fs/btrfs/file.c
> > +++ b/fs/btrfs/file.c
> > @@ -3612,7 +3612,8 @@ static ssize_t btrfs_file_read_iter(struct kiocb *iocb, struct iov_iter *to)
> >  		inode_lock_shared(inode);
> >  		ret = btrfs_direct_IO(iocb, to);
> >  		inode_unlock_shared(inode);
> > -		if (ret < 0)
> > +		if (ret < 0 || !iov_iter_count(to) ||
> > +		    iocb->ki_pos >= i_size_read(file_inode(iocb->ki_filp)))
> >  			return ret;

JFYI, this conflicts with patch "btrfs: split btrfs_direct_IO to read
and write" from your dsync/dio series it will need to be refreshed.

^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-10-23 16:06 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-22 14:05 [PATCH v2] btrfs: don't fallback to buffered read if we don't need to Johannes Thumshirn
2020-10-22 15:00 ` Goldwyn Rodrigues
2020-10-23 16:05   ` David Sterba

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.