linux-unionfs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] overlayfs, doc: Do not allow lower layer recreation with redirect_dir enabled
@ 2020-07-09 14:02 Vivek Goyal
  2020-07-09 14:14 ` Vivek Goyal
  0 siblings, 1 reply; 4+ messages in thread
From: Vivek Goyal @ 2020-07-09 14:02 UTC (permalink / raw)
  To: Amir Goldstein, Miklos Szeredi; +Cc: linux-unionfs

Currently we seem to support lower layer recreation and re-use with existing
upper until and unless "index" or "metadata only copy up" feature is
enabled.

If redirect_dir feature is enabled then re-creating/modifying lower layers
will break things. For example.

- mkdir lower lower/foo upper work merged
- touch lower/foo/foo-child
- mount -t overlay -o lowerdir=lower,upperdir=upper,workdir=work,redirect_dir=on none merged
- mv merged/foo merged/bar
- ls merged/bar/ (this should list foo-child)

- umount merged
- mv lower/foo lower/baz
- mount -t overlay -o lowerdir=lower,upperdir=upper,workdir=work,redirect_dir=on none merged
- ls merged/bar/  (Now foo-child has disappeared)

IOW, modifying lower layers did not crash overlay but it resulted in
directory contents being lost and that can be unexpected. So don't
support lower layer recreation/modification when redirect_dir is enabled
at any point of time.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 Documentation/filesystems/overlayfs.rst | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/Documentation/filesystems/overlayfs.rst b/Documentation/filesystems/overlayfs.rst
index 660dbaf0b9b8..1d1a8da7fdbc 100644
--- a/Documentation/filesystems/overlayfs.rst
+++ b/Documentation/filesystems/overlayfs.rst
@@ -371,8 +371,8 @@ conflict with metacopy=on, and will result in an error.
 [*] redirect_dir=follow only conflicts with metacopy=on if upperdir=... is
 given.
 
-Sharing and copying layers
---------------------------
+Sharing, copying and recreating lower layers
+--------------------------------------------
 
 Lower layers may be shared among several overlay mounts and that is indeed
 a very common practice.  An overlay mount may use the same lower layer
@@ -388,8 +388,12 @@ though it will not result in a crash or deadlock.
 
 Mounting an overlay using an upper layer path, where the upper layer path
 was previously used by another mounted overlay in combination with a
-different lower layer path, is allowed, unless the "inodes index" feature
-or "metadata only copy up" feature is enabled.
+different lower layer path, is allowed, unless any of the following features
+is enabled at any point of time.
+
+- inode index
+- metadata only copy up
+- redirect_dir
 
 With the "inodes index" feature, on the first time mount, an NFS file
 handle of the lower layer root directory, along with the UUID of the lower
-- 
2.25.4


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

* Re: [PATCH] overlayfs, doc: Do not allow lower layer recreation with redirect_dir enabled
  2020-07-09 14:02 [PATCH] overlayfs, doc: Do not allow lower layer recreation with redirect_dir enabled Vivek Goyal
@ 2020-07-09 14:14 ` Vivek Goyal
  2020-07-09 14:49   ` Amir Goldstein
  0 siblings, 1 reply; 4+ messages in thread
From: Vivek Goyal @ 2020-07-09 14:14 UTC (permalink / raw)
  To: Amir Goldstein, Miklos Szeredi; +Cc: linux-unionfs

On Thu, Jul 09, 2020 at 10:02:20AM -0400, Vivek Goyal wrote:
> Currently we seem to support lower layer recreation and re-use with existing
> upper until and unless "index" or "metadata only copy up" feature is
> enabled.
> 
> If redirect_dir feature is enabled then re-creating/modifying lower layers
> will break things. For example.
> 
> - mkdir lower lower/foo upper work merged
> - touch lower/foo/foo-child
> - mount -t overlay -o lowerdir=lower,upperdir=upper,workdir=work,redirect_dir=on none merged
> - mv merged/foo merged/bar
> - ls merged/bar/ (this should list foo-child)
> 
> - umount merged
> - mv lower/foo lower/baz
> - mount -t overlay -o lowerdir=lower,upperdir=upper,workdir=work,redirect_dir=on none merged
> - ls merged/bar/  (Now foo-child has disappeared)
> 
> IOW, modifying lower layers did not crash overlay but it resulted in
> directory contents being lost and that can be unexpected. So don't
> support lower layer recreation/modification when redirect_dir is enabled
> at any point of time.
> 
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  Documentation/filesystems/overlayfs.rst | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/filesystems/overlayfs.rst b/Documentation/filesystems/overlayfs.rst
> index 660dbaf0b9b8..1d1a8da7fdbc 100644
> --- a/Documentation/filesystems/overlayfs.rst
> +++ b/Documentation/filesystems/overlayfs.rst
> @@ -371,8 +371,8 @@ conflict with metacopy=on, and will result in an error.
>  [*] redirect_dir=follow only conflicts with metacopy=on if upperdir=... is
>  given.
>  
> -Sharing and copying layers
> ---------------------------
> +Sharing, copying and recreating lower layers
> +--------------------------------------------
>  
>  Lower layers may be shared among several overlay mounts and that is indeed
>  a very common practice.  An overlay mount may use the same lower layer
> @@ -388,8 +388,12 @@ though it will not result in a crash or deadlock.
>  
>  Mounting an overlay using an upper layer path, where the upper layer path
>  was previously used by another mounted overlay in combination with a
> -different lower layer path, is allowed, unless the "inodes index" feature
> -or "metadata only copy up" feature is enabled.
> +different lower layer path, is allowed, unless any of the following features
> +is enabled at any point of time.
> +
> +- inode index
> +- metadata only copy up
> +- redirect_dir

I probably should add "nfs_export" to the list as well. Though it is
implicitly there as enabling nfs export requires to enable index. But
saying it explicitly is even better.

Vivek

>  
>  With the "inodes index" feature, on the first time mount, an NFS file
>  handle of the lower layer root directory, along with the UUID of the lower
> -- 
> 2.25.4
> 


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

* Re: [PATCH] overlayfs, doc: Do not allow lower layer recreation with redirect_dir enabled
  2020-07-09 14:14 ` Vivek Goyal
