[PATCH 0/4] Assorted overlayfs fixes for 4.19-rc5

[PATCH 0/4] Assorted overlayfs fixes for 4.19-rc5

[Amir Goldstein]

Miklos,

Found one memory leak that needs a fix for stable and
two missing freeze protections in stacked f_ops.

Last patch is not a bugfix, but it fixes a trap that we set
for developers back in v4.10 - and you happened to step into.

The series passed xfstests -g quick with overlayfs over
xfs and ext4 and the usual unionmount testsuite torture.

The memory leak was detected with tests overlay/051,053
after I enabled kmemleak on my test setup.

Thanks,
Amir.

Amir Goldstein (4):
  ovl: fix memory leak on unlink of indexed file
  ovl: fix freeze protection bypass in ovl_write_iter()
  ovl: fix freeze protection bypass in ovl_clone_file_range()
  vfs: swap names of {do,vfs}_clone_file_range()

 fs/ioctl.c             |  2 +-
 fs/nfsd/vfs.c          |  3 ++-
 fs/overlayfs/copy_up.c |  2 +-
 fs/overlayfs/file.c    |  2 ++
 fs/overlayfs/util.c    |  3 ++-
 fs/read_write.c        | 17 +++++++++++++++--
 include/linux/fs.h     | 17 +++--------------
 7 files changed, 26 insertions(+), 20 deletions(-)

-- 
2.17.1

[PATCH 1/4] ovl: fix memory leak on unlink of indexed file

[Amir Goldstein]

Fixes: caf70cb2ba5d ("ovl: cleanup orphan index entries")
Cc: <stable@vger.kernel.org> # v4.13
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/util.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index 8cfb62cc8672..ace4fe4c39a9 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -683,7 +683,7 @@ static void ovl_cleanup_index(struct dentry *dentry)
 	struct dentry *upperdentry = ovl_dentry_upper(dentry);
 	struct dentry *index = NULL;
 	struct inode *inode;
-	struct qstr name;
+	struct qstr name = { };
 	int err;
 
 	err = ovl_get_index_name(lowerdentry, &name);
@@ -726,6 +726,7 @@ static void ovl_cleanup_index(struct dentry *dentry)
 		goto fail;
 
 out:
+	kfree(name.name);
 	dput(index);
 	return;
 
-- 
2.17.1

[PATCH 2/4] ovl: fix freeze protection bypass in ovl_write_iter()

[Amir Goldstein]

Tested by re-writing to an open overlayfs file while upper ext4 is frozen:

  xfs_io -f /ovl/x
  xfs_io> pwrite 0 4096
                             fsfreeze -f /ext4
  xfs_io> pwrite 0 4096

  WARNING: CPU: 0 PID: 1492 at fs/ext4/ext4_jbd2.c:53 \
           ext4_journal_check_start+0x48/0x82

After the fix, the second write blocks in ovl_write_iter() and avoids
hitting WARN_ON(sb->s_writers.frozen == SB_FREEZE_COMPLETE) in
ext4_journal_check_start().

Fixes: 2a92e07edc5e ("ovl: add ovl_write_iter()")
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/file.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
index aeaefd2a551b..986313da0c88 100644
--- a/fs/overlayfs/file.c
+++ b/fs/overlayfs/file.c
@@ -240,8 +240,10 @@ static ssize_t ovl_write_iter(struct kiocb *iocb, struct iov_iter *iter)
 		goto out_unlock;
 
 	old_cred = ovl_override_creds(file_inode(file)->i_sb);
+	file_start_write(real.file);
 	ret = vfs_iter_write(real.file, iter, &iocb->ki_pos,
 			     ovl_iocb_to_rwf(iocb));
+	file_end_write(real.file);
 	revert_creds(old_cred);
 
 	/* Update size */
-- 
2.17.1

[PATCH 3/4] ovl: fix freeze protection bypass in ovl_clone_file_range()

[Amir Goldstein]

Tested by doing clone on overlayfs while upper xfs+reflink is frozen:

  xfs_io -f /ovl/y
                             fsfreeze -f /xfs
  xfs_io> reflink /ovl/x

Before the fix xfs_io enters xfs_reflink_remap_range() and blocks
in xfs_trans_alloc(). After the fix, xfs_io blocks outside xfs code
in ovl_clone_file_range().

