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

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

[Amir Goldstein]

Miklos,

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

I got rid of the wrong patch to convert sb_start_write() to
file_start_write() and moved the open flag checks into
ovl_d_real().

Amir.

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 (3):
  vfs: ftruncate check IS_APPEND() on real inode
  vfs: truncate check IS_APPEND() on real inode
  ovl: check IS_APPEND() on real upper inode

 fs/open.c            | 20 ++++++++++----------
 fs/overlayfs/super.c | 30 +++++++++++++++++++++++++++---
 2 files changed, 37 insertions(+), 13 deletions(-)

-- 
2.7.4

[PATCH v2 1/3] vfs: ftruncate check IS_APPEND() on real inode

[Amir Goldstein]

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

Set 'inode' var to file_inode() so all checks are performed on
the real inode and use the overlay inode/sb explicitly for
freeze protection and file lock checks.

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

diff --git a/fs/open.c b/fs/open.c
index 949cef2..b7d5ab1 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -182,7 +182,7 @@ static long do_sys_ftruncate(unsigned int fd, loff_t length, int small)
 		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;
@@ -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);
-	error = locks_verify_truncate(inode, f.file, length);
+	sb_start_write(dentry->d_sb);
+	error = locks_verify_truncate(d_inode(dentry), 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);
+	sb_end_write(dentry->d_sb);
 out_putf:
 	fdput(f);
 out:
-- 
2.7.4

[PATCH v2 2/3] vfs: truncate check IS_APPEND() on real inode

[Amir Goldstein]

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

Move the IS_APPEND() check to after we have the upperdentry
and pass it the real upper inode.

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 b7d5ab1..425db52 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_inode(upperdentry)))
+		goto mnt_drop_write_and_out;
+
+	error = get_write_access(d_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_inode(upperdentry));
 mnt_drop_write_and_out:
 	mnt_drop_write(path->mnt);
 out:
-- 
2.7.4

[PATCH v2 3/3] 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 | 30 +++++++++++++++++++++++++++---
 1 file changed, 27 insertions(+), 3 deletions(-)

diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index c9e70d3..13806b8 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -49,11 +49,30 @@ static void ovl_dentry_release(struct dentry *dentry)
 	}
 }
 
+static int ovl_may_open(struct dentry *dentry, int flag)
+{
+	struct inode *inode = dentry->d_inode;
+
+	/*
+	 * This test was moot in vfs may_open() because overlay inode does
+	 * not have the S_APPEND flag
+	 */
+	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 +84,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_may_open(real, open_flags);
+			if (err)
+				return ERR_PTR(err);
+		}
 		return real;
+	}
 
 	real = ovl_dentry_lower(dentry);
 	if (!real)
-- 
2.7.4

Re: [PATCH v2 1/3] vfs: ftruncate check IS_APPEND() on real inode

[Miklos Szeredi]

On Fri, Apr 7, 2017 at 11:01 AM, Amir Goldstein <amir73il@gmail.com> wrote:
> ftruncate an overlayfs inode was checking IS_APPEND() on
> overlay inode, but overlay inode does not have the S_APPEND flag.
>
> Set 'inode' var to file_inode() so all checks are performed on
> the real inode and use the overlay inode/sb explicitly for
> freeze protection and file lock checks.

If you'd just add the file_inode() to the IS_APPEND check then the
patch would look a lot less complicated and the effect should be
exactly the same.  Am I missing something?

Thanks,
Miklos

>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/open.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/fs/open.c b/fs/open.c
> index 949cef2..b7d5ab1 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -182,7 +182,7 @@ static long do_sys_ftruncate(unsigned int fd, loff_t length, int small)
>                 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;
> @@ -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);
> -       error = locks_verify_truncate(inode, f.file, length);
> +       sb_start_write(dentry->d_sb);
> +       error = locks_verify_truncate(d_inode(dentry), 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);
> +       sb_end_write(dentry->d_sb);
>  out_putf:
>         fdput(f);
>  out:
> --
> 2.7.4
>

Re: [PATCH v2 2/3] vfs: truncate check IS_APPEND() on real inode

[Miklos Szeredi]