@ 2020-07-09 14:49   ` Amir Goldstein
  2020-07-09 15:36     ` Vivek Goyal
  0 siblings, 1 reply; 4+ messages in thread
From: Amir Goldstein @ 2020-07-09 14:49 UTC (permalink / raw)
  To: Vivek Goyal; +Cc: Miklos Szeredi, overlayfs

On Thu, Jul 9, 2020 at 5:14 PM Vivek Goyal <vgoyal@redhat.com> wrote:
>
> On Thu, Jul 09, 2020 at 10:02:20AM -0400, Vivek Goyal wrote:
> > Currently we seem to support lower layer recreation and re-use with existing
> > upper until and unless "index" or "metadata only copy up" feature is
> > enabled.
> >
> > If redirect_dir feature is enabled then re-creating/modifying lower layers
> > will break things. For example.
> >
> > - mkdir lower lower/foo upper work merged
> > - touch lower/foo/foo-child
> > - mount -t overlay -o lowerdir=lower,upperdir=upper,workdir=work,redirect_dir=on none merged
> > - mv merged/foo merged/bar
> > - ls merged/bar/ (this should list foo-child)
> >
> > - umount merged
> > - mv lower/foo lower/baz
> > - mount -t overlay -o lowerdir=lower,upperdir=upper,workdir=work,redirect_dir=on none merged
> > - ls merged/bar/  (Now foo-child has disappeared)
> >
> > IOW, modifying lower layers did not crash overlay but it resulted in
> > directory contents being lost and that can be unexpected. So don't
> > support lower layer recreation/modification when redirect_dir is enabled
> > at any point of time.

I don't understand why this has to do with redirect_dir.
The same text holds also if you do not do mv merged/foo merged/bar


> >
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > ---
> >  Documentation/filesystems/overlayfs.rst | 12 ++++++++----
> >  1 file changed, 8 insertions(+), 4 deletions(-)
> >
> > diff --git a/Documentation/filesystems/overlayfs.rst b/Documentation/filesystems/overlayfs.rst
> > index 660dbaf0b9b8..1d1a8da7fdbc 100644
> > --- a/Documentation/filesystems/overlayfs.rst
> > +++ b/Documentation/filesystems/overlayfs.rst
> > @@ -371,8 +371,8 @@ conflict with metacopy=on, and will result in an error.
> >  [*] redirect_dir=follow only conflicts with metacopy=on if upperdir=... is
> >  given.
> >
> > -Sharing and copying layers
> > ---------------------------
> > +Sharing, copying and recreating lower layers
> > +--------------------------------------------
> >
> >  Lower layers may be shared among several overlay mounts and that is indeed
> >  a very common practice.  An overlay mount may use the same lower layer
> > @@ -388,8 +388,12 @@ though it will not result in a crash or deadlock.
> >
> >  Mounting an overlay using an upper layer path, where the upper layer path
> >  was previously used by another mounted overlay in combination with a
> > -different lower layer path, is allowed, unless the "inodes index" feature
> > -or "metadata only copy up" feature is enabled.
> > +different lower layer path, is allowed, unless any of the following features
> > +is enabled at any point of time.
> > +
> > +- inode index
> > +- metadata only copy up
> > +- redirect_dir
>
> I probably should add "nfs_export" to the list as well. Though it is
> implicitly there as enabling nfs export requires to enable index. But
> saying it explicitly is even better.
>

Ok you can mention it but xino is also relevant.
I wonder if we should define "legacy mode" where no options expect for
lowerdir,upperdir,workdir,default_permissions are provided

Thanks,
Amir.

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

* Re: [PATCH] overlayfs, doc: Do not allow lower layer recreation with redirect_dir enabled
  2020-07-09 14:49   ` Amir Goldstein
@ 2020-07-09 15:36     ` Vivek Goyal
  0 siblings, 0 replies; 4+ messages in thread
