[RFC PATCH 1/2] ovl: skip checking lower file's write permisson on truncate

[RFC PATCH 1/2] ovl: skip checking lower file's write permisson on truncate

[Chengguang Xu]

Lower files may be shared in overlayfs so strictly checking write
perssmion on lower file will cause interferes between different
overlayfs instances.

Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
---
 fs/overlayfs/inode.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index 28c71978eb2e..17d1add0af1a 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -31,12 +31,6 @@ int ovl_setattr(struct user_namespace *mnt_userns, struct dentry *dentry,
 		goto out;
 
 	if (attr->ia_valid & ATTR_SIZE) {
-		struct inode *realinode = d_inode(ovl_dentry_real(dentry));
-
-		err = -ETXTBSY;
-		if (atomic_read(&realinode->i_writecount) < 0)
-			goto out_drop_write;
-
 		/* Truncate should trigger data copy up as well */
 		full_copy_up = true;
 	}
-- 
2.27.0

[RFC PATCH 2/2] ovl: enhance write permission check for writable open

[Chengguang Xu]

Check upper file's write permission when open on writable mode.

NOTE: lower files may be shared between differnt overlayfs instances,
so we skip the check of lower file to avoid introducing interferes.

Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
---
 fs/overlayfs/file.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
index 6e454a294046..1c3c24d07d01 100644
--- a/fs/overlayfs/file.c
+++ b/fs/overlayfs/file.c
@@ -144,12 +144,18 @@ static int ovl_real_fdget(const struct file *file, struct fd *real)
 static int ovl_open(struct inode *inode, struct file *file)
 {
 	struct file *realfile;
+	struct inode *upperinode;
 	int err;
 
 	err = ovl_maybe_copy_up(file_dentry(file), file->f_flags);
 	if (err)
 		return err;
 
+	upperinode = ovl_inode_upper(inode);
+	if (((file->f_mode & FMODE_WRITE) || file->f_flags & O_TRUNC) &&
+	    (upperinode && atomic_read(&upperinode->i_writecount) < 0))
+		return -ETXTBSY;
+
 	/* No longer need these flags, so don't pass them on to underlying fs */
 	file->f_flags &= ~(O_CREAT | O_EXCL | O_NOCTTY | O_TRUNC);
 
-- 
2.27.0

回复:[RFC PATCH 1/2] ovl: skip checking lower file's write permisson on truncate

[Chengguang Xu]

 ---- 在 星期六, 2021-04-24 22:03:15 Chengguang Xu <cgxu519@mykernel.net> 撰写 ----
 > Lower files may be shared in overlayfs so strictly checking write
 > perssmion on lower file will cause interferes between different
 > overlayfs instances.

Any comment for this?

Thanks,
Chengguang



 > 
 > Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
 > ---
 >  fs/overlayfs/inode.c | 6 ------
 >  1 file changed, 6 deletions(-)
 > 
 > diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
 > index 28c71978eb2e..17d1add0af1a 100644
 > --- a/fs/overlayfs/inode.c
 > +++ b/fs/overlayfs/inode.c
 > @@ -31,12 +31,6 @@ int ovl_setattr(struct user_namespace *mnt_userns, struct dentry *dentry,
 >          goto out;
 >  
 >      if (attr->ia_valid & ATTR_SIZE) {
 > -        struct inode *realinode = d_inode(ovl_dentry_real(dentry));
 > -
 > -        err = -ETXTBSY;
 > -        if (atomic_read(&realinode->i_writecount) < 0)
 > -            goto out_drop_write;
 > -
 >          /* Truncate should trigger data copy up as well */
 >          full_copy_up = true;
 >      }
 > -- 
 > 2.27.0
 > 
 > 
 > 

Re: [RFC PATCH 1/2] ovl: skip checking lower file's write permisson on truncate

[Miklos Szeredi]

On Sat, 24 Apr 2021 at 16:04, Chengguang Xu <cgxu519@mykernel.net> wrote:
>
> Lower files may be shared in overlayfs so strictly checking write
> perssmion on lower file will cause interferes between different
> overlayfs instances.

How so?

i_writecount on lower inode is not modified by overlayfs (at least not
in this codepath).  Which means that there should be no interference
between overlayfs instances sharing a lower directory tree.

Thanks,
Miklos



>
> Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
> ---
>  fs/overlayfs/inode.c | 6 ------
>  1 file changed, 6 deletions(-)
>
> diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> index 28c71978eb2e..17d1add0af1a 100644
> --- a/fs/overlayfs/inode.c
> +++ b/fs/overlayfs/inode.c
> @@ -31,12 +31,6 @@ int ovl_setattr(struct user_namespace *mnt_userns, struct dentry *dentry,
>                 goto out;
>
>         if (attr->ia_valid & ATTR_SIZE) {
> -               struct inode *realinode = d_inode(ovl_dentry_real(dentry));
> -
> -               err = -ETXTBSY;
> -               if (atomic_read(&realinode->i_writecount) < 0)
> -                       goto out_drop_write;
> -
>                 /* Truncate should trigger data copy up as well */
>                 full_copy_up = true;
>         }
> --
> 2.27.0
>
>

Re: [RFC PATCH 1/2] ovl: skip checking lower file's write permisson on truncate

[Miklos Szeredi]

On Tue, 20 Jul 2021 at 16:35, Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Sat, 24 Apr 2021 at 16:04, Chengguang Xu <cgxu519@mykernel.net> wrote:
> >
> > Lower files may be shared in overlayfs so strictly checking write
> > perssmion on lower file will cause interferes between different
> > overlayfs instances.
>
> How so?
>
> i_writecount on lower inode is not modified by overlayfs (at least not
> in this codepath).  Which means that there should be no interference
> between overlayfs instances sharing a lower directory tree.

I'm beginning to see what you are worrying about.

So on one instance a file on lower gets executed and on another
instance sharing the lower layer the file is truncated.  The truncate
is currently denied due to the negative i_writecount on the lower
file.  Also behavior is inconsistent between open(path, O_TRUNC) and
truncate(path) even though the two should be equivalent.

Applied with the following description:

It is possible that a directory tree is shared between multiple overlay
instances as a lower layer.  In this case when one instance executes a file
residing on the lower layer, the other instance denies a truncate(2) call
on this file.

This only happens for truncate(2) and not for open(2) with the O_TRUNC
flag.

Fix this interference and inconsistency by removing the preliminary
i_writecount check before copy-up.

This means that unlike on normal filesystems truncate(argv[0]) will now
succeed.  If this ever causes a regression in a real world use case this
needs to be revisited.

One way to fix this properly would be to keep a correct i_writecount in the
overlay inode, but that is difficult due to memory mapping code only
dealing with the real file/inode.

Thanks,
Miklos

Re: [RFC PATCH 1/2] ovl: skip checking lower file's write permisson on truncate

[Chengguang Xu]

 ---- 在 星期二, 2021-07-20 23:19:16 Miklos Szeredi <miklos@szeredi.hu> 撰写 ----
 > On Tue, 20 Jul 2021 at 16:35, Miklos Szeredi <miklos@szeredi.hu> wrote:
 > >
 > > On Sat, 24 Apr 2021 at 16:04, Chengguang Xu <cgxu519@mykernel.net> wrote:
 > > >
 > > > Lower files may be shared in overlayfs so strictly checking write
 > > > perssmion on lower file will cause interferes between different
 > > > overlayfs instances.
 > >
 > > How so?
 > >
 > > i_writecount on lower inode is not modified by overlayfs (at least not
 > > in this codepath).  Which means that there should be no interference
 > > between overlayfs instances sharing a lower directory tree.
 > 
 > I'm beginning to see what you are worrying about.
 > 
 > So on one instance a file on lower gets executed and on another
 > instance sharing the lower layer the file is truncated.  The truncate
 > is currently denied due to the negative i_writecount on the lower
 > file.  Also behavior is inconsistent between open(path, O_TRUNC) and
 > truncate(path) even though the two should be equivalent.

Yeah, that's it.
Thanks for applying the patch and supplementary description.

Thanks,
Chengguang

Re: [RFC PATCH 1/2] ovl: skip checking lower file's write permisson on truncate

[Miklos Szeredi]

On Tue, 20 Jul 2021 at 17:19, Miklos Szeredi <miklos@szeredi.hu> wrote:

> So on one instance a file on lower gets executed and on another
> instance sharing the lower layer the file is truncated.  The truncate
> is currently denied due to the negative i_writecount on the lower
> file.  Also behavior is inconsistent between open(path, O_TRUNC) and
> truncate(path) even though the two should be equivalent.
>
> Applied with the following description:
>
[...]

Also adding the following documentation in the "Non-standard behavior" section:

c) If a file residing on a lower layer is being executed, then opening that
file for write or truncating the file will not be denied with ETXTBSY.

Looked at the POSIX standard and it only documents ETXTBUSY for O_RDWR
and O_WRONLY and not for truncate(2) or O_TRUNC.  So strictly speaking
this patch doesn't even change the POSIX correctness.

Thanks,
Miklos

Re: [RFC PATCH 1/2] ovl: skip checking lower file's write permisson on truncate

[Chengguang Xu]

 ---- 在 星期三, 2021-07-21 00:01:11 Miklos Szeredi <miklos@szeredi.hu> 撰写 ----
 > On Tue, 20 Jul 2021 at 17:19, Miklos Szeredi <miklos@szeredi.hu> wrote:
 > 
 > > So on one instance a file on lower gets executed and on another
 > > instance sharing the lower layer the file is truncated.  The truncate
 > > is currently denied due to the negative i_writecount on the lower
 > > file.  Also behavior is inconsistent between open(path, O_TRUNC) and
 > > truncate(path) even though the two should be equivalent.
 > >
 > > Applied with the following description:
 > >
 > [...]
 > 
 > Also adding the following documentation in the "Non-standard behavior" section:
 > 
 > c) If a file residing on a lower layer is being executed, then opening that
 > file for write or truncating the file will not be denied with ETXTBSY.
 > 
 > Looked at the POSIX standard and it only documents ETXTBUSY for O_RDWR
 > and O_WRONLY and not for truncate(2) or O_TRUNC.  So strictly speaking
 > this patch doesn't even change the POSIX correctness.
 > 

Hi Miklos,

Thanks for doing this too.

Thanks,
Chengguang

Re: [RFC PATCH 2/2] ovl: enhance write permission check for writable open

[Miklos Szeredi]

On Sat, 24 Apr 2021 at 16:04, Chengguang Xu <cgxu519@mykernel.net> wrote:
>
> Check upper file's write permission when open on writable mode.

This should already be done in ovl_open() -> ovl_open_realfile() ->
open_with_fake_path() -> do_dentry_open().

Do you have a test case indicating that the writecount test is not working?

Thanks,
Miklos

>
> NOTE: lower files may be shared between differnt overlayfs instances,
> so we skip the check of lower file to avoid introducing interferes.
>
> Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
> ---
>  fs/overlayfs/file.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
> index 6e454a294046..1c3c24d07d01 100644
> --- a/fs/overlayfs/file.c
> +++ b/fs/overlayfs/file.c
> @@ -144,12 +144,18 @@ static int ovl_real_fdget(const struct file *file, struct fd *real)
>  static int ovl_open(struct inode *inode, struct file *file)
>  {
>         struct file *realfile;
> +       struct inode *upperinode;
>         int err;
>
>         err = ovl_maybe_copy_up(file_dentry(file), file->f_flags);
>         if (err)
>                 return err;
>
> +       upperinode = ovl_inode_upper(inode);
> +       if (((file->f_mode & FMODE_WRITE) || file->f_flags & O_TRUNC) &&
> +           (upperinode && atomic_read(&upperinode->i_writecount) < 0))
> +               return -ETXTBSY;
> +
>         /* No longer need these flags, so don't pass them on to underlying fs */
>         file->f_flags &= ~(O_CREAT | O_EXCL | O_NOCTTY | O_TRUNC);
>
> --
> 2.27.0
>
>