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

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

[Amir Goldstein]

Miklos,

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

I addressed your comments on v2 and removed the patch to vfs_truncate()
because d_real() already takes cares of IS_APPEND().

Amir.

v3:
- address comments by Miklos
- discard unneeded patch to vfs_truncate()

v2:
- move open check to ovl_d_real()
- discard wrong patch to file_start_write() in ftruncate()

v1:
- fix checks in [f]truncate and vfs_open()

Amir Goldstein (2):
  vfs: ftruncate check IS_APPEND() on real upper inode
  ovl: check IS_APPEND() on real upper inode

 fs/open.c            |  3 ++-
 fs/overlayfs/super.c | 28 +++++++++++++++++++++++++---
 2 files changed, 27 insertions(+), 4 deletions(-)

-- 
2.7.4

[PATCH v3 1/2] vfs: ftruncate check IS_APPEND() on real upper inode

[Amir Goldstein]

ftruncate an overlayfs inode was checking IS_APPEND() on
overlay inode, but overlay inode does not have the S_APPEND flag.

Check IS_APPEND() on real upper inode instead.

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

diff --git a/fs/open.c b/fs/open.c
index 949cef2..993a91d 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -193,7 +193,8 @@ static long do_sys_ftruncate(unsigned int fd, loff_t length, int small)
 		goto out_putf;
 
 	error = -EPERM;
-	if (IS_APPEND(inode))
+	/* Check IS_APPEND on real upper inode */
+	if (IS_APPEND(file_inode(f.file)))
 		goto out_putf;
 
 	sb_start_write(inode->i_sb);
-- 
2.7.4

[PATCH v3 2/2] ovl: check IS_APPEND() on real upper inode

[Amir Goldstein]

For overlay file open, check IS_APPEND() on the real upper inode
inside d_real(), because the overlay inode does not have the
S_APPEND flag and IS_APPEND() can only be checked at open time.

Note that because overlayfs does not copy up the chattr inode flags
(i.e. S_APPEND, S_IMMUTABLE), the IS_APPEND() check is only relevant
for upper inodes that were set with chattr +a and not to lower
inodes that had chattr +a before copy up.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/super.c | 28 +++++++++++++++++++++++++---
 1 file changed, 25 insertions(+), 3 deletions(-)

diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index c9e70d3..c072a0c 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -49,11 +49,28 @@ static void ovl_dentry_release(struct dentry *dentry)
 	}
 }
 
+static int ovl_check_append_only(struct inode *inode, int flag)
+{
+	/*
+	 * This test was moot in vfs may_open() because overlay inode does
+	 * not have the S_APPEND flag, so re-check on real upper inode
+	 */
+	if (IS_APPEND(inode)) {
+		if  ((flag & O_ACCMODE) != O_RDONLY && !(flag & O_APPEND))
+			return -EPERM;
+		if (flag & O_TRUNC)
+			return -EPERM;
+	}
+
+	return 0;
+}
+
 static struct dentry *ovl_d_real(struct dentry *dentry,
 				 const struct inode *inode,
 				 unsigned int open_flags)
 {
 	struct dentry *real;
+	int err;
 
 	if (!d_is_reg(dentry)) {
 		if (!inode || inode == d_inode(dentry))
@@ -65,15 +82,20 @@ static struct dentry *ovl_d_real(struct dentry *dentry,
 		return dentry;
 
 	if (open_flags) {
-		int err = ovl_open_maybe_copy_up(dentry, open_flags);
-
+		err = ovl_open_maybe_copy_up(dentry, open_flags);
 		if (err)
 			return ERR_PTR(err);
 	}
 
 	real = ovl_dentry_upper(dentry);
-	if (real && (!inode || inode == d_inode(real)))
+	if (real && (!inode || inode == d_inode(real))) {
+		if (!inode) {
+			err = ovl_check_append_only(d_inode(real), open_flags);
+			if (err)
+				return ERR_PTR(err);
+		}
 		return real;
+	}
 
 	real = ovl_dentry_lower(dentry);
 	if (!real)
-- 
2.7.4

Re: [PATCH v3 0/2] ovl: fix IS_APPEND() checks

[Amir Goldstein]

On Sat, Apr 8, 2017 at 2:49 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> Miklos,
>
> This series fixes the append-only violations found by the new
> xfstest overlay/030.
>
> I addressed your comments on v2 and removed the patch to vfs_truncate()
> because d_real() already takes cares of IS_APPEND().
>

Miklos,

I know I bombed you with patches to review and
you are probably still catching up on your emails.

Since 4.11 release is around the corner, wanted to
ping you on these fix patches from 2 weeks ago,
so they have some time to soak in -next.

V3 turned out to be quite simple and not controversial IMO.

Thanks,
Amir.

> Amir.
>
> v3:
> - address comments by Miklos
> - discard unneeded patch to vfs_truncate()
>
> v2:
> - move open check to ovl_d_real()
> - discard wrong patch to file_start_write() in ftruncate()
>
> v1:
> - fix checks in [f]truncate and vfs_open()
>
> Amir Goldstein (2):
>   vfs: ftruncate check IS_APPEND() on real upper inode
>   ovl: check IS_APPEND() on real upper inode
>
>  fs/open.c            |  3 ++-
>  fs/overlayfs/super.c | 28 +++++++++++++++++++++++++---
>  2 files changed, 27 insertions(+), 4 deletions(-)
>
> --
> 2.7.4
>

Re: [PATCH v3 0/2] ovl: fix IS_APPEND() checks

[Miklos Szeredi]

On Thu, Apr 20, 2017 at 2:19 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Sat, Apr 8, 2017 at 2:49 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>> Miklos,
>>
>> This series fixes the append-only violations found by the new
>> xfstest overlay/030.
>>
>> I addressed your comments on v2 and removed the patch to vfs_truncate()
>> because d_real() already takes cares of IS_APPEND().

Pushed.

Thanks,
Miklos