linux-unionfs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFC PATCH 1/2] ovl: skip checking lower file's write permisson on truncate
@ 2021-04-24 14:03 Chengguang Xu
  2021-04-24 14:03 ` [RFC PATCH 2/2] ovl: enhance write permission check for writable open Chengguang Xu
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Chengguang Xu @ 2021-04-24 14:03 UTC (permalink / raw)
  To: miklos; +Cc: linux-unionfs, 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



^ permalink raw reply	[flat|nested] 9+ messages in thread

* [RFC PATCH 2/2] ovl: enhance write permission check for writable open
  2021-04-24 14:03 [RFC PATCH 1/2] ovl: skip checking lower file's write permisson on truncate Chengguang Xu
@ 2021-04-24 14:03 ` Chengguang Xu
  2021-07-21 13:14   ` Miklos Szeredi
  2021-04-28 12:18 ` 回复:[RFC PATCH 1/2] ovl: skip checking lower file's write permisson on truncate Chengguang Xu
  2021-07-20 14:35 ` [RFC " Miklos Szeredi
  2 siblings, 1 reply; 9+ messages in thread
From: Chengguang Xu @ 2021-04-24 14:03 UTC (permalink / raw)
  To: miklos; +Cc: linux-unionfs, 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



^ permalink raw reply	[flat|nested] 9+ messages in thread

* 回复:[RFC PATCH 1/2] ovl: skip checking lower file's write permisson on truncate
  2021-04-24 14:03 [RFC PATCH 1/2] ovl: skip checking lower file's write permisson on truncate Chengguang Xu
  2021-04-24 14:03 ` [RFC PATCH 2/2] ovl: enhance write permission check for writable open Chengguang Xu
@ 2021-04-28 12:18 ` Chengguang Xu
  2021-07-20 14:35 ` [RFC " Miklos Szeredi
  2 siblings, 0 replies; 9+ messages in thread
From: Chengguang Xu @ 2021-04-28 12:18 UTC (permalink / raw)
  To: Chengguang Xu; +Cc: miklos, linux-unionfs

 ---- 在 星期六, 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
 > 
 > 
 > 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC PATCH 1/2] ovl: skip checking lower file's write permisson on truncate
  2021-04-24 14:03 [RFC PATCH 1/2] ovl: skip checking lower file's write permisson on truncate Chengguang Xu
  2021-04-24 14:03 ` [RFC PATCH 2/2] ovl: enhance write permission check for writable open Chengguang Xu
  2021-04-28 12:18 ` 回复:[RFC PATCH 1/2] ovl: skip checking lower file's write permisson on truncate Chengguang Xu
@ 2021-07-20 14:35 ` Miklos Szeredi
  2021-07-20 15:19   ` Miklos Szeredi
  2 siblings, 1 reply; 9+ messages in thread
From: Miklos Szeredi @ 2021-07-20 14:35 UTC (permalink / raw)
  To: Chengguang Xu; +Cc: overlayfs

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
>
>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC PATCH 1/2] ovl: skip checking lower file's write permisson on truncate
  2021-07-20 14:35 ` [RFC " Miklos Szeredi
@ 2021-07-20 15:19   ` Miklos Szeredi
  2021-07-20 16:01     ` Chengguang Xu
  2021-07-20 16:01     ` Miklos Szeredi
  0 siblings, 2 replies; 9+ messages in thread
From: Miklos Szeredi @ 2021-07-20 15:19 UTC (permalink / raw)
  To: Chengguang Xu; +Cc: overlayfs

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC PATCH 1/2] ovl: skip checking lower file's write permisson on truncate
  2021-07-20 15:19   ` Miklos Szeredi
@ 2021-07-20 16:01     ` Chengguang Xu
  2021-07-20 16:01     ` Miklos Szeredi
  1 sibling, 0 replies; 9+ messages in thread
From: Chengguang Xu @ 2021-07-20 16:01 UTC (permalink / raw)
  To: Miklos Szeredi; +Cc: overlayfs

 ---- 在 星期二, 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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC PATCH 1/2] ovl: skip checking lower file's write permisson on truncate
  2021-07-20 15:19   ` Miklos Szeredi
  2021-07-20 16:01     ` Chengguang Xu
@ 2021-07-20 16:01     ` Miklos Szeredi
  2021-07-20 16:04       ` Chengguang Xu
  1 sibling, 1 reply; 9+ messages in thread
From: Miklos Szeredi @ 2021-07-20 16:01 UTC (permalink / raw)
  To: Chengguang Xu; +Cc: overlayfs

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC PATCH 1/2] ovl: skip checking lower file's write permisson on truncate
  2021-07-20 16:01     ` Miklos Szeredi
@ 2021-07-20 16:04       ` Chengguang Xu
  0 siblings, 0 replies; 9+ messages in thread
From: Chengguang Xu @ 2021-07-20 16:04 UTC (permalink / raw)
  To: Miklos Szeredi; +Cc: overlayfs

 ---- 在 星期三, 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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC PATCH 2/2] ovl: enhance write permission check for writable open
  2021-04-24 14:03 ` [RFC PATCH 2/2] ovl: enhance write permission check for writable open Chengguang Xu
@ 2021-07-21 13:14   ` Miklos Szeredi
  0 siblings, 0 replies; 9+ messages in thread
From: Miklos Szeredi @ 2021-07-21 13:14 UTC (permalink / raw)
  To: Chengguang Xu; +Cc: overlayfs

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
>
>

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2021-07-21 13:14 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-24 14:03 [RFC PATCH 1/2] ovl: skip checking lower file's write permisson on truncate Chengguang Xu
2021-04-24 14:03 ` [RFC PATCH 2/2] ovl: enhance write permission check for writable open Chengguang Xu
2021-07-21 13:14   ` Miklos Szeredi
2021-04-28 12:18 ` 回复:[RFC PATCH 1/2] ovl: skip checking lower file's write permisson on truncate Chengguang Xu
2021-07-20 14:35 ` [RFC " Miklos Szeredi
2021-07-20 15:19   ` Miklos Szeredi
2021-07-20 16:01     ` Chengguang Xu
2021-07-20 16:01     ` Miklos Szeredi
2021-07-20 16:04       ` Chengguang Xu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).