From: Vivek Goyal @ 2020-07-09 15:36 UTC (permalink / raw)
  To: Amir Goldstein; +Cc: Miklos Szeredi, overlayfs

On Thu, Jul 09, 2020 at 05:49:05PM +0300, Amir Goldstein wrote:
> On Thu, Jul 9, 2020 at 5:14 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> >
> > On Thu, Jul 09, 2020 at 10:02:20AM -0400, Vivek Goyal wrote:
> > > Currently we seem to support lower layer recreation and re-use with existing
> > > upper until and unless "index" or "metadata only copy up" feature is
> > > enabled.
> > >
> > > If redirect_dir feature is enabled then re-creating/modifying lower layers
> > > will break things. For example.
> > >
> > > - mkdir lower lower/foo upper work merged
> > > - touch lower/foo/foo-child
> > > - mount -t overlay -o lowerdir=lower,upperdir=upper,workdir=work,redirect_dir=on none merged
> > > - mv merged/foo merged/bar
> > > - ls merged/bar/ (this should list foo-child)
> > >
> > > - umount merged
> > > - mv lower/foo lower/baz
> > > - mount -t overlay -o lowerdir=lower,upperdir=upper,workdir=work,redirect_dir=on none merged
> > > - ls merged/bar/  (Now foo-child has disappeared)
> > >
> > > IOW, modifying lower layers did not crash overlay but it resulted in
> > > directory contents being lost and that can be unexpected. So don't
> > > support lower layer recreation/modification when redirect_dir is enabled
> > > at any point of time.
> 
> I don't understand why this has to do with redirect_dir.
> The same text holds also if you do not do mv merged/foo merged/bar

It does not. "mv foo bar" gets -EXDEV and then mv copies up whole directory
tree. So even if lower layer is modified, we still see "bar/foo-child".

So behavior is little different in two cases. In one case, we lose
directory tree (as lower renamed it) and in another case we still
see it(because we copied it up).

Now one can argue that probably user will expect it (as long as they
know what redirect_dir does and all the semantics of copy up).

If our goal is that we need to allow this (as long as system does
not crash and does not throw errors) and all the resulting behaviors
can be explained away, then we probably don't need this patch.

I am not sure why do we need to allow updating lower and reuse with
same upper. It is hard to support these configurations and resulting
behavior. So with this patch I am trying to cut down the number
of options which allow this configuraiton.

In general, if any overlay features stores metadata in upper about lower
layer, then with lower layer change, that metadata can be broken. And
we probably should not allow such configuraiton. I am approaching it
from this angle. (until and unless there is a strong use case that
why we need to continue to support it with newer overlay features).

Thanks
Vivek


> 
> 
> > >
> > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > > ---
> > >  Documentation/filesystems/overlayfs.rst | 12 ++++++++----
> > >  1 file changed, 8 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/Documentation/filesystems/overlayfs.rst b/Documentation/filesystems/overlayfs.rst
> > > index 660dbaf0b9b8..1d1a8da7fdbc 100644
> > > --- a/Documentation/filesystems/overlayfs.rst
> > > +++ b/Documentation/filesystems/overlayfs.rst
> > > @@ -371,8 +371,8 @@ conflict with metacopy=on, and will result in an error.
> > >  [*] redirect_dir=follow only conflicts with metacopy=on if upperdir=... is
> > >  given.
> > >
> > > -Sharing and copying layers
> > > ---------------------------
> > > +Sharing, copying and recreating lower layers
> > > +--------------------------------------------
> > >
> > >  Lower layers may be shared among several overlay mounts and that is indeed
> > >  a very common practice.  An overlay mount may use the same lower layer
> > > @@ -388,8 +388,12 @@ though it will not result in a crash or deadlock.
> > >
> > >  Mounting an overlay using an upper layer path, where the upper layer path
> > >  was previously used by another mounted overlay in combination with a
> > > -different lower layer path, is allowed, unless the "inodes index" feature
> > > -or "metadata only copy up" feature is enabled.
> > > +different lower layer path, is allowed, unless any of the following features
> > > +is enabled at any point of time.
> > > +
> > > +- inode index
> > > +- metadata only copy up
> > > +- redirect_dir
> >
> > I probably should add "nfs_export" to the list as well. Though it is
> > implicitly there as enabling nfs export requires to enable index. But
> > saying it explicitly is even better.
> >
> 
> Ok you can mention it but xino is also relevant.
> I wonder if we should define "legacy mode" where no options expect for
> lowerdir,upperdir,workdir,default_permissions are provided
> 
> Thanks,
> Amir.
> 


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

end of thread, other threads:[~2020-07-09 15:36 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-09 14:02 [PATCH] overlayfs, doc: Do not allow lower layer recreation with redirect_dir enabled Vivek Goyal
2020-07-09 14:14 ` Vivek Goyal
2020-07-09 14:49   ` Amir Goldstein
2020-07-09 15:36     ` Vivek Goyal

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