On Sun, Aug 30, 2020 at 10:28 PM Amir Goldstein <amir73il@xxxxxxxxx> wrote:
>
> An incompatible feature is marked by a non-empty directory nested
> 2 levels deep under "work" dir, e.g.:
> workdir/work/incompat/volatile.
>
> This commit checks for marked incompat features, warns about them
> and fails to mount the overlay, for example:
> overlayfs: overlay with incompat feature 'volatile' cannot be mounted
>
> Very old kernels (i.e. v3.18) will fail to remove a non-empty "work"
> dir and fail the mount. Newer kernels will fail to remove a "work"
> dir with entries nested 3 levels and fall back to read-only mount.
>
> User mounting with old kernel will see a warning like these in dmesg:
> overlayfs: cleanup of 'incompat/...' failed (-39)
> overlayfs: cleanup of 'work/incompat' failed (-39)
> overlayfs: cleanup of 'ovl-work/work' failed (-39)
> overlayfs: failed to create directory /vdf/ovl-work/work (errno: 17);
> mounting read-only
>
> These warnings should give the hint to the user that:
> 1. mount failure is caused by backward incompatible features
> 2. mount failure can be resolved by manually removing the "work" directory
>
> There is nothing preventing users on old kernels from manually removing
> workdir entirely or mounting overlay with a new workdir, so this is in
> no way a full proof backward compatibility enforcement, but only a best
> effort.
>
> Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
> ---
>
> Vivek,
>
> Re-posting with minor cleanups.
> Also pushed to branch ovl-incompat.
>
> Thanks,
> Amir.
>
> Changes since v1:
> - Move check for "incompat" name to ovl_workdir_cleanup_recurse()
>
>
> fs/overlayfs/readdir.c | 30 +++++++++++++++++++++++++-----
> fs/overlayfs/super.c | 25 ++++++++++++++++++-------
> 2 files changed, 43 insertions(+), 12 deletions(-)
>
> diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
> index 6918b98faeb6..683c6f27ab77 100644
> --- a/fs/overlayfs/readdir.c
> +++ b/fs/overlayfs/readdir.c
> @@ -1051,7 +1051,9 @@ int ovl_check_d_type_supported(struct path *realpath)
> return rdd.d_type_supported;
> }
>
> -static void ovl_workdir_cleanup_recurse(struct path *path, int level)
> +#define OVL_INCOMPATDIR_NAME "incompat"
> +
> +static int ovl_workdir_cleanup_recurse(struct path *path, int level)
> {
> int err;
> struct inode *dir = path->dentry->d_inode;
> @@ -1065,6 +1067,15 @@ static void ovl_workdir_cleanup_recurse(struct path *path, int level)
> .root = &root,
> .is_lowest = false,
> };
> + bool incompat = false;
> +
> + /*
> + * The "work/incompat" directory is treated specially - if it is not
> + * empty, instead of printing a generic error and mounting read-only,
> + * we will error about incompat features and fail the mount.
> + */
> + if (level == 2 && !strcmp(path->dentry->d_name.name, OVL_INCOMPATDIR_NAME))
> + incompat = true;
Should the test be specific to "work/incompat"? AFAICS this will
trigger under "index" as well...
>
> err = ovl_dir_read(path, &rdd);
> if (err)
> @@ -1079,21 +1090,29 @@ static void ovl_workdir_cleanup_recurse(struct path *path, int level)
> continue;
> if (p->len == 2 && p->name[1] == '.')
> continue;
> + } else if (incompat) {
> + pr_warn("overlay with incompat feature '%.*s' cannot be mounted\n",
> + p->len, p->name);
> + err = -EEXIST;
EEXIST feels counterintuitive. I'd rather opt for EINVAL.
Thanks,
Miklos
