Re: [PATCH 3/5] ovl: implement lookup in data-only layers

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Apr 18, 2023 at 3:41 PM Amir Goldstein <amir73il@xxxxxxxxx> wrote:
>
> On Tue, Apr 18, 2023 at 3:40 PM Alexander Larsson <alexl@xxxxxxxxxx> wrote:
> >
> > On Wed, 2023-04-12 at 16:54 +0300, Amir Goldstein wrote:
> > > Lookup in data-only layers only for a lower metacopy with an absolute
> > > redirect xattr.
> > >
> > > The metacopy xattr is not checked on files found in the data-only
> > > layers
> > > and redirect xattr are not followed in the data-only layers.
> > >
> > > Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
> >
> > Reviewed-by: Alexander Larsson <alexl@xxxxxxxxxx>
> >
> > > ---
> > >  fs/overlayfs/namei.c | 77
> > > ++++++++++++++++++++++++++++++++++++++++++--
> > >  1 file changed, 75 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> > > index ff82155b4f7e..82e103e2308b 100644
> > > --- a/fs/overlayfs/namei.c
> > > +++ b/fs/overlayfs/namei.c
> > > @@ -14,6 +14,8 @@
> > >  #include <linux/exportfs.h>
> > >  #include "overlayfs.h"
> > >
> > > +#include "../internal.h"       /* for vfs_path_lookup */
> > > +
> > >  struct ovl_lookup_data {
> > >         struct super_block *sb;
> > >         struct vfsmount *mnt;
> > > @@ -24,6 +26,8 @@ struct ovl_lookup_data {
> > >         bool last;
> > >         char *redirect;
> > >         bool metacopy;
> > > +       /* Referring to last redirect xattr */
> > > +       bool absolute_redirect;
> > >  };
> > >
> > >  static int ovl_check_redirect(const struct path *path, struct
> > > ovl_lookup_data *d,
> > > @@ -33,11 +37,13 @@ static int ovl_check_redirect(const struct path
> > > *path, struct ovl_lookup_data *d
> > >         char *buf;
> > >         struct ovl_fs *ofs = OVL_FS(d->sb);
> > >
> > > +       d->absolute_redirect = false;
> > >         buf = ovl_get_redirect_xattr(ofs, path, prelen +
> > > strlen(post));
> > >         if (IS_ERR_OR_NULL(buf))
> > >                 return PTR_ERR(buf);
> > >
> > >         if (buf[0] == '/') {
> > > +               d->absolute_redirect = true;
> > >                 /*
> > >                  * One of the ancestor path elements in an absolute
> > > path
> > >                  * lookup in ovl_lookup_layer() could have been
> > > opaque and
> > > @@ -349,6 +355,61 @@ static int ovl_lookup_layer(struct dentry *base,
> > > struct ovl_lookup_data *d,
> > >         return 0;
> > >  }
> > >
> > > +static int ovl_lookup_data_layer(struct dentry *dentry, const char
> > > *redirect,
> > > +                                const struct ovl_layer *layer,
> > > +                                struct path *datapath)
> > > +{
> > > +       int err;
> > > +
> > > +       err = vfs_path_lookup(layer->mnt->mnt_root, layer->mnt,
> > > redirect,
> > > +                       LOOKUP_BENEATH | LOOKUP_NO_SYMLINKS |
> > > LOOKUP_NO_XDEV,
> > > +                       datapath);
> > > +       pr_debug("lookup lowerdata (%pd2, redirect=\"%s\", layer=%d,
> > > err=%i)\n",
> > > +                dentry, redirect, layer->idx, err);
> > > +
> > > +       if (err)
> > > +               return err;
> > > +
> > > +       err = -EREMOTE;
> > > +       if (ovl_dentry_weird(datapath->dentry))
> > > +               goto out_path_put;
> > > +
> > > +       err = -ENOENT;
> > > +       /* Only regular file is acceptable as lower data */
> > > +       if (!d_is_reg(datapath->dentry))
> > > +               goto out_path_put;
> > > +
> > > +       return 0;
> > > +
> > > +out_path_put:
> > > +       path_put(datapath);
> > > +
> > > +       return err;
> > > +}
> > > +
> > > +/* Lookup in data-only layers by absolute redirect to layer root */
> > > +static int ovl_lookup_data_layers(struct dentry *dentry, const char
> > > *redirect,
> > > +                                 struct ovl_path *lowerdata)
> > > +{
> > > +       struct ovl_fs *ofs = OVL_FS(dentry->d_sb);
> > > +       const struct ovl_layer *layer;
> > > +       struct path datapath;
> > > +       int err = -ENOENT;
> > > +       int i;
> > > +
> > > +       layer = &ofs->layers[ofs->numlayer - ofs->numdatalayer];
> > > +       for (i = 0; i < ofs->numdatalayer; i++, layer++) {
> > > +               err = ovl_lookup_data_layer(dentry, redirect, layer,
> > > &datapath);
> > > +               if (!err) {
> > > +                       mntput(datapath.mnt);
> > > +                       lowerdata->dentry = datapath.dentry;
> > > +                       lowerdata->layer = layer;
> > > +                       return 0;
> > > +               }
> > > +       }
> > > +
> > > +       return err;
> > > +}
> > >
> > >  int ovl_check_origin_fh(struct ovl_fs *ofs, struct ovl_fh *fh, bool
> > > connected,
> > >                         struct dentry *upperdentry, struct ovl_path
> > > **stackp)
> > > @@ -907,7 +968,9 @@ struct dentry *ovl_lookup(struct inode *dir,
> > > struct dentry *dentry,
> > >
> > >         if (!d.stop && ovl_numlower(poe)) {
> > >                 err = -ENOMEM;
> > > -               stack = ovl_stack_alloc(ovl_numlowerlayer(ofs));
> > > +               /* May need to reserve space in lowerstack for
> > > lowerdata */
> > > +               stack = ovl_stack_alloc(ovl_numlowerlayer(ofs) +
> > > +                                       (!d.is_dir && !!ofs-
> > > >numdatalayer));
> >
> > I think this runs into issues if ovl_numlower(poe) is zero, but we
> > have a redirect into the lowerdata, due to the if (ovl_numlower(poe))
> > check above. We won't be allocating the lowerdata stack space in this
> > case.
> >
>
> Redirect from upper directly into data-only is not valid.

Ah.

> As the comment below says:
> /* Lookup absolute redirect from lower metacopy in data-only layers */
> data-only can only be accessed by absolute redirect from
> lower layer.
> Is that a problem for your use case?

No, just something I noticed that looked wrong.

> > >                 if (!stack)
> > >                         goto out_put_upper;
> > >         }
> > > @@ -917,7 +980,7 @@ struct dentry *ovl_lookup(struct inode *dir,
> > > struct dentry *dentry,
> > >
> > >                 if (!ofs->config.redirect_follow)
> > >                         d.last = i == ovl_numlower(poe) - 1;
> > > -               else
> > > +               else if (d.is_dir || !ofs->numdatalayer)
> > >                         d.last = lower.layer->idx ==
> > > ovl_numlower(roe);
> > >
> > >                 d.mnt = lower.layer->mnt;
> > > @@ -1011,6 +1074,16 @@ struct dentry *ovl_lookup(struct inode *dir,
> > > struct dentry *dentry,
> > >                 }
> > >         }
> > >
> > > +       /* Lookup absolute redirect from lower metacopy in data-only
> > > layers */
> > > +       if (d.metacopy && ctr && ofs->numdatalayer &&
> > > d.absolute_redirect) {
> > > +               err = ovl_lookup_data_layers(dentry, d.redirect,
> > > +                                            &stack[ctr]);
> > > +               if (!err) {
> > > +                       d.metacopy = false;
> > > +                       ctr++;
> > > +               }
> > > +       }
> >
> > This code runs even if ofs->config.redirect_follow is false. I think
> > this is probably correct, but maybe it should be mentioned in the docs?
> >
>
> ofs->numdatalayer > 0 depends on ofs->config.metacopy
> which depends on ofs->config.redirect or ofs->config.redirect_follow.
>
> Thanks,
> Amir.
>


-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander Larsson                                Red Hat, Inc
       alexl@xxxxxxxxxx         alexander.larsson@xxxxxxxxx





[Index of Archives]     [Linux Filesystems Devel]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux