[PATCH 0/4] ovl: fix IS_APPEND() checks

[PATCH 0/4] ovl: fix IS_APPEND() checks

[Amir Goldstein]

Miklos,

This series fixes the append-only violations found by the new
xfstest overlay/030.

I did it to see how bad it would be compared to propagating
the S_APPEND/S_IMMUTABLE flags to overlay inode.

IMO, the [f]truncate patches look ok, but the last vfs_open()
patch less so (?).

Do you think we can get away without propagating the flags?
If getflags()/setflags() were standard vfs ops, we could have
handled this nicer...
Those "fs specific flags" have long been a standard, so perhaps
its time to make the setflags/getflags a vfs api?

Amir.

Amir Goldstein (4):
  vfs: ftruncate freeze protect backing inode
  vfs: ftruncate check IS_APPEND() on backing inode
  vfs: truncate check IS_APPEND() on backing inode
  vfs: open check IS_APPEND() on backing inode

 fs/open.c | 34 +++++++++++++++++++++-------------
 1 file changed, 21 insertions(+), 13 deletions(-)

-- 
2.7.4

[PATCH 1/4] vfs: ftruncate freeze protect backing inode

[Amir Goldstein]

ftruncate an overlayfs inode was wrongly freeze protecting the
overlay file system instead of the backing file system.

Use file_start_write() instead of sb_start_write(), which
does the right thing and will also freeze protect both overlay and
backing fs when the time comes.

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

diff --git a/fs/open.c b/fs/open.c
index 949cef2..53b1b33 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -196,13 +196,13 @@ static long do_sys_ftruncate(unsigned int fd, loff_t length, int small)
 	if (IS_APPEND(inode))
 		goto out_putf;
 
-	sb_start_write(inode->i_sb);
+	file_start_write(f.file);
 	error = locks_verify_truncate(inode, f.file, length);
 	if (!error)
 		error = security_path_truncate(&f.file->f_path);
 	if (!error)
 		error = do_truncate(dentry, length, ATTR_MTIME|ATTR_CTIME, f.file);
-	sb_end_write(inode->i_sb);
+	file_end_write(f.file);
 out_putf:
 	fdput(f);
 out:
-- 
2.7.4

Re: [PATCH 1/4] vfs: ftruncate freeze protect backing inode

[Amir Goldstein]

On Thu, Apr 6, 2017 at 11:13 AM, Amir Goldstein <amir73il@gmail.com> wrote:
> ftruncate an overlayfs inode was wrongly freeze protecting the
> overlay file system instead of the backing file system.
>
> Use file_start_write() instead of sb_start_write(), which
> does the right thing and will also freeze protect both overlay and
> backing fs when the time comes.
>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/open.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fs/open.c b/fs/open.c
> index 949cef2..53b1b33 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -196,13 +196,13 @@ static long do_sys_ftruncate(unsigned int fd, loff_t length, int small)
>         if (IS_APPEND(inode))
>                 goto out_putf;
>
> -       sb_start_write(inode->i_sb);
> +       file_start_write(f.file);

Nah! this is wrong.
Should be mnt_want_write_file(f.file)

