[PATCH] ovl: add xino to "changes to underlying fs" docs

[PATCH] ovl: add xino to "changes to underlying fs" docs

[Kevin Locke]

Add "xino" to the list of features which cause undefined behavior for
offline changes to the lower tree in the "Changes to underlying
filesystems" section of the documentation to make users aware of
potential issues if the lower tree is modified and xino was enabled.

This omission was noticed by Amir Goldstein, who mentioned that xino is
one of the "forbidden" features for making offline changes to the lower
tree and that it wasn't currently documented.

Signed-off-by: Kevin Locke <kevin@kevinlocke.name>
---
 Documentation/filesystems/overlayfs.rst | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/filesystems/overlayfs.rst b/Documentation/filesystems/overlayfs.rst
index 78240e29b0bb..52d47bed9ef8 100644
--- a/Documentation/filesystems/overlayfs.rst
+++ b/Documentation/filesystems/overlayfs.rst
@@ -476,9 +476,9 @@ a crash or deadlock.
 
 Offline changes, when the overlay is not mounted, are allowed to the
 upper tree.  Offline changes to the lower tree are only allowed if the
-"metadata only copy up", "inode index", and "redirect_dir" features
-have not been used.  If the lower tree is modified and any of these
-features has been used, the behavior of the overlay is undefined,
+"metadata only copy up", "inode index", "redirect_dir", and "xino"
+features have not been used.  If the lower tree is modified and any of
+these features has been used, the behavior of the overlay is undefined,
 though it will not result in a crash or deadlock.
 
 When the overlay NFS export feature is enabled, overlay filesystems
-- 
2.30.1

Re: [PATCH] ovl: add xino to "changes to underlying fs" docs

[Amir Goldstein]

On Mon, Mar 8, 2021 at 5:23 PM Kevin Locke <kevin@kevinlocke.name> wrote:
>
> Add "xino" to the list of features which cause undefined behavior for
> offline changes to the lower tree in the "Changes to underlying
> filesystems" section of the documentation to make users aware of
> potential issues if the lower tree is modified and xino was enabled.
>
> This omission was noticed by Amir Goldstein, who mentioned that xino is
> one of the "forbidden" features for making offline changes to the lower
> tree and that it wasn't currently documented.
>

Hi Kevin,

Thanks for following up on this.
I see my original comment did not make it to the list, because the message
was formatted incorrectly.

My full comment was:

"...
My feeling is that we need to adjust the fix and not treat the case
of xino=auto as "user opted-in for xino" thus disabling origin inode
decode for lower without uuid.

Also the documentation does not mention the xino feature as one
of the "forbidden" features for this use case.
..."

So your Documentation fix may represent the current code, but
I think we should fix the code instead.

When looking again, I actually don't see a reason to include "xino"
in this check at all (not xino=on nor xino=auto):

 if (!ofs->config.index && !ofs->config.metacopy && !ofs->config.xino &&
     uuid_is_null(uuid))
         return false;

The reason that "index" and "metacopy" are in this check is because
they *need* to follow the lower inode of a non-dir upper in order to
operate correctly. The same is not true for "xino".

Moreover, "xino" will happily be enabled also when lower fs does not
support file handles at all. It will operate sub-optimally, but it will live up
to the promise to provide a unified inode namespace and uniform st_dev.


> Signed-off-by: Kevin Locke <kevin@kevinlocke.name>
> ---
>  Documentation/filesystems/overlayfs.rst | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/filesystems/overlayfs.rst b/Documentation/filesystems/overlayfs.rst
> index 78240e29b0bb..52d47bed9ef8 100644
> --- a/Documentation/filesystems/overlayfs.rst
> +++ b/Documentation/filesystems/overlayfs.rst
> @@ -476,9 +476,9 @@ a crash or deadlock.
>
>  Offline changes, when the overlay is not mounted, are allowed to the
>  upper tree.  Offline changes to the lower tree are only allowed if the
> -"metadata only copy up", "inode index", and "redirect_dir" features
> -have not been used.  If the lower tree is modified and any of these
> -features has been used, the behavior of the overlay is undefined,
> +"metadata only copy up", "inode index", "redirect_dir", and "xino"
> +features have not been used.  If the lower tree is modified and any of
> +these features has been used, the behavior of the overlay is undefined,
>  though it will not result in a crash or deadlock.

