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

[Vivek Goyal <vgoyal@redhat.com>]

On Fri, May 29, 2020 at 12:07:45AM +0300, Amir Goldstein wrote:

[..]
> > +       /* Found a metacopy dentry but did not find corresponding data dentry */
> > +       if (d.metacopy) {
> > +               err = -EIO;
> > +               goto out_put;
> > +       }
> >
> > +       if (lowermetacopy || uppermetacopy) {
> >                 err = -EPERM;
> >                 if (!ofs->config.metacopy) {
> >                         pr_warn_ratelimited("refusing to follow metacopy origin for (%pd2)\n",
> 
> Move that test up to where setting metacopy = true for lower layers
> similar to "refusing to follow redirect" and make it:
>        if (uppermetacopy || d.metacopy) {
> 
> Then you got rid of lowermetacopy.

Agreed. Will change. 

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

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

> 
> 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?

> 
> I think it would be best to move the check
>                  * Do not store intermediate metacopy dentries in chain,
> to right after ovl_lookup_layer(), before the ovl_fix_origin() and
> ovl_verify_origin() checks.

Thanks
Vivek