[PATCH] vfs: fix vfs_clone_file_range() for overlayfs files

[PATCH] vfs: fix vfs_clone_file_range() for overlayfs files

[Amir Goldstein]

With overlayfs, it is wrong to compare file_inode(inode)->i_sb
of regular files with those of non-regular files, because the
former reference the real (upper/lower) sb and the latter reference
the overlayfs sb.

Move the test for same super block after the sanity tests for
clone range of directory and non-regular file.

This change fixes xfstest generic/157, which returned EXDEV instead
of EISDIR/EINVAL in the following test cases over overlayfs:

  echo "Try to reflink a dir"
  _reflink_range $testdir1/dir1 0 $testdir1/file2 0 $blksz

  echo "Try to reflink a device"
  _reflink_range $testdir1/dev1 0 $testdir1/file2 0 $blksz

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/read_write.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/read_write.c b/fs/read_write.c
index 1244bd5..d547301 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -1657,6 +1657,11 @@ int vfs_clone_file_range(struct file *file_in, loff_t pos_in,
 	struct inode *inode_out = file_inode(file_out);
 	int ret;
 
+	if (S_ISDIR(inode_in->i_mode) || S_ISDIR(inode_out->i_mode))
+		return -EISDIR;
+	if (!S_ISREG(inode_in->i_mode) || !S_ISREG(inode_out->i_mode))
+		return -EINVAL;
+
 	/*
 	 * FICLONE/FICLONERANGE ioctls enforce that src and dest files are on
 	 * the same mount. Practically, they only need to be on the same file
@@ -1665,11 +1670,6 @@ int vfs_clone_file_range(struct file *file_in, loff_t pos_in,
 	if (inode_in->i_sb != inode_out->i_sb)
 		return -EXDEV;
 
-	if (S_ISDIR(inode_in->i_mode) || S_ISDIR(inode_out->i_mode))
-		return -EISDIR;
-	if (!S_ISREG(inode_in->i_mode) || !S_ISREG(inode_out->i_mode))
-		return -EINVAL;
-
 	if (!(file_in->f_mode & FMODE_READ) ||
 	    !(file_out->f_mode & FMODE_WRITE) ||
 	    (file_out->f_flags & O_APPEND))
-- 
2.7.4

Re: [PATCH] vfs: fix vfs_clone_file_range() for overlayfs files

[Miklos Szeredi]

On Wed, Oct 26, 2016 at 9:34 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> With overlayfs, it is wrong to compare file_inode(inode)->i_sb
> of regular files with those of non-regular files, because the
> former reference the real (upper/lower) sb and the latter reference
> the overlayfs sb.
>
> Move the test for same super block after the sanity tests for
> clone range of directory and non-regular file.

Better:  compare ->f_path.dentry->d_sb instead of file_inode()->i_sb.
We don't want to be mixing files that come from overlayfs and ones
that come from the underlying layers.

BTW, would it be worthwhile adding clone_file_range() support to overlayfs?

Thanks,
Miklos

Re: [PATCH] vfs: fix vfs_clone_file_range() for overlayfs files

[Amir Goldstein]

On Fri, Oct 28, 2016 at 5:25 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Wed, Oct 26, 2016 at 9:34 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>> With overlayfs, it is wrong to compare file_inode(inode)->i_sb
>> of regular files with those of non-regular files, because the
>> former reference the real (upper/lower) sb and the latter reference
>> the overlayfs sb.
>>
>> Move the test for same super block after the sanity tests for
>> clone range of directory and non-regular file.
>
> Better:  compare ->f_path.dentry->d_sb instead of file_inode()->i_sb.
> We don't want to be mixing files that come from overlayfs and ones
> that come from the underlying layers.

That's not a good option.
When source file is in lower and dest file is in upper, then clone range
should go forward iff both lower and upper inodes are on the same sb.
The test as it is checks for this properly.

>
> BTW, would it be worthwhile adding clone_file_range() support to overlayfs?
>

There is nothing to add. It works quite well because clone_file_range works
on inode level. In fact, the entire 'clone' group of xfstests passes
with overlayfs
of lower and upper on the same XFS filesystem (with reflink support).

Amir.

Re: [PATCH] vfs: fix vfs_clone_file_range() for overlayfs files

[Amir Goldstein]

On Fri, Oct 28, 2016 at 6:25 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Fri, Oct 28, 2016 at 5:25 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>> On Wed, Oct 26, 2016 at 9:34 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>>> With overlayfs, it is wrong to compare file_inode(inode)->i_sb
>>> of regular files with those of non-regular files, because the
>>> former reference the real (upper/lower) sb and the latter reference
>>> the overlayfs sb.
>>>
>>> Move the test for same super block after the sanity tests for
>>> clone range of directory and non-regular file.
>>
>> Better:  compare ->f_path.dentry->d_sb instead of file_inode()->i_sb.
>> We don't want to be mixing files that come from overlayfs and ones
>> that come from the underlying layers.
>
> That's not a good option.
> When source file is in lower and dest file is in upper, then clone range
> should go forward iff both lower and upper inodes are on the same sb.
> The test as it is checks for this properly.
>

Ping.

Do you have any more question about this change?
Do you mind queuing this up for next along with the rest of the
clone_file_range() changes?

It'd be nice to have all xfstests pass for overlayfs on 4.10...

>>
>> BTW, would it be worthwhile adding clone_file_range() support to overlayfs?
>>
>
> There is nothing to add. It works quite well because clone_file_range works
> on inode level. In fact, the entire 'clone' group of xfstests passes
> with overlayfs
> of lower and upper on the same XFS filesystem (with reflink support).
>
> Amir.

Re: [PATCH] vfs: fix vfs_clone_file_range() for overlayfs files

[Miklos Szeredi]

On Thu, Nov 3, 2016 at 8:02 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Fri, Oct 28, 2016 at 6:25 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>> On Fri, Oct 28, 2016 at 5:25 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>>> On Wed, Oct 26, 2016 at 9:34 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>>>> With overlayfs, it is wrong to compare file_inode(inode)->i_sb
>>>> of regular files with those of non-regular files, because the
>>>> former reference the real (upper/lower) sb and the latter reference
>>>> the overlayfs sb.
>>>>
>>>> Move the test for same super block after the sanity tests for
>>>> clone range of directory and non-regular file.
>>>
>>> Better:  compare ->f_path.dentry->d_sb instead of file_inode()->i_sb.
>>> We don't want to be mixing files that come from overlayfs and ones
>>> that come from the underlying layers.
>>
>> That's not a good option.
>> When source file is in lower and dest file is in upper, then clone range
>> should go forward iff both lower and upper inodes are on the same sb.
>> The test as it is checks for this properly.
>>
>
> Ping.
>
> Do you have any more question about this change?
> Do you mind queuing this up for next along with the rest of the
> clone_file_range() changes?

Applied and pushed to #overlayfs-next.

Thanks,
Miklos