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
