Re: [syzbot] WARNING in iov_iter_revert (3)

[Al Viro <viro@zeniv.linux.org.uk>]

On Tue, Nov 29, 2022 at 05:08:31PM +0800, Hillf Danton wrote:
> On 29 Nov 2022 04:04:35 +0000 Al Viro <viro@zeniv.linux.org.uk>
> > On Mon, Nov 28, 2022 at 02:57:49PM -0800, syzbot wrote:
> > > syzbot has found a reproducer for the following issue on:
> > 
> > [snip]
> > 
> > > syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=17219fbb880000
> > 
> > "syz_mount_image$ntfs3(" followed by arseloads of garbage.  And the thing
> > conspiciously missing?  Why, any ntfs3 maintainers in Cc...  Or lists,
> > for that matter...
> > 
> > >  generic_file_read_iter+0x3d4/0x540 mm/filemap.c:2804
> > >  do_iter_read+0x6e3/0xc10 fs/read_write.c:796
> > >  vfs_readv fs/read_write.c:916 [inline]
> > >  do_preadv+0x1f4/0x330 fs/read_write.c:1008
> > >  do_syscall_x64 arch/x86/entry/common.c:50 [inline]
> > >  do_syscall_64+0x3d/0xb0 arch/x86/entry/common.c:80
> > >  entry_SYSCALL_64_after_hwframe+0x63/0xcd
> > 
> > At a guess - something's screwed in ntfs3 ->direct_IO() (return value, most
> > likely).
> 
> 2798		retval = mapping->a_ops->direct_IO(iocb, iter);
> 2799		if (retval >= 0) {
> 2800		        iocb->ki_pos += retval;
> 2801		        count -= retval;
> 2802		}
> 2803		if (retval != -EIOCBQUEUED)
> 2804		        iov_iter_revert(iter, count - iov_iter_count(iter));
> 2805		
> 2806		/*
> 2807		 * Btrfs can have a short DIO read if we encounter
> 2808		 * compressed extents, so if there was an error, or if
> 2809		 * we've already read everything we wanted to, or if
> 2810		 * there was a short read because we hit EOF, go ahead
> 2811		 * and return.  Otherwise fallthrough to buffered io for
> 2812		 * the rest of the read.  Buffered reads will not work for
> 2813		 * DAX files, so don't bother trying.
> 2814		 */
> 2815		if (retval < 0 || !count || IS_DAX(inode))
> 2816		        return retval;
> 2817		if (iocb->ki_pos >= i_size_read(inode))
> 2818		        return retval;
> 
> 
> If ntfs3 is supposed to do nothing wrong with retval set to 5, why is
> iov_iter_revert() invoked? Is it correct to check -EIOCBQUEUED only if
> the direct_IO callback returns error?

->direct_IO() should return the amount of data actually copied to userland;
if that's how much it has consumed from iterator - great, iov_iter_revert(i, 0)
is a no-op.  If it has consumed more, the caller will take care of that.
If it has consumed say 4Kb of data from iterator, but claims that it has
managed to store 12Kb into that, it's broken and should be fixed.

If it wants to do revert on its own, for whatever reason, it is welcome - nothing
will break, as long as you do *not* return the value greater than the amount you
ended up taking from iterator.  However, I don't understand the reason why ntfs3
wants to bother (and appears to get it wrong, at that); the current rules are
such that caller will take care of revert.