Note that "redirect_dir" is not one of the "forbidden" features.

Thanks,
Amir.

Re: [PATCH] ovl: add xino to "changes to underlying fs" docs

[Kevin Locke]

Hi Amir,

On Mon, 2021-03-08 at 19:41 +0200, Amir Goldstein wrote:
> On Mon, Mar 8, 2021 at 5:23 PM Kevin Locke <kevin@kevinlocke.name> wrote:
>> Add "xino" to the list of features which cause undefined behavior for
>> offline changes to the lower tree in the "Changes to underlying
>> filesystems" section of the documentation to make users aware of
>> potential issues if the lower tree is modified and xino was enabled.
>>
>> This omission was noticed by Amir Goldstein, who mentioned that xino is
>> one of the "forbidden" features for making offline changes to the lower
>> tree and that it wasn't currently documented.
> 
> [...]
> When looking again, I actually don't see a reason to include "xino"
> in this check at all (not xino=on nor xino=auto):
> 
>  if (!ofs->config.index && !ofs->config.metacopy && !ofs->config.xino &&
>      uuid_is_null(uuid))
>          return false;
> 
> The reason that "index" and "metacopy" are in this check is because
> they *need* to follow the lower inode of a non-dir upper in order to
> operate correctly. The same is not true for "xino".
> 
> Moreover, "xino" will happily be enabled also when lower fs does not
> support file handles at all. It will operate sub-optimally, but it will live up
> to the promise to provide a unified inode namespace and uniform st_dev.

Good observation!  I think you are right.  After a bit of testing, I did
not notice any issues after making offline changes to lower with xino
enabled.

> Note that "redirect_dir" is not one of the "forbidden" features.

To be clear, are you saying that offline modifications to directories in
lower layers which are the redirection target of an upper layer does not
cause undefined behavior?  Would it make sense for me to work up a patch
which documents the behavior, or is it better to leave as "defined but
undocumented"?

My understanding of the current behavior:

1. If a redirection target dir is removed from lower, contents which do
   not exist in any upper are removed from the redirected dir in the
   overlay.  Contents which exist in an upper are unchanged.
2. If a redirection target dir is renamed in lower, it has the same
   effect as removing the directory with the old name and creating one
   with the new name and contents.
3. Permission changes to a redirection target dir have no effect in the
   overlay.
4. If a previously removed redirection target dir is created, its
   contents are added to the redirected dir in the overlay, unless the
   redirected dir had been renamed after removal, in which case it is
   ignored (because the redirected dir becomes opaque when renamed).
5. If a file with the name of a previously removed redirection target
   dir is created, it is ignored.

The only behavior which seems a bit surprising to me is #4:  If
directory B in upper is redirected to A in lower and A is subsequently
removed, then (possibly years later) a directory named A is created, its
contents would appear in B in the overlay.  However, if B had been
renamed to C after A was removed, it becomes opaque, causing A and its
contents not to appear in the overlay.  Either of these may surprise
users.

Does that match your understanding of the current behavior?  Worth
documenting or better to just remove redirect_dir from the list of
features where offline modification causes undefined behavior?

Thanks,
Kevin

Re: [PATCH] ovl: add xino to "changes to underlying fs" docs

[Amir Goldstein]