On Fri, Apr 7, 2017 at 11:01 AM, Amir Goldstein <amir73il@gmail.com> wrote:
> truncate an overlayfs inode was checking IS_APPEND() on
> overlay inode, but overlay inode does not have the S_APPEND flag.
>
> Move the IS_APPEND() check to after we have the upperdentry
> and pass it the real upper inode.

Not sure the reordering is worth it.  Just use d_real_inode() in the
IS_APPEND() check.  OTOH it shouldn't matter (well, except whether the
file is copied up or not in the error caae ).  But mainly I just feel
when there's a choice of a simpler way we should use that.

Also it's usually better not to mix fixes with cleanups (the d_inode() thing).

Thanks,
Miklos

>
> 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 b7d5ab1..425db52 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_inode(upperdentry)))
> +               goto mnt_drop_write_and_out;
> +
> +       error = get_write_access(d_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_inode(upperdentry));
>  mnt_drop_write_and_out:
>         mnt_drop_write(path->mnt);
>  out:
> --
> 2.7.4
>

Re: [PATCH v2 3/3] ovl: check IS_APPEND() on real upper inode

[Miklos Szeredi]

On Fri, Apr 7, 2017 at 11:01 AM, Amir Goldstein <amir73il@gmail.com> wrote:
> 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.

Okay.   Maybe rename ovl_may_open() to ovl_check_append_only(),
because that what it does.

Thanks,
Miklos

>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/overlayfs/super.c | 30 +++++++++++++++++++++++++++---
>  1 file changed, 27 insertions(+), 3 deletions(-)
>
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index c9e70d3..13806b8 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -49,11 +49,30 @@ static void ovl_dentry_release(struct dentry *dentry)
>         }
>  }
>
> +static int ovl_may_open(struct dentry *dentry, int flag)
> +{
> +       struct inode *inode = dentry->d_inode;
> +
> +       /*
> +        * This test was moot in vfs may_open() because overlay inode does
> +        * not have the S_APPEND flag
> +        */
> +       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 +84,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_may_open(real, open_flags);
> +                       if (err)
> +                               return ERR_PTR(err);
> +               }
>                 return real;
> +       }
>
>         real = ovl_dentry_lower(dentry);
>         if (!real)
> --
> 2.7.4
>

Re: [PATCH v2 1/3] vfs: ftruncate check IS_APPEND() on real inode

[Amir Goldstein]

On Fri, Apr 7, 2017 at 4:10 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Fri, Apr 7, 2017 at 11:01 AM, Amir Goldstein <amir73il@gmail.com> wrote:
>> ftruncate an overlayfs inode was checking IS_APPEND() on
>> overlay inode, but overlay inode does not have the S_APPEND flag.
>>
>> Set 'inode' var to file_inode() so all checks are performed on
>> the real inode and use the overlay inode/sb explicitly for
>> freeze protection and file lock checks.
>
> If you'd just add the file_inode() to the IS_APPEND check then the
> patch would look a lot less complicated and the effect should be
> exactly the same.  Am I missing something?
>

Technically you are right.
Social engineering wise, its quite hard to look at a random
vfs function and say what 'inode' stands.

I surveyed some functions and found that in most cases
where file->f_inode is set a local var named inode stands
for file_inode() and locks_inode() is the exception, so this
patch brings do_sys_ftruncate() into compliance.


> Thanks,
> Miklos
>
>>
>> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>> ---
>>  fs/open.c | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/open.c b/fs/open.c
>> index 949cef2..b7d5ab1 100644
>> --- a/fs/open.c
>> +++ b/fs/open.c
>> @@ -182,7 +182,7 @@ static long do_sys_ftruncate(unsigned int fd, loff_t length, int small)
>>                 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;
>> @@ -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);
>> -       error = locks_verify_truncate(inode, f.file, length);
>> +       sb_start_write(dentry->d_sb);
>> +       error = locks_verify_truncate(d_inode(dentry), 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);
>> +       sb_end_write(dentry->d_sb);
>>  out_putf:
>>         fdput(f);
>>  out:
>> --
>> 2.7.4
>>

Re: [PATCH v2 2/3] vfs: truncate check IS_APPEND() on real inode

[Amir Goldstein]

