Re: [RFC PATCH v2 2/9] ovl: read feature set from each layer's root dir

[Amir Goldstein <amir73il@gmail.com>]

On Tue, Jul 31, 2018 at 12:37 PM, zhangyi (F) <yi.zhang@huawei.com> wrote:
> In order to distinguish the backward compatible and backward
> incompatible features in overlay filesystem, we introduce compatible,
> read-only compatible and incompatible these three kinds of features,
> store them as __be64 bit mask on each layer's root directory via
> "trusted.overlay.feature" xattr, which contains the features of this
> layer.
>
> This patch just read feature bitsets from each layer, doesn't add any
> already known features and doesn't yet use the feature bitsets.
>
> Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
> Suggested-by: Miklos Szeredi <miklos@szeredi.hu>
> Suggested-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/overlayfs/Makefile    |   2 +-
>  fs/overlayfs/feature.c   | 119 +++++++++++++++++++++++++++++++++++++++++++++++
>  fs/overlayfs/overlayfs.h |  15 ++++++
>  fs/overlayfs/ovl_entry.h |   5 ++
>  fs/overlayfs/super.c     |   5 ++
>  5 files changed, 145 insertions(+), 1 deletion(-)
>  create mode 100644 fs/overlayfs/feature.c
>
> diff --git a/fs/overlayfs/Makefile b/fs/overlayfs/Makefile
> index 30802347a020..219c927069d2 100644
> --- a/fs/overlayfs/Makefile
> +++ b/fs/overlayfs/Makefile
> @@ -5,4 +5,4 @@
>  obj-$(CONFIG_OVERLAY_FS) += overlay.o
>
>  overlay-objs := super.o namei.o util.o inode.o dir.o readdir.o copy_up.o \
> -               export.o
> +               export.o feature.o
> diff --git a/fs/overlayfs/feature.c b/fs/overlayfs/feature.c
> new file mode 100644
> index 000000000000..eed6644a2a31
> --- /dev/null
> +++ b/fs/overlayfs/feature.c
> @@ -0,0 +1,119 @@
> +/*
> + * Overlayfs feature sets support.
> + *
> + * Copyright (C) 2018 Huawei. All Rights Reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published by
> + * the Free Software Foundation.
> + */
> +
> +#include <linux/fs.h>
> +#include <linux/xattr.h>
> +#include "overlayfs.h"
> +
> +/*
> + * Get overlay features from the layer's root dir.
> + *
> + * @realroot: real root dir dentry
> + *
> + * Return on-disk overlay feature struct if success, NULL if no feature
> + * and error ptr if error.
> + */
> +static struct ovl_d_feature *ovl_get_feature(struct dentry *realroot)

It may be more appropriate to use plural (features) for function, struct
and xattr name. Don't feel so strongly about it.

> +{
> +       struct ovl_d_feature *odf = NULL;
> +       int res;
> +
> +       res = vfs_getxattr(realroot, OVL_XATTR_FEATURE, NULL, 0);
> +       if (res <= 0) {
> +               if (res == -EOPNOTSUPP || res == -ENODATA)
> +                       return NULL;
> +               goto fail;
> +       }
> +
> +       odf = kzalloc(res, GFP_KERNEL);

Don't really see the point in allocating this struct. You can keep the
features buffer on the stack and check res != sizeof(struct ovl_d_feature)
before reading into the buffer.

> +       if (!odf)
> +               return ERR_PTR(-ENOMEM);
> +
> +       res = vfs_getxattr(realroot, OVL_XATTR_FEATURE, odf, res);
> +       if (res < 0)
> +               goto fail;
> +
> +       /* Check validity */
> +       if (res < sizeof(struct ovl_d_feature)) {
> +               res = -EINVAL;
> +               goto invalid;
> +       }
> +       if (odf->magic != OVL_FEATURE_MAGIC) {
> +               res = -EINVAL;
> +               goto invalid;
> +       }
> +
> +       return odf;
> +out:
> +       kfree(odf);
> +       return ERR_PTR(res);
> +fail:
> +       pr_err("overlayfs: failed to get features (%i)\n", res);
> +       goto out;
> +invalid:
> +       pr_err("overlayfs: invalid features (%*phN)\n", res, odf);
> +       goto out;
> +}
> +
> +/*
> + * Get features from each underlying root dir.
> + *
> + * @ofs: overlay filesystem information
> + * @oe: overlay lower stack
> + *
> + * Return 0 if success, errno otherwise.
> + */
> +int ovl_init_feature_set(struct ovl_fs *ofs, struct ovl_entry *oe)
> +{
> +       struct ovl_layer *upper_layer = ofs->upper_layer;
> +       struct dentry *upper = ofs->upperdir;
> +       struct ovl_d_feature *odf;
> +       int i;
> +
> +       /* Get features from the upper root dir */
> +       if (upper_layer) {
> +               odf = ovl_get_feature(upper);
> +               if (IS_ERR(odf))
> +                       return PTR_ERR(odf);
> +
> +               if (odf) {
> +                       upper_layer->compat = be64_to_cpu(odf->compat);
> +                       upper_layer->ro_compat = be64_to_cpu(odf->ro_compat);
> +                       upper_layer->incompat = be64_to_cpu(odf->incompat);
> +                       upper_layer->feature = true;
> +                       kfree(odf);
> +               } else {
> +                       upper_layer->feature = false;
> +               }

Please pass ovl_layer * into ovl_get_feature[s]() and do all this
inside the helper - it is repeated for lower layers.

> +       }
> +
> +       /* Get features from each lower's root dir */
> +       for (i = 0; i < oe->numlower; i++) {
> +               struct ovl_path *lowerpath = &oe->lowerstack[i];
> +               struct ovl_layer *lower_layer = &ofs->lower_layers[i];
> +
> +               odf = ovl_get_feature(lowerpath->dentry);
> +               if (IS_ERR(odf))
> +                       return PTR_ERR(odf);
> +
> +               if (!odf) {
> +                       lower_layer->feature = false;
> +                       continue;
> +               }
> +
> +               lower_layer->compat = be64_to_cpu(odf->compat);
> +               lower_layer->ro_compat = be64_to_cpu(odf->ro_compat);
> +               lower_layer->incompat = be64_to_cpu(odf->incompat);
> +               lower_layer->feature = true;
> +               kfree(odf);
> +       }
> +
> +       return 0;
> +}
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index 7538b9b56237..745f3b89a99e 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -28,6 +28,7 @@ enum ovl_path_type {
>  #define OVL_XATTR_IMPURE OVL_XATTR_PREFIX "impure"
>  #define OVL_XATTR_NLINK OVL_XATTR_PREFIX "nlink"
>  #define OVL_XATTR_UPPER OVL_XATTR_PREFIX "upper"
> +#define OVL_XATTR_FEATURE OVL_XATTR_PREFIX "feature"
>
>  enum ovl_inode_flag {
>         /* Pure upper dir that may contain non pure upper entries */
> @@ -43,6 +44,17 @@ enum ovl_entry_flag {
>         OVL_E_CONNECTED,
>  };
>
> +/* Features */
> +#define OVL_FEATURE_MAGIC 0xfe
> +
> +/* On-disk format of overlay layer features */
> +struct ovl_d_feature {
u8 version; (it may be 2 if we want to relate to ovl format v2)

> +       u8 magic;               /* 0xfe */

u16 pad; (just for semantically defining the on-disk format zero padding)

> +       __be64 compat;          /* compatible features */
> +       __be64 ro_compat;       /* read-only compatible features */
> +       __be64 incompat;        /* incompatible features */
> +} __packed;
> +
>  /*
>   * The tuple (fh,uuid) is a universal unique identifier for a copy up origin,
>   * where:
> @@ -379,3 +391,6 @@ int ovl_set_origin(struct dentry *dentry, struct dentry *lower,
>
>  /* export.c */
>  extern const struct export_operations ovl_export_operations;
> +
> +/* feature.c */
> +int ovl_init_feature_set(struct ovl_fs *ofs, struct ovl_entry *oe);
> diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
> index 38ff6689a293..b1b6627f3350 100644
> --- a/fs/overlayfs/ovl_entry.h
> +++ b/fs/overlayfs/ovl_entry.h
> @@ -34,6 +34,11 @@ struct ovl_layer {
>         int idx;
>         /* One fsid per unique underlying sb (upper fsid == 0) */
>         int fsid;
> +       /* Features of this layer */
> +       u64 compat;
> +       u64 ro_compat;
> +       u64 incompat;
> +       bool feature;

Not that size of ovl_layer matters so much, but technically, the
boolean 'feature' can be represented with a single compat feature bit
if we define that the on-disk format is never initialized with all zeros,
so checking !layer->feature is equivalent to checking !layer->compat.

For example the feature OVL_FEATURE_COMPAT_V2
will always be set if features xattrs exists.
I write 'compat' only because old kernel *will* mount an overlay
with this feature set (not knowing to check it), but fsck.overlay
should not have any "official" version that does not check the
"V2" feature.

I did not yet look at all the patches to determine if the 'feature'
boolean is useful by itself, so not ruling it out completely.

Thanks,
Amir.