At the time of this patch mnt_want_write_file(f.file) translated to
sb_start_write(inode->i_sb)
but when next patch is going to change inode = file_inode(f.file)
sb_start_write(inode->i_sb) will no longer be correct
and mnt_want_write_file(f.file) is nicer than sb_start_write(locks_inode(f.file)


>         error = locks_verify_truncate(inode, f.file, length);
>         if (!error)
>                 error = security_path_truncate(&f.file->f_path);
>         if (!error)
>                 error = do_truncate(dentry, length, ATTR_MTIME|ATTR_CTIME, f.file);
> -       sb_end_write(inode->i_sb);
> +       file_end_write(f.file);
>  out_putf:
>         fdput(f);
>  out:
> --
> 2.7.4
>

[PATCH 2/4] vfs: ftruncate check IS_APPEND() on backing inode

[Amir Goldstein]

ftruncate an overlayfs inode was checking IS_APPEND() on
overlay inode, but overlay inode does not have the S_APPEND
flag and IS_APPEND() is always checked on backing inode in
other places.

Set 'inode' var to file_inode() so all checks are performed on
backing inode and use locks_inode() explicitly for calling
locks_verify_truncate().

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

diff --git a/fs/open.c b/fs/open.c
index 53b1b33..0e3c12b 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -165,7 +165,6 @@ COMPAT_SYSCALL_DEFINE2(truncate, const char __user *, path, compat_off_t, length
 static long do_sys_ftruncate(unsigned int fd, loff_t length, int small)
 {
 	struct inode *inode;
-	struct dentry *dentry;
 	struct fd f;
 	int error;
 
@@ -181,8 +180,7 @@ static long do_sys_ftruncate(unsigned int fd, loff_t length, int small)
 	if (f.file->f_flags & O_LARGEFILE)
 		small = 0;
 
-	dentry = f.file->f_path.dentry;
-	inode = dentry->d_inode;
+	inode = file_inode(f.file);
 	error = -EINVAL;
 	if (!S_ISREG(inode->i_mode) || !(f.file->f_mode & FMODE_WRITE))
 		goto out_putf;
@@ -197,11 +195,12 @@ static long do_sys_ftruncate(unsigned int fd, loff_t length, int small)
 		goto out_putf;
 
 	file_start_write(f.file);
-	error = locks_verify_truncate(inode, f.file, length);
+	error = locks_verify_truncate(locks_inode(f.file), f.file, length);
 	if (!error)
 		error = security_path_truncate(&f.file->f_path);
 	if (!error)
-		error = do_truncate(dentry, length, ATTR_MTIME|ATTR_CTIME, f.file);
+		error = do_truncate(f.file->f_path.dentry, length,
+				    ATTR_MTIME|ATTR_CTIME, f.file);
 	file_end_write(f.file);
 out_putf:
 	fdput(f);
-- 
2.7.4

[PATCH 3/4] vfs: truncate check IS_APPEND() on backing inode

[Amir Goldstein]

truncate an overlayfs inode was checking IS_APPEND() on
overlay inode, but overlay inode does not have the S_APPEND
flag and IS_APPEND() is always checked on backing inode in
other places.

Move the IS_APPEND() check to after we have the upperdentry
and use the d_backing_inode() macro to explicitly highlight
the places where the backing inode is used.

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

diff --git a/fs/open.c b/fs/open.c
index 0e3c12b..baf67cd 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -87,10 +87,6 @@ long vfs_truncate(const struct path *path, loff_t length)
 	if (error)
 		goto mnt_drop_write_and_out;
 
-	error = -EPERM;
-	if (IS_APPEND(inode))
-		goto mnt_drop_write_and_out;
-
 	/*
 	 * If this is an overlayfs then do as if opening the file so we get
 	 * write access on the upper inode, not on the overlay inode.  For
@@ -101,7 +97,11 @@ long vfs_truncate(const struct path *path, loff_t length)
 	if (IS_ERR(upperdentry))
 		goto mnt_drop_write_and_out;
 
-	error = get_write_access(upperdentry->d_inode);
+	error = -EPERM;
+	if (IS_APPEND(d_backing_inode(upperdentry)))
+		goto mnt_drop_write_and_out;
+
+	error = get_write_access(d_backing_inode(upperdentry));
 	if (error)
 		goto mnt_drop_write_and_out;
 
@@ -120,7 +120,7 @@ long vfs_truncate(const struct path *path, loff_t length)
 		error = do_truncate(path->dentry, length, 0, NULL);
 
 put_write_and_out:
-	put_write_access(upperdentry->d_inode);
+	put_write_access(d_backing_inode(upperdentry));
 mnt_drop_write_and_out:
 	mnt_drop_write(path->mnt);
 out:
-- 
2.7.4

[PATCH 4/4] vfs: open check IS_APPEND() on backing inode

[Amir Goldstein]

For overlay file open, check IS_APPEND() on real backing inode
inside vfs_open(), because the overlay inode does not have the
S_APPEND flag.

This seems like the wrong place for this check.
Probably better to propagate S_APPEND flag to overlay inode.

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

diff --git a/fs/open.c b/fs/open.c
index baf67cd..8f1754d 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -859,6 +859,15 @@ int vfs_open(const struct path *path, struct file *file,
 	if (IS_ERR(dentry))
 		return PTR_ERR(dentry);
 
+	/* Check append-only open flags also against real inode */
+	if (dentry != path->dentry && IS_APPEND(d_backing_inode(dentry))) {
+		if  ((file->f_flags & O_ACCMODE) != O_RDONLY &&
+		     !(file->f_flags & O_APPEND))
+			return -EPERM;
+		if (file->f_flags & O_TRUNC)
+			return -EPERM;
+	}
+
 	file->f_path = *path;
 	return do_dentry_open(file, d_backing_inode(dentry), NULL, cred);
 }
-- 
2.7.4