On Tue, Mar 9, 2021 at 1:50 AM Kevin Locke <kevin@kevinlocke.name> wrote:
>
> Hi Amir,
>
> On Mon, 2021-03-08 at 19:41 +0200, Amir Goldstein wrote:
> > On Mon, Mar 8, 2021 at 5:23 PM Kevin Locke <kevin@kevinlocke.name> wrote:
> >> Add "xino" to the list of features which cause undefined behavior for
> >> offline changes to the lower tree in the "Changes to underlying
> >> filesystems" section of the documentation to make users aware of
> >> potential issues if the lower tree is modified and xino was enabled.
> >>
> >> This omission was noticed by Amir Goldstein, who mentioned that xino is
> >> one of the "forbidden" features for making offline changes to the lower
> >> tree and that it wasn't currently documented.
> >
> > [...]
> > When looking again, I actually don't see a reason to include "xino"
> > in this check at all (not xino=on nor xino=auto):
> >
> >  if (!ofs->config.index && !ofs->config.metacopy && !ofs->config.xino &&
> >      uuid_is_null(uuid))
> >          return false;
> >
> > The reason that "index" and "metacopy" are in this check is because
> > they *need* to follow the lower inode of a non-dir upper in order to
> > operate correctly. The same is not true for "xino".
> >
> > Moreover, "xino" will happily be enabled also when lower fs does not
> > support file handles at all. It will operate sub-optimally, but it will live up
> > to the promise to provide a unified inode namespace and uniform st_dev.
>
> Good observation!  I think you are right.  After a bit of testing, I did
> not notice any issues after making offline changes to lower with xino
> enabled.
>

He, that's not what I meant.
I wouldn't expect that you *observe* any issues, because the issues
with following the wrong object are quite rare and you need to make
changes to lower squashfs to notice them, see:
https://lore.kernel.org/lkml/20191106234301.283006-1-colin.king@canonical.com/

But as a matter of fact, I was wrong and I misled you. Sorry.

I read the code backwards.

It's not true that we can allow lower modification with "xino=on/auto" -
quite the opposite - we may need to disallow lower modifications also
with "xino=off".

Let me explain.
The following table documents expected behavior with different
features and layer setups:
https://www.kernel.org/doc/html/latest/filesystems/overlayfs.html#inode-properties

As you can see, the matrix is quite complex.
The problem lies with the documented behavior of "Persistent st_ino of !dir"
for the case of "Layers not on same fs, xino=off".

It claims that st_ino will be persistent, but in fact it is only true
if lower fs
supports file handles AND has a unique [*] UUID amongst the lower layers.
The claim that st_ino is persistent for !dir in case of "ino overflow" is also
incorrect.

[*] The special case of NULL UUID (e.g. squashfs) was recently changed
     and depends on whether the opt-in features are enabled...

In any case, the documented behavior for Persistent st_ino (!dir) is
incorrect for the case of (e.g.) lower squashfs with -no-exports.
IWO, in this setup, st_ino of a lower file will change following copy up
and mount cycle.

I do not want to add all this story to documentation - the matrix is
complex enough to follow as it is.

Seeing that distros are switching to enable xino by default, I was
contemplating to change the behavior of the code as follows:

- If user opts-out of xino by mount option (xino=off is *shown*
  in /proc/mounts) do not follow origin by file handle
- Let index and metacopy require and auto-enable xino, so e.g.:
  mount options index=on,xino=off will be a conflict
- If lower does not support file handles or has NULL UUID and
  xino is enabled by default, then auto-disable xino and do not
  follow origin (xino=off will be shown in /proc/mounts)
- If xino is disabled by default, we DO follow origin as we always
  did (xino=off is NOT shown in /proc/mounts)
- Change the documented value for Persistent st_ino (!dir) in case
  of "xino=off" and in case of "ino overflow" to N

Pros:
1. This makes for simpler and more coherent documentation IMO.
2. It doesn't change behavior for legacy layers with all default
    kernel configs and default mount options.
3. It actively averts the reported issues caused by re-formatting
    lower squashfs with distro kernel configs and default mount options.

Cons:
1. After kernel upgrade, existing setups with lower squashfs that did
    not opt-in for xino by mount option will lose xino
2. Existing setups that opt-out of xino by mount option (because of old
    32bit applications?) will loose persistent st_ino behavior

IMO, the Pros out weight the Cons.

I've suggested adding a way to opt-out of following origin several times,
but was not able to convince Miklos so far.
Let's see if this time is any different...

> > Note that "redirect_dir" is not one of the "forbidden" features.
>
> To be clear, are you saying that offline modifications to directories in
> lower layers which are the redirection target of an upper layer does not
> cause undefined behavior?  Would it make sense for me to work up a patch
> which documents the behavior, or is it better to leave as "defined but
> undocumented"?
>