On Fri, Apr 7, 2017 at 4:21 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Fri, Apr 7, 2017 at 11:01 AM, Amir Goldstein <amir73il@gmail.com> wrote:
>> truncate an overlayfs inode was checking IS_APPEND() on
>> overlay inode, but overlay inode does not have the S_APPEND flag.
>>
>> Move the IS_APPEND() check to after we have the upperdentry
>> and pass it the real upper inode.
>
> Not sure the reordering is worth it.  Just use d_real_inode() in the

Hmm, I meant move after d_real() which actually does a copy up.
If we use d_real_inode() before d_real() then we prevent truncate
of a lower with chattr +a.
That is not consistent at all with open(O_TRUNC) and ftruncate
and with checks for IS_IMMUTABLE() which only test upper
(and I think it is better this way).

> IS_APPEND() check.  OTOH it shouldn't matter (well, except whether the
> file is copied up or not in the error caae ).  But mainly I just feel
> when there's a choice of a simpler way we should use that.
>
> Also it's usually better not to mix fixes with cleanups (the d_inode() thing).
>

Sure, I can split that or drop the cleanup altogether.

> Thanks,
> Miklos
>
>>
>> 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 b7d5ab1..425db52 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_inode(upperdentry)))
>> +               goto mnt_drop_write_and_out;
>> +
>> +       error = get_write_access(d_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_inode(upperdentry));
>>  mnt_drop_write_and_out:
>>         mnt_drop_write(path->mnt);
>>  out:
>> --
>> 2.7.4
>>

Re: [PATCH v2 3/3] ovl: check IS_APPEND() on real upper inode

[Amir Goldstein]

On Fri, Apr 7, 2017 at 4:33 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Fri, Apr 7, 2017 at 11:01 AM, Amir Goldstein <amir73il@gmail.com> wrote:
>> 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.
>
> Okay.   Maybe rename ovl_may_open() to ovl_check_append_only(),
> because that what it does.

Agree. Also I can pass the inode instead of dentry.

>
> Thanks,
> Miklos
>
>>
>> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>> ---
>>  fs/overlayfs/super.c | 30 +++++++++++++++++++++++++++---
>>  1 file changed, 27 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
>> index c9e70d3..13806b8 100644
>> --- a/fs/overlayfs/super.c
>> +++ b/fs/overlayfs/super.c
>> @@ -49,11 +49,30 @@ static void ovl_dentry_release(struct dentry *dentry)
>>         }
>>  }
>>
>> +static int ovl_may_open(struct dentry *dentry, int flag)
>> +{
>> +       struct inode *inode = dentry->d_inode;
>> +
>> +       /*
>> +        * This test was moot in vfs may_open() because overlay inode does
>> +        * not have the S_APPEND flag
>> +        */
>> +       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 +84,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_may_open(real, open_flags);
>> +                       if (err)
>> +                               return ERR_PTR(err);
>> +               }
>>                 return real;
>> +       }
>>
>>         real = ovl_dentry_lower(dentry);
>>         if (!real)
>> --
>> 2.7.4
>>

Re: [PATCH v2 2/3] vfs: truncate check IS_APPEND() on real inode

[Amir Goldstein]

On Fri, Apr 7, 2017 at 7:16 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Fri, Apr 7, 2017 at 4:21 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>> On Fri, Apr 7, 2017 at 11:01 AM, Amir Goldstein <amir73il@gmail.com> wrote:
>>> truncate an overlayfs inode was checking IS_APPEND() on
>>> overlay inode, but overlay inode does not have the S_APPEND flag.
>>>
>>> Move the IS_APPEND() check to after we have the upperdentry
>>> and pass it the real upper inode.
>>
>> Not sure the reordering is worth it.  Just use d_real_inode() in the
>
> Hmm, I meant move after d_real() which actually does a copy up.
> If we use d_real_inode() before d_real() then we prevent truncate
> of a lower with chattr +a.
> That is not consistent at all with open(O_TRUNC) and ftruncate
> and with checks for IS_IMMUTABLE() which only test upper
> (and I think it is better this way).
>
>> IS_APPEND() check.  OTOH it shouldn't matter (well, except whether the
>> file is copied up or not in the error caae ).  But mainly I just feel
>> when there's a choice of a simpler way we should use that.
>>
>> Also it's usually better not to mix fixes with cleanups (the d_inode() thing).
>>
>
> Sure, I can split that or drop the cleanup altogether.
>

Or drop the patch altogether... d_real() already takes care of IS_APPEND()
now :)