On Fri, May 29, 2020 at 06:46:43PM +0300, Amir Goldstein wrote:
> > > > @@ -1023,7 +1020,7 @@ struct dentry *ovl_lookup(struct inode *
> > > > *
> > > > * Always lookup index of non-dir non-metacopy and non-upper.
> > > > */
> > > > - if (ctr && (!upperdentry || (!d.is_dir && !metacopy)))
> > > > + if (ctr && (!upperdentry || (!d.is_dir && !uppermetacopy)))
> > > > origin = stack[0].dentry;
> > > >
> > >
> > > I think this should be:
> > >
> > > * Always lookup index of non-dir and non-upper.
> > > */
> > > if (!origin && ctr && (!upperdentry || !d.is_dir))
> > > origin = stack[0].dentry;
> > >
> > > uppermetacopy is guaranteed to either have origin already set or
> > > exit with an an error for ovl_verify_origin().
> >
> > Only if index is enabled and upper had origin xattr.
> >
> > (!d.is_dir && ofs->config.index && origin_path)
> >
> > So if index is disabled or uppermetacopy did not have "origin" xattr,
> > we will not have origin set by the time we come out of the loop.
> >
>
> True. But if index is disabled, setting origin is moot. origin is only
> ever used here to lookup the index.
Well, while looking up for index, we are checking for presence of
index dir (and not checking whether index is currently enabled or
not). So if somebody mounts overlayfs with index=on and later remounts
with index=off, we can still start looking up the index even if it
is not enabled. Is it intentional? If not, to simplify it, should
we lookup index only if it is enabled.
if (origin && ofs->config.index &&
(!d.is_dir || ovl_index_all(dentry->d_sb))) {
index = ovl_lookup_index(ofs, upperdentry, origin, true);
>
> About "origin" xattr. If it is not set in upper that lower fs probably does
> not have file handle support. In that case, index cannot be enabled
> anyway.
What about the case of multiple lower layers. IIUC, we will only
ensure that top most lower layer has file handle support and not
worry about rest of the layers. This will break the case of setting
origin for !upperdentry. This will lookup index and fail if lower
layer does not support file handle.
So may be while enabling index, we should make sure all lower
layers support file handles otherwise fail?
>
> > I see for non-metacopy regular files, if upper did not have origin
> > xattr, that means origin_path will by NULL. That means ctr will be
> > 0 and that means we will not set "origin" for non-metacopy regular
> > files in such case. So question is, should we set "origin" for
> > metacopy upper files in such a case.
> >
> > We did not have origin xattr, but we looked up lower layers for
> > upper metacopy. In theory, stack[0].dentry is origin for upper
> > metacopy files. Should we use it? Current logic does not and that's
> > why this additiona check (!d.is_dir && !uppermetacopy).
> >
>
> I agree with your analysis, but this is a very theoretical discussion.
> Unless I am missing something, I think we have written a very complex
> condition for a corner case that doesn't seem to be valid or interesting.
I agree. I want to simplify it too. Just trying to make sure that
I don't end up breaking some valid configuration.
>
> Basically, for non-dir, if there is no "origin" xattr, then there should be no
> index, because the metacopy feature was added way long after we
> started storing "origin" on copy up. That's not the case for directories.
>
> There is one corner case where it may be relevant -
> overlay layers with metacopy that were created on fs with no file handle
> support (or no uuid) that are migrated to a filesystem with file handle
> support (and metacopy xattr are preserved in migration).
> In that case, index may be enabled while upper metacopy exists
> without "origin".
>
> What happens if we do not set origin and do not lookup index in that case?
> We can get two overlay inodes, both from different metacopy upper inodes
> redirected to the same lower inode, that have the same st_ino, but differnt
> metadata.
We do not set origin on upper for broken hardlinks. So we will report
inode number from upper. I tried. it.
I tried following.
- touch foo.txt
- ln foo.txt foo-link.txt
- mount with metacopy=on
- chwon test:test foo.txt
- umount
- Goto upper/ and remove origin xattr from foo.txt. But there should not
be one because we do not create ORIGIN for broken hardlinks if index is
not enabled.
- mount overlay with index=on
- Do stat on foo.txt and foo-link.txt. foo.txt reports inode number from
upper and foo-link.txt reports inode number from lower.
- chown test:test foo-link.txt
- stat foo-link.txt still reports inode number from lower.
Anyway, at this point of time, how about following.
- For non-upper dentry, always set origin.
- For upperdentry, there are 3 cases.
- directories
- regular files
- regular metacopy files
For directories and regular metacopy files only use verified origin.
That means upper has origin xattr and it matches patch based looked
up dentry. If we did not verify because either ORIGIN xattr is not
there, or because index is not enabled or because ovl_verify_lower()
is not set, then don't use path based looked up dentry as origin.
For the case of regular file upper dentry, use unverified origin.
It implies that ORGIN xattr is there. As there is no path based
lookup origin for upper regular files.
I am attaching a simple patch. Please let me know what do you think
of it.
>
> > >
> > > HOWEVER, if we set origin to lower, which turns out to be a lower
> > > metacopy, we then skip this layer to the next one, but origin remains
> > > set on the skipped layer dentry, which we had already dput().
> > > Ay ay ay!
> >
> > We only skip the intermediate metacopy entries in lower. So top most
> > lower metacopy will still be retained. For example, if there are 3
> > lower layers where top two are metacopy and one data, then we will
> > only skip middle one. And middle one should not be origin for upper.
> >
> > /*
> > * Do not store intermediate metacopy dentries in chain,
> > * except top most lower metacopy dentry
> > */
> > if (d.metacopy && ctr) {
> > dput(this);
> > continue;
> > }
> >
> > For the first lower, ctr will be 0 and we will always store it in
> > stack. So if it is metacopy dentry, it will still be stored at
> > stack[0].
> >
> > Do you still see the problem?
>
> No. it's fine. My eyes missed the ctr condition.
> I still think since you are changing this code.
> It will be much easier to follow if both simple continue statement
> are at the top of the loop.
Ok, will do.
Here is the patch to simplify the condition or origin. I will add some
changelog and comments in code in v2 of patch if you like the patch.
Thanks
Vivek
---
fs/overlayfs/namei.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
Index: redhat-linux/fs/overlayfs/namei.c
===================================================================
--- redhat-linux.orig/fs/overlayfs/namei.c 2020-05-29 14:24:45.997113946 -0400
+++ redhat-linux/fs/overlayfs/namei.c 2020-05-29 14:46:46.692113946 -0400
@@ -1005,6 +1005,7 @@ struct dentry *ovl_lookup(struct inode *
}
stack = origin_path;
ctr = 1;
+ origin = origin_path->dentry;
origin_path = NULL;
}
@@ -1021,9 +1022,9 @@ struct dentry *ovl_lookup(struct inode *
* index. This case should be handled in same way as a non-dir upper
* without ORIGIN is handled.
*
- * Always lookup index of non-dir non-metacopy and non-upper.
+ * Always lookup index of non-upper.
*/
- if (ctr && (!upperdentry || (!d.is_dir && !metacopy)))
+ if (!origin && ctr && !upperdentry)
origin = stack[0].dentry;
if (origin && ovl_indexdir(dentry->d_sb) &&
