On Fri, May 29, 2020 at 10:01 PM Vivek Goyal <vgoyal@xxxxxxxxxx> wrote:
>
> 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);
>
What do you mean by remount? actual -o remount cannot
change any overlay config variables.
For umount/mount, ofs->indexdir doesn't mean that there is an
index dir, it means that index dir is in use because index is enabled.
See:
if (!(ovl_force_readonly(ofs)) && ofs->config.index) {
...
err = ovl_get_indexdir(sb, ofs, oe, &upperpath);
...
if (!ofs->indexdir) {
ofs->config.index = false;
Most of the code checks for ofs->indexdir.
The test for ofs->config.index is also correct, but inconsistent, although
I see that we also use ofs->config.index in other places in ovl_lookup().
>
> >
> > 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?
>
Enabling index requires that all layers support file handles:
pr_warn("fs on '%s' does not support file handles,
falling back to index=off,nfs_export=off.\n",
name);
}
> >
> > > 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.
Good point. I figured we had to have some protection in place.
>
> 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;
>
As I wrote in review, the condition could be further simplified to
(!upperdentry).
Thanks for digging up the truth,
Amir.