Fixes: 8ede205541ff ("ovl: add reflink/copyfile/dedup support")
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/file.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
index 986313da0c88..5d1b4b38f743 100644
--- a/fs/overlayfs/file.c
+++ b/fs/overlayfs/file.c
@@ -461,7 +461,7 @@ static ssize_t ovl_copyfile(struct file *file_in, loff_t pos_in,
 		break;
 
 	case OVL_CLONE:
-		ret = vfs_clone_file_range(real_in.file, pos_in,
+		ret = do_clone_file_range(real_in.file, pos_in,
 					   real_out.file, pos_out, len);
 		break;
 
-- 
2.17.1

[PATCH 4/4] vfs: swap names of {do,vfs}_clone_file_range()

[Amir Goldstein]

Commit 031a072a0b8a ("vfs: call vfs_clone_file_range() under freeze
protection") created a wrapper do_clone_file_range() around
vfs_clone_file_range() moving the freeze protection to former, so
overlayfs could call the latter.

The more common vfs practice is to call do_xxx helpers from vfs_xxx
helpers, where freeze protecction is taken in the vfs_xxx helper, so
this anomality could be a source of confusion.

It seems that commit 8ede205541ff ("ovl: add reflink/copyfile/dedup
support") may have fallen a victim to this confusion -
ovl_clone_file_range() calls the vfs_clone_file_range() helper in the
hope of getting freeze protection on upper fs, but in fact results in
overlayfs allowing to bypass upper fs freeze protection.

Swap the names of the two helpers to conform to common vfs practice
and call the correct helpers from overlayfs and nfsd.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/ioctl.c             |  2 +-
 fs/nfsd/vfs.c          |  3 ++-
 fs/overlayfs/copy_up.c |  2 +-
 fs/overlayfs/file.c    |  2 +-
 fs/read_write.c        | 17 +++++++++++++++--
 include/linux/fs.h     | 17 +++--------------
 6 files changed, 23 insertions(+), 20 deletions(-)

diff --git a/fs/ioctl.c b/fs/ioctl.c
index 3212c29235ce..2005529af560 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -230,7 +230,7 @@ static long ioctl_file_clone(struct file *dst_file, unsigned long srcfd,
 	ret = -EXDEV;
 	if (src_file.file->f_path.mnt != dst_file->f_path.mnt)
 		goto fdput;
-	ret = do_clone_file_range(src_file.file, off, dst_file, destoff, olen);
+	ret = vfs_clone_file_range(src_file.file, off, dst_file, destoff, olen);
 fdput:
 	fdput(src_file);
 	return ret;
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 55a099e47ba2..b53e76391e52 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -541,7 +541,8 @@ __be32 nfsd4_set_nfs4_label(struct svc_rqst *rqstp, struct svc_fh *fhp,
 __be32 nfsd4_clone_file_range(struct file *src, u64 src_pos, struct file *dst,
 		u64 dst_pos, u64 count)
 {
-	return nfserrno(do_clone_file_range(src, src_pos, dst, dst_pos, count));
+	return nfserrno(vfs_clone_file_range(src, src_pos, dst, dst_pos,
+					     count));
 }
 
 ssize_t nfsd_copy_file_range(struct file *src, u64 src_pos, struct file *dst,
diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 296037afecdb..1cc797a08a5b 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -141,7 +141,7 @@ static int ovl_copy_up_data(struct path *old, struct path *new, loff_t len)
 	}
 
 	/* Try to use clone_file_range to clone up within the same fs */
-	error = vfs_clone_file_range(old_file, 0, new_file, 0, len);
+	error = do_clone_file_range(old_file, 0, new_file, 0, len);
 	if (!error)
 		goto out;
 	/* Couldn't clone, so now we try to copy the data */
diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
index 5d1b4b38f743..986313da0c88 100644
--- a/fs/overlayfs/file.c
+++ b/fs/overlayfs/file.c
@@ -461,7 +461,7 @@ static ssize_t ovl_copyfile(struct file *file_in, loff_t pos_in,
 		break;
 
 	case OVL_CLONE:
-		ret = do_clone_file_range(real_in.file, pos_in,
+		ret = vfs_clone_file_range(real_in.file, pos_in,
 					   real_out.file, pos_out, len);
 		break;
 
diff --git a/fs/read_write.c b/fs/read_write.c
index 39b4a21dd933..8a2737f0d61d 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -1818,8 +1818,8 @@ int vfs_clone_file_prep_inodes(struct inode *inode_in, loff_t pos_in,
 }
 EXPORT_SYMBOL(vfs_clone_file_prep_inodes);
 
-int vfs_clone_file_range(struct file *file_in, loff_t pos_in,
-		struct file *file_out, loff_t pos_out, u64 len)
+int do_clone_file_range(struct file *file_in, loff_t pos_in,
+			struct file *file_out, loff_t pos_out, u64 len)
 {
 	struct inode *inode_in = file_inode(file_in);
 	struct inode *inode_out = file_inode(file_out);
@@ -1866,6 +1866,19 @@ int vfs_clone_file_range(struct file *file_in, loff_t pos_in,
 
 	return ret;
 }
+EXPORT_SYMBOL(do_clone_file_range);
+
+int vfs_clone_file_range(struct file *file_in, loff_t pos_in,
+			 struct file *file_out, loff_t pos_out, u64 len)
+{
+	int ret;
+
+	file_start_write(file_out);
+	ret = do_clone_file_range(file_in, pos_in, file_out, pos_out, len);
+	file_end_write(file_out);
+
+	return ret;
+}
 EXPORT_SYMBOL(vfs_clone_file_range);
 
 /*
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 4f0a11d0d296..25a449f37bb1 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1833,8 +1833,10 @@ extern ssize_t vfs_copy_file_range(struct file *, loff_t , struct file *,
 extern int vfs_clone_file_prep_inodes(struct inode *inode_in, loff_t pos_in,
 				      struct inode *inode_out, loff_t pos_out,
 				      u64 *len, bool is_dedupe);
+extern int do_clone_file_range(struct file *file_in, loff_t pos_in,
+			       struct file *file_out, loff_t pos_out, u64 len);
 extern int vfs_clone_file_range(struct file *file_in, loff_t pos_in,
-		struct file *file_out, loff_t pos_out, u64 len);
+				struct file *file_out, loff_t pos_out, u64 len);
 extern int vfs_dedupe_file_range_compare(struct inode *src, loff_t srcoff,
 					 struct inode *dest, loff_t destoff,
 					 loff_t len, bool *is_same);
@@ -2778,19 +2780,6 @@ static inline void file_end_write(struct file *file)
 	__sb_end_write(file_inode(file)->i_sb, SB_FREEZE_WRITE);
 }
 
-static inline int do_clone_file_range(struct file *file_in, loff_t pos_in,
-				      struct file *file_out, loff_t pos_out,
-				      u64 len)
-{
-	int ret;
-
-	file_start_write(file_out);
-	ret = vfs_clone_file_range(file_in, pos_in, file_out, pos_out, len);
-	file_end_write(file_out);
-
-	return ret;
-}
-
 /*
  * get_write_access() gets write permission for a file.
  * put_write_access() releases this write permission.
-- 
2.17.1

Re: [PATCH 1/4] ovl: fix memory leak on unlink of indexed file

[Greg KH]

On Tue, Sep 18, 2018 at 04:34:31PM +0300, Amir Goldstein wrote:
> Fixes: caf70cb2ba5d ("ovl: cleanup orphan index entries")
> Cc: <stable@vger.kernel.org> # v4.13
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/overlayfs/util.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

I know I reject patches without any changelog text present.  Maybe
Miklos is more forgiving than me... :)

greg k-h

Re: [PATCH 1/4] ovl: fix memory leak on unlink of indexed file

[Amir Goldstein]

On Tue, Sep 18, 2018 at 4:59 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Tue, Sep 18, 2018 at 04:34:31PM +0300, Amir Goldstein wrote:
> > Fixes: caf70cb2ba5d ("ovl: cleanup orphan index entries")
> > Cc: <stable@vger.kernel.org> # v4.13
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > ---
> >  fs/overlayfs/util.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
>
> I know I reject patches without any changelog text present.  Maybe
> Miklos is more forgiving than me... :)
>

Well, I did not intend to CC stable at this point, but since I did and since
you chimed in, Miklos, feel free to add to commit message:

The memory leak was detected by kmemleak when running xfstests
overlay/051,053

Thanks,
Amir.

Re: [PATCH 0/4] Assorted overlayfs fixes for 4.19-rc5

[Amir Goldstein]

On Tue, Sep 18, 2018 at 4:34 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> Miklos,
>
> Found one memory leak that needs a fix for stable and
> two missing freeze protections in stacked f_ops.
>
> Last patch is not a bugfix, but it fixes a trap that we set
> for developers back in v4.10 - and you happened to step into.
>
> The series passed xfstests -g quick with overlayfs over
> xfs and ext4 and the usual unionmount testsuite torture.
>
> The memory leak was detected with tests overlay/051,053
> after I enabled kmemleak on my test setup.
>

Miklos,

Maybe not enough time has passed to justify a PING, but you know I get
edgy towards the end of a release cycle, especially a weird release cycle as
this one...

P.S. removing Al from CC because his emails have been bouncing.
Al, are you there?

Thanks,
Amir.

Re: [PATCH 0/4] Assorted overlayfs fixes for 4.19-rc5

[Miklos Szeredi]

On Mon, Sep 24, 2018 at 10:25 AM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Tue, Sep 18, 2018 at 4:34 PM Amir Goldstein <amir73il@gmail.com> wrote:
>>
>> Miklos,
>>
>> Found one memory leak that needs a fix for stable and
>> two missing freeze protections in stacked f_ops.
>>
>> Last patch is not a bugfix, but it fixes a trap that we set
>> for developers back in v4.10 - and you happened to step into.
>>
>> The series passed xfstests -g quick with overlayfs over
>> xfs and ext4 and the usual unionmount testsuite torture.
>>
>> The memory leak was detected with tests overlay/051,053
>> after I enabled kmemleak on my test setup.
>>
>
> Miklos,
>
> Maybe not enough time has passed to justify a PING, but you know I get
> edgy towards the end of a release cycle, especially a weird release cycle as
> this one...

Thanks for the reminder.   Pushed to overlayfs-next and will ask Greg
to pull before -rc6.  If I forget, please remind again ;)

Thanks,
Miklos