* [PATCH v2 0/3] ovl: fix IS_APPEND() checks
@ 2017-04-07 9:01 Amir Goldstein
2017-04-07 9:01 ` [PATCH v2 1/3] vfs: ftruncate check IS_APPEND() on real inode Amir Goldstein
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Amir Goldstein @ 2017-04-07 9:01 UTC (permalink / raw)
To: Miklos Szeredi; +Cc: Al Viro, linux-unionfs, linux-fsdevel
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 1/3] vfs: ftruncate check IS_APPEND() on real inode
2017-04-07 9:01 [PATCH v2 0/3] ovl: fix IS_APPEND() checks Amir Goldstein
@ 2017-04-07 9:01 ` Amir Goldstein
2017-04-07 13:10 ` Miklos Szeredi
2017-04-07 9:01 ` [PATCH v2 2/3] vfs: truncate " Amir Goldstein
2017-04-07 9:01 ` [PATCH v2 3/3] ovl: check IS_APPEND() on real upper inode Amir Goldstein
2 siblings, 1 reply; 11+ messages in thread
From: Amir Goldstein @ 2017-04-07 9:01 UTC (permalink / raw)
To: Miklos Szeredi; +Cc: Al Viro, linux-unionfs, linux-fsdevel
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2 2/3] vfs: truncate check IS_APPEND() on real inode
2017-04-07 9:01 [PATCH v2 0/3] ovl: fix IS_APPEND() checks Amir Goldstein
2017-04-07 9:01 ` [PATCH v2 1/3] vfs: ftruncate check IS_APPEND() on real inode Amir Goldstein
@ 2017-04-07 9:01 ` Amir Goldstein
2017-04-07 13:21 ` Miklos Szeredi
2017-04-07 9:01 ` [PATCH v2 3/3] ovl: check IS_APPEND() on real upper inode Amir Goldstein
2 siblings, 1 reply; 11+ messages in thread
From: Amir Goldstein @ 2017-04-07 9:01 UTC (permalink / raw)
To: Miklos Szeredi; +Cc: Al Viro, linux-unionfs, linux-fsdevel
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2 3/3] ovl: check IS_APPEND() on real upper inode
2017-04-07 9:01 [PATCH v2 0/3] ovl: fix IS_APPEND() checks Amir Goldstein
2017-04-07 9:01 ` [PATCH v2 1/3] vfs: ftruncate check IS_APPEND() on real inode Amir Goldstein
2017-04-07 9:01 ` [PATCH v2 2/3] vfs: truncate " Amir Goldstein
@ 2017-04-07 9:01 ` Amir Goldstein
2017-04-07 13:33 ` Miklos Szeredi
2 siblings, 1 reply; 11+ messages in thread
From: Amir Goldstein @ 2017-04-07 9:01 UTC (permalink / raw)
To: Miklos Szeredi; +Cc: Al Viro, linux-unionfs, linux-fsdevel
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/3] vfs: ftruncate check IS_APPEND() on real inode
2017-04-07 9:01 ` [PATCH v2 1/3] vfs: ftruncate check IS_APPEND() on real inode Amir Goldstein
@ 2017-04-07 13:10 ` Miklos Szeredi
2017-04-07 16:03 ` Amir Goldstein
0 siblings, 1 reply; 11+ messages in thread
From: Miklos Szeredi @ 2017-04-07 13:10 UTC (permalink / raw)
To: Amir Goldstein; +Cc: Al Viro, linux-unionfs, linux-fsdevel
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
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/3] vfs: truncate check IS_APPEND() on real inode
2017-04-07 9:01 ` [PATCH v2 2/3] vfs: truncate " Amir Goldstein
@ 2017-04-07 13:21 ` Miklos Szeredi
2017-04-07 16:16 ` Amir Goldstein
0 siblings, 1 reply; 11+ messages in thread
From: Miklos Szeredi @ 2017-04-07 13:21 UTC (permalink / raw)
To: Amir Goldstein; +Cc: Al Viro, linux-unionfs, linux-fsdevel
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
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 3/3] ovl: check IS_APPEND() on real upper inode
2017-04-07 9:01 ` [PATCH v2 3/3] ovl: check IS_APPEND() on real upper inode Amir Goldstein
@ 2017-04-07 13:33 ` Miklos Szeredi
2017-04-07 16:19 ` Amir Goldstein
0 siblings, 1 reply; 11+ messages in thread
From: Miklos Szeredi @ 2017-04-07 13:33 UTC (permalink / raw)
To: Amir Goldstein; +Cc: Al Viro, linux-unionfs, linux-fsdevel
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
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/3] vfs: ftruncate check IS_APPEND() on real inode
2017-04-07 13:10 ` Miklos Szeredi
@ 2017-04-07 16:03 ` Amir Goldstein
0 siblings, 0 replies; 11+ messages in thread
From: Amir Goldstein @ 2017-04-07 16:03 UTC (permalink / raw)
To: Miklos Szeredi; +Cc: Al Viro, linux-unionfs, linux-fsdevel
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
>>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/3] vfs: truncate check IS_APPEND() on real inode
2017-04-07 13:21 ` Miklos Szeredi
@ 2017-04-07 16:16 ` Amir Goldstein
2017-04-08 11:50 ` Amir Goldstein
0 siblings, 1 reply; 11+ messages in thread
From: Amir Goldstein @ 2017-04-07 16:16 UTC (permalink / raw)
To: Miklos Szeredi; +Cc: Al Viro, linux-unionfs, linux-fsdevel
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
>>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 3/3] ovl: check IS_APPEND() on real upper inode
2017-04-07 13:33 ` Miklos Szeredi
@ 2017-04-07 16:19 ` Amir Goldstein
0 siblings, 0 replies; 11+ messages in thread
From: Amir Goldstein @ 2017-04-07 16:19 UTC (permalink / raw)
To: Miklos Szeredi; +Cc: Al Viro, linux-unionfs, linux-fsdevel
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
>>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/3] vfs: truncate check IS_APPEND() on real inode
2017-04-07 16:16 ` Amir Goldstein
@ 2017-04-08 11:50 ` Amir Goldstein
0 siblings, 0 replies; 11+ messages in thread
From: Amir Goldstein @ 2017-04-08 11:50 UTC (permalink / raw)
To: Miklos Szeredi; +Cc: Al Viro, linux-unionfs, linux-fsdevel
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 :)
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2017-04-08 11:50 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-07 9:01 [PATCH v2 0/3] ovl: fix IS_APPEND() checks Amir Goldstein
2017-04-07 9:01 ` [PATCH v2 1/3] vfs: ftruncate check IS_APPEND() on real inode Amir Goldstein
2017-04-07 13:10 ` Miklos Szeredi
2017-04-07 16:03 ` Amir Goldstein
2017-04-07 9:01 ` [PATCH v2 2/3] vfs: truncate " Amir Goldstein
2017-04-07 13:21 ` Miklos Szeredi
2017-04-07 16:16 ` Amir Goldstein
2017-04-08 11:50 ` Amir Goldstein
2017-04-07 9:01 ` [PATCH v2 3/3] ovl: check IS_APPEND() on real upper inode Amir Goldstein
2017-04-07 13:33 ` Miklos Szeredi
2017-04-07 16:19 ` Amir Goldstein
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.