On Tue, Sep 1, 2020 at 12:17 PM Miklos Szeredi <miklos@xxxxxxxxxx> wrote:
>
> 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...
When called from ovl_indexdir_cleanup(), path->dentry->d_name.name[0] == '#',
because cleanup starts at level 1 and ovl_workdir_cleanup_recurse() is called
with level 2.
>
> >
> > 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.
I usually prefer to use EINVAL for illegal user input and this is a border line,
so I prefer errors that indicate the state of the object, like EEXIST
or ENOTEMPTY,
but because these errors are not expected on mount, I can live with EINVAL.
Assuming that you agree with my response to ovl_indexdir_cleanup(), let
me know if you need me to post v3 or if you can change the choice of error
and s/pr_warn/pr_err on commit.
Thanks,
Amir.