I just mislead you. Sorry for that.
We should leave "redirect_dir" in the documented list and add "xino"
just like the patch you posted.
But I guess if I am going to post a patch to change the xino behavior,
it would be better to include your change in my patch for context.

Thanks,
Amir.

Re: [PATCH] ovl: add xino to "changes to underlying fs" docs

[Amir Goldstein]

On Tue, Mar 9, 2021 at 9:24 AM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Tue, Mar 9, 2021 at 1:50 AM Kevin Locke <kevin@kevinlocke.name> wrote:
> >
> > Hi Amir,
> >
> > On Mon, 2021-03-08 at 19:41 +0200, Amir Goldstein wrote:
> > > On Mon, Mar 8, 2021 at 5:23 PM Kevin Locke <kevin@kevinlocke.name> wrote:
> > >> Add "xino" to the list of features which cause undefined behavior for
> > >> offline changes to the lower tree in the "Changes to underlying
> > >> filesystems" section of the documentation to make users aware of
> > >> potential issues if the lower tree is modified and xino was enabled.
> > >>
> > >> This omission was noticed by Amir Goldstein, who mentioned that xino is
> > >> one of the "forbidden" features for making offline changes to the lower
> > >> tree and that it wasn't currently documented.
> > >
> > > [...]
> > > When looking again, I actually don't see a reason to include "xino"
> > > in this check at all (not xino=on nor xino=auto):
> > >
> > >  if (!ofs->config.index && !ofs->config.metacopy && !ofs->config.xino &&
> > >      uuid_is_null(uuid))
> > >          return false;
> > >
> > > The reason that "index" and "metacopy" are in this check is because
> > > they *need* to follow the lower inode of a non-dir upper in order to
> > > operate correctly. The same is not true for "xino".
> > >
> > > Moreover, "xino" will happily be enabled also when lower fs does not
> > > support file handles at all. It will operate sub-optimally, but it will live up
> > > to the promise to provide a unified inode namespace and uniform st_dev.
> >
> > Good observation!  I think you are right.  After a bit of testing, I did
> > not notice any issues after making offline changes to lower with xino
> > enabled.
> >
>
> He, that's not what I meant.
> I wouldn't expect that you *observe* any issues, because the issues
> with following the wrong object are quite rare and you need to make
> changes to lower squashfs to notice them, see:
> https://lore.kernel.org/lkml/20191106234301.283006-1-colin.king@canonical.com/
>
> But as a matter of fact, I was wrong and I misled you. Sorry.
>
> I read the code backwards.
>
> It's not true that we can allow lower modification with "xino=on/auto" -
> quite the opposite - we may need to disallow lower modifications also
> with "xino=off".
>
> Let me explain.
> The following table documents expected behavior with different
> features and layer setups:
> https://www.kernel.org/doc/html/latest/filesystems/overlayfs.html#inode-properties
>
> As you can see, the matrix is quite complex.
> The problem lies with the documented behavior of "Persistent st_ino of !dir"
> for the case of "Layers not on same fs, xino=off".
>
> It claims that st_ino will be persistent, but in fact it is only true
> if lower fs
> supports file handles AND has a unique [*] UUID amongst the lower layers.
> The claim that st_ino is persistent for !dir in case of "ino overflow" is also
> incorrect.
>
> [*] The special case of NULL UUID (e.g. squashfs) was recently changed
>      and depends on whether the opt-in features are enabled...
>
> In any case, the documented behavior for Persistent st_ino (!dir) is
> incorrect for the case of (e.g.) lower squashfs with -no-exports.
> IWO, in this setup, st_ino of a lower file will change following copy up
> and mount cycle.
>
> I do not want to add all this story to documentation - the matrix is
> complex enough to follow as it is.
>

This came out too complicated. Let me try again -

The documentation in the section:
https://www.kernel.org/doc/html/latest/filesystems/overlayfs.html#overlay-objects
speaks about overlayfs objects having non-unique and non-persistent st_ino.
It then goes on to say that "xino" can be used to make overlayfs "compliant",
but in fact never speaks of persistent st_ino until the comparison table,
where the documented values are incorrect.

So I decided to try and promote "xino" from a feature that "makes inode
numbers unique" to a feature that "makes inode numbers unique and
if possible, also persistent" by adding the following text to the section:

"...
The "xino" feature can be enabled with the "-o xino=on" overlay mount option.
If all underlying filesystems support NFS file handles, the value of st_ino
for overlay filesystem objects is not only unique, but also persistent over
the lifetime of the filesystem.  The "-o xino=auto" overlay mount option
enables the "xino" feature only if the persistent st_ino requirement is met.
..."

And with this I pured new meaning into xino=auto, which lost its original
meaning after commit:
926e94d79baf ("ovl: enable xino automatically in more cases")

The code change is to fall back from xino=auto to xino=off in cases
where the lower layer has no file handle support or bad uuid.

I'll post the patch for review soon.

Thanks,
Amir.

Re: [PATCH] ovl: add xino to "changes to underlying fs" docs

[Kevin Locke]

On Tue, 2021-03-09 at 09:24 +0200, Amir Goldstein wrote:
> We should leave "redirect_dir" in the documented list and add "xino"
> just like the patch you posted.
> But I guess if I am going to post a patch to change the xino behavior,
> it would be better to include your change in my patch for context.

Many thanks for the detailed explanations!  My apologies for misreading
(and reading too much into) your previous post.  That sounds good to me.
The "xino" docs addition in the patch you posted looks great!

Thanks again,
Kevin

Re: [PATCH] ovl: add xino to "changes to underlying fs" docs

[Vivek Goyal]

