On Tue, Nov 29, 2022 at 05:08:31PM +0800, Hillf Danton wrote: > On 29 Nov 2022 04:04:35 +0000 Al Viro <viro@xxxxxxxxxxxxxxxxxx> > > 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.
