On Tue, Apr 18, 2023 at 5:00 PM Alexander Larsson <alexl@xxxxxxxxxx> wrote:
>
> 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.
I haven't actually explained why that limitation exists.
Apart from the fact that your use case does not need
redirect from upper to data-only, when the stack is composed
of only upper and lower (not 2 lowers) then the lower is used
for the overlay ino/index/fh.
data-only layers cannot be used for ovelray ino/index/fh because
they are data-only and ino/fh is metadata.
That's also the reason that data-only layers do not need to go through
the ovl_lower_uuid_ok() check, which allows greater flexibility
with the filesystems being used for the backing store of the data blobs.
Thanks,
Amir.