On Tue, Mar 09, 2021 at 09:24:22AM +0200, Amir Goldstein wrote:
> On Tue, Mar 9, 2021 at 1:50 AM Kevin Locke <kevin@kevinlocke.name> wrote:
> >
> > Hi Amir,
> >
> > On Mon, 2021-03-08 at 19:41 +0200, Amir Goldstein wrote:
> > > On Mon, Mar 8, 2021 at 5:23 PM Kevin Locke <kevin@kevinlocke.name> wrote:
> > >> Add "xino" to the list of features which cause undefined behavior for
> > >> offline changes to the lower tree in the "Changes to underlying
> > >> filesystems" section of the documentation to make users aware of
> > >> potential issues if the lower tree is modified and xino was enabled.
> > >>
> > >> This omission was noticed by Amir Goldstein, who mentioned that xino is
> > >> one of the "forbidden" features for making offline changes to the lower
> > >> tree and that it wasn't currently documented.
> > >
> > > [...]
> > > When looking again, I actually don't see a reason to include "xino"
> > > in this check at all (not xino=on nor xino=auto):
> > >
> > >  if (!ofs->config.index && !ofs->config.metacopy && !ofs->config.xino &&
> > >      uuid_is_null(uuid))
> > >          return false;
> > >
> > > The reason that "index" and "metacopy" are in this check is because
> > > they *need* to follow the lower inode of a non-dir upper in order to
> > > operate correctly. The same is not true for "xino".
> > >
> > > Moreover, "xino" will happily be enabled also when lower fs does not
> > > support file handles at all. It will operate sub-optimally, but it will live up
> > > to the promise to provide a unified inode namespace and uniform st_dev.
> >
> > Good observation!  I think you are right.  After a bit of testing, I did
> > not notice any issues after making offline changes to lower with xino
> > enabled.
> >
> 
> He, that's not what I meant.
> I wouldn't expect that you *observe* any issues, because the issues
> with following the wrong object are quite rare and you need to make
> changes to lower squashfs to notice them, see:
> https://lore.kernel.org/lkml/20191106234301.283006-1-colin.king@canonical.com/
> 
> But as a matter of fact, I was wrong and I misled you. Sorry.
> 
> I read the code backwards.
> 
> It's not true that we can allow lower modification with "xino=on/auto" -
> quite the opposite - we may need to disallow lower modifications also
> with "xino=off".
> 
> Let me explain.
> The following table documents expected behavior with different
> features and layer setups:
> https://www.kernel.org/doc/html/latest/filesystems/overlayfs.html#inode-properties
> 
> As you can see, the matrix is quite complex.
> The problem lies with the documented behavior of "Persistent st_ino of !dir"
> for the case of "Layers not on same fs, xino=off".
> 
> It claims that st_ino will be persistent, but in fact it is only true
> if lower fs
> supports file handles AND has a unique [*] UUID amongst the lower layers.
> The claim that st_ino is persistent for !dir in case of "ino overflow" is also
> incorrect.
> 
> [*] The special case of NULL UUID (e.g. squashfs) was recently changed
>      and depends on whether the opt-in features are enabled...
> 
> In any case, the documented behavior for Persistent st_ino (!dir) is
> incorrect for the case of (e.g.) lower squashfs with -no-exports.
> IWO, in this setup, st_ino of a lower file will change following copy up
> and mount cycle.
> 
> I do not want to add all this story to documentation - the matrix is
> complex enough to follow as it is.
> 
> Seeing that distros are switching to enable xino by default, I was
> contemplating to change the behavior of the code as follows:
> 
> - If user opts-out of xino by mount option (xino=off is *shown*
>   in /proc/mounts) do not follow origin by file handle
> - Let index and metacopy require and auto-enable xino, so e.g.:
>   mount options index=on,xino=off will be a conflict
> - If lower does not support file handles or has NULL UUID and
>   xino is enabled by default, then auto-disable xino and do not
>   follow origin (xino=off will be shown in /proc/mounts)
> - If xino is disabled by default, we DO follow origin as we always
>   did (xino=off is NOT shown in /proc/mounts)
> - Change the documented value for Persistent st_ino (!dir) in case
>   of "xino=off" and in case of "ino overflow" to N
> 
> Pros:
> 1. This makes for simpler and more coherent documentation IMO.
> 2. It doesn't change behavior for legacy layers with all default
>     kernel configs and default mount options.
> 3. It actively averts the reported issues caused by re-formatting
>     lower squashfs with distro kernel configs and default mount options.
> 
> Cons:
> 1. After kernel upgrade, existing setups with lower squashfs that did
>     not opt-in for xino by mount option will lose xino
> 2. Existing setups that opt-out of xino by mount option (because of old
>     32bit applications?) will loose persistent st_ino behavior
> 
> IMO, the Pros out weight the Cons.
> 
> I've suggested adding a way to opt-out of following origin several times,
> but was not able to convince Miklos so far.
> Let's see if this time is any different...
> 
> > > Note that "redirect_dir" is not one of the "forbidden" features.
> >
> > To be clear, are you saying that offline modifications to directories in
> > lower layers which are the redirection target of an upper layer does not
> > cause undefined behavior?  Would it make sense for me to work up a patch
> > which documents the behavior, or is it better to leave as "defined but
> > undocumented"?
> >
> 
> I just mislead you. Sorry for that.
> We should leave "redirect_dir" in the documented list and add "xino"
> just like the patch you posted.
> But I guess if I am going to post a patch to change the xino behavior,
> it would be better to include your change in my patch for context.

This is quite complex to understand. I think I still stick to general
stand that if any overlay feature stores any metadata info about
lower layer in upper layer, then we should not allow changes to
offline layers.  Otherwise there are so many possibilities to analyze
to figure out the effect of a offline change. It is confusing for
developers as well as users. So, IMHO, I will take simpler approach of
no lower layer modifications for all these advanced features otherwise
expect the unexpected. :-)

Thanks
Vivek

Re: [PATCH] ovl: add xino to "changes to underlying fs" docs

[Amir Goldstein]

> > I just mislead you. Sorry for that.
> > We should leave "redirect_dir" in the documented list and add "xino"
> > just like the patch you posted.
> > But I guess if I am going to post a patch to change the xino behavior,
> > it would be better to include your change in my patch for context.
>
> This is quite complex to understand. I think I still stick to general
> stand that if any overlay feature stores any metadata info about
> lower layer in upper layer, then we should not allow changes to
> offline layers.  Otherwise there are so many possibilities to analyze
> to figure out the effect of a offline change. It is confusing for
> developers as well as users. So, IMHO, I will take simpler approach of
> no lower layer modifications for all these advanced features otherwise
> expect the unexpected. :-)
>

Eventually, I only added "xino" to the list, the same as Kevin's patch,
but I also changed the code and documentation of the "xino" feature.

Thanks,
Amir.