fs/ntfs3: disable page fault during ntfs_fiemap()

Commit Message

Tetsuo Handa April 12, 2023, 1:11 p.m. UTC

syzbot is reporting circular locking dependency between ntfs_file_mmap()
(which has mm->mmap_lock => ni->ni_lock dependency) and ntfs_fiemap()
(which has ni->ni_lock => mm->mmap_lock dependency).

Since ni_fiemap() is called by ioctl(FS_IOC_FIEMAP) via optional
"struct inode_operations"->fiemap callback, I assume that importance of
ni_fiemap() is lower than ntfs_file_mmap().

Also, since Documentation/filesystems/fiemap.rst says that "If an error
is encountered while copying the extent to user memory, -EFAULT will be
returned.", I assume that ioctl(FS_IOC_FIEMAP) users can handle -EFAULT
error.

Therefore, in order to eliminate possibility of deadlock, until

  Assumed ni_lock.
  TODO: Less aggressive locks.

comment in ni_fiemap() is removed, use ni_fiemap() with best-effort basis
(i.e. fail with -EFAULT when a page fault is inevitable).

Reported-by: syzbot <[e-mail]>
Link: https://syzkaller.appspot.com/bug?extid=96cee7d33ca3f87eee86
Fixes: 4342306f0f0d ("fs/ntfs3: Add file operations and implementation")
Signed-off-by: Tetsuo Handa <[e-mail]>
---
 fs/ntfs3/file.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Matthew Wilcox April 12, 2023, 1:13 p.m. UTC | #1

On Wed, Apr 12, 2023 at 10:11:08PM +0900, Tetsuo Handa wrote:
> syzbot is reporting circular locking dependency between ntfs_file_mmap()
> (which has mm->mmap_lock => ni->ni_lock dependency) and ntfs_fiemap()
> (which has ni->ni_lock => mm->mmap_lock dependency).
> 
> Since ni_fiemap() is called by ioctl(FS_IOC_FIEMAP) via optional
> "struct inode_operations"->fiemap callback, I assume that importance of
> ni_fiemap() is lower than ntfs_file_mmap().
> 
> Also, since Documentation/filesystems/fiemap.rst says that "If an error
> is encountered while copying the extent to user memory, -EFAULT will be
> returned.", I assume that ioctl(FS_IOC_FIEMAP) users can handle -EFAULT
> error.

What?  No, that doesn't mean "You can return -EFAULT because random luck".
That means "If you pass it an invalid address, you'll get -EFAULT back".

NACK.

Tetsuo Handa April 12, 2023, 1:29 p.m. UTC | #2

On 2023/04/12 22:13, Matthew Wilcox wrote:
>> Also, since Documentation/filesystems/fiemap.rst says that "If an error
>> is encountered while copying the extent to user memory, -EFAULT will be
>> returned.", I assume that ioctl(FS_IOC_FIEMAP) users can handle -EFAULT
>> error.
> 
> What?  No, that doesn't mean "You can return -EFAULT because random luck".
> That means "If you pass it an invalid address, you'll get -EFAULT back".
> 
> NACK.

Then, why does fiemap.rst say "If an error is encountered" rather than
"If an invalid address is passed" ?

Does the definition of -EFAULT limited to "the caller does not have permission
to access this address" ?

----------
int fiemap_fill_next_extent(struct fiemap_extent_info *fieinfo, u64 logical,
			    u64 phys, u64 len, u32 flags)
{
	struct fiemap_extent extent;
	struct fiemap_extent __user *dest = fieinfo->fi_extents_start;

	/* only count the extents */
	if (fieinfo->fi_extents_max == 0) {
		fieinfo->fi_extents_mapped++;
		return (flags & FIEMAP_EXTENT_LAST) ? 1 : 0;
	}

	if (fieinfo->fi_extents_mapped >= fieinfo->fi_extents_max)
		return 1;

	if (flags & SET_UNKNOWN_FLAGS)
		flags |= FIEMAP_EXTENT_UNKNOWN;
	if (flags & SET_NO_UNMOUNTED_IO_FLAGS)
		flags |= FIEMAP_EXTENT_ENCODED;
	if (flags & SET_NOT_ALIGNED_FLAGS)
		flags |= FIEMAP_EXTENT_NOT_ALIGNED;

	memset(&extent, 0, sizeof(extent));
	extent.fe_logical = logical;
	extent.fe_physical = phys;
	extent.fe_length = len;
	extent.fe_flags = flags;

	dest += fieinfo->fi_extents_mapped;
	if (copy_to_user(dest, &extent, sizeof(extent)))
		return -EFAULT;

	fieinfo->fi_extents_mapped++;
	if (fieinfo->fi_extents_mapped == fieinfo->fi_extents_max)
		return 1;
	return (flags & FIEMAP_EXTENT_LAST) ? 1 : 0;
}
----------

If copy_to_user() must not fail other than "the caller does not have
permission to access this address", what should we do for now?
Just remove ntfs_fiemap() and return -EOPNOTSUPP ?

Matthew Wilcox April 12, 2023, 2:24 p.m. UTC | #3

On Wed, Apr 12, 2023 at 10:29:37PM +0900, Tetsuo Handa wrote:
> On 2023/04/12 22:13, Matthew Wilcox wrote:
> >> Also, since Documentation/filesystems/fiemap.rst says that "If an error
> >> is encountered while copying the extent to user memory, -EFAULT will be
> >> returned.", I assume that ioctl(FS_IOC_FIEMAP) users can handle -EFAULT
> >> error.
> > 
> > What?  No, that doesn't mean "You can return -EFAULT because random luck".
> > That means "If you pass it an invalid address, you'll get -EFAULT back".
> > 
> > NACK.
> 
> Then, why does fiemap.rst say "If an error is encountered" rather than
> "If an invalid address is passed" ?

Because people are bad at writing.

> Does the definition of -EFAULT limited to "the caller does not have permission
> to access this address" ?

Or the address isn't mapped.

> If copy_to_user() must not fail other than "the caller does not have
> permission to access this address", what should we do for now?
> Just remove ntfs_fiemap() and return -EOPNOTSUPP ?

No, fix it properly.  Or don't fix it at all and let somebody else fix it.

Patch

diff --git a/fs/ntfs3/file.c b/fs/ntfs3/file.c
index e9bdc1ff08c9..a9e7204e1579 100644
--- a/fs/ntfs3/file.c
+++ b/fs/ntfs3/file.c
@@ -1146,9 +1146,11 @@  int ntfs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
 		return err;
 
 	ni_lock(ni);
+	pagefault_disable();
 
 	err = ni_fiemap(ni, fieinfo, start, len);
 
+	pagefault_enable();
 	ni_unlock(ni);
 
 	return err;