Re: [PATCH] ovl: fix some bug exist in ovl_get_inode

[Vivek Goyal <vgoyal@redhat.com>]

On Sat, May 30, 2020 at 02:35:14PM +0300, Amir Goldstein wrote:
> On Fri, May 29, 2020 at 10:01 PM Vivek Goyal <vgoyal@redhat.com> 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;

I mean umount/mount. Got it so ovl_indexdir() will be true only if
index is enabled (and not just because index dir is present).

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

It will be nice if we had a single check to determine if index is
enabled or not. (instead of having instances of both ofs->config.index
as well as ovl_indexdir()).

> 
> >
> > >
> > > 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);
>         }

Got it. So we will make sure all lower layers support file handles.

Thanks
Vivek