Re: [RFC PATCH 3/3] overlay: Add the ability to remount volatile directories when safe

[Amir Goldstein <amir73il@gmail.com>]

> > > +       inode_lock_nested(d_dirty->d_inode, I_MUTEX_XATTR);
> >
> > What's this lock for?
> >
> I need to take a lock on this inode to prevent modifications to it, right, or is
> getting the xattr safe?

No. see Documentation/filesystems/locking.rst.

>
> > > +       err = ovl_do_getxattr(d_dirty, OVL_XATTR_VOLATILE, &info, sizeof(info));
> > > +       inode_unlock(d_dirty->d_inode);
> > > +       if (err != sizeof(info))
> > > +               goto out_putdirty;
> > > +
> > > +       if (!uuid_equal(&overlay_boot_id, &info.overlay_boot_id)) {
> > > +               pr_debug("boot id has changed (reboot or module reloaded)\n");
> > > +               goto out_putdirty;
> > > +       }
> > > +
> > > +       if (d_dirty->d_inode->i_sb->s_instance_id != info.s_instance_id) {
> > > +               pr_debug("workdir has been unmounted and remounted\n");
> > > +               goto out_putdirty;
> > > +       }
> > > +
> > > +       err = errseq_check(&d_dirty->d_inode->i_sb->s_wb_err, info.errseq);
> > > +       if (err) {
> > > +               pr_debug("workdir dir has experienced errors: %d\n", err);
> > > +               goto out_putdirty;
> > > +       }
> >
> > Please put all the above including getxattr in helper ovl_verify_volatile_info()
> >
> Is it okay if the helper stays in super.c?
>

Yes.

>
> > > +
> > > +       /* Dirty file is okay, delete it. */
> > > +       ret = ovl_do_unlink(d_volatile->d_inode, d_dirty);
> >
> > That's a problem. By doing this, you have now approved a regular overlay
> > re-mount, but you need only approve a volatile overlay re-mount.
> > Need to pass ofs to ovl_workdir_cleanup{,_recurse}.
> >
> I can add a check to make sure this behaviour is only allowed on remounts back
> into volatile. There's technically a race condition here, where if there
> is an error between this check, and the mounting being finished, the FS
> could be dirty, but that already exists with the impl today.
>

If you follow my suggestion below and never unlink dirty file,
the filesystem will never be not-dirty so it is safer.

> > > +
> > > +out_putdirty:
> > > +       dput(d_dirty);
> > > +out_putvolatile:
> > > +       inode_unlock(d_volatile->d_inode);
> > > +       dput(d_volatile);
> > > +       return ret;
> > > +}
> > > +
> > > +/*
> > > + * check_incompat checks this specific incompat entry for incompatibility.
> > > + * If it is found to be incompatible -EINVAL will be returned.
> > > + *
> > > + * Any other -errno indicates an unknown error, and filesystem mounting
> > > + * should be aborted.
> > > + */
> > > +static int ovl_check_incompat(struct ovl_cache_entry *p, struct path *path)
> > > +{
> > > +       int err = -EINVAL;
> > > +
> > > +       if (!strcmp(p->name, OVL_VOLATILEDIR_NAME))
> > > +               err = ovl_check_incompat_volatile(p, path);
> > > +
> > > +       if (err == -EINVAL)
> > > +               pr_err("incompat feature '%s' cannot be mounted\n", p->name);
> > > +       else
> > > +               pr_debug("incompat '%s' handled: %d\n", p->name, err);
> > > +
> > > +       return err;
> > > +}
> > >
> > >  static int ovl_workdir_cleanup_recurse(struct path *path, int level)
> > >  {
> > > @@ -1098,10 +1175,9 @@ static int ovl_workdir_cleanup_recurse(struct path *path, int level)
> > >                         if (p->len == 2 && p->name[1] == '.')
> > >                                 continue;
> > >                 } else if (incompat) {
> > > -                       pr_err("overlay with incompat feature '%s' cannot be mounted\n",
> > > -                               p->name);
> > > -                       err = -EINVAL;
> > > -                       break;
> > > +                       err = ovl_check_incompat(p, path);
> > > +                       if (err)
> > > +                               break;
> >
> > The call to ovl_check_incompat here is too soon and it makes
> > you need to lookup both the volatile dir and dirty file.
> > What you need to do and let cleanup recurse into the next level
> > while letting it know that we are now traversing the "incompat"
> > subtree.
> >
> Maybe a dumb question but why is it incompat/volatile/dirty, rather than just
> incompat/volatile, where volatile is a file?

Not dumb. It's so old kernels cannot mount with this workdir,
because recursive cleanup never recursed more than 2 levels.
If it were just incompat/volatile old kernels would have cleaned it
and allowed it to mount.

> Are there any caveats with putting the xattr on the directory

Not that I can think of.

> , or alternatively are there any reasons not to make
> the structure incompat/volatile/dirty?
>

Old kernels as I wrote above.
The sole purpose of the dirty file is to cause rmdir on volatile to fail
in old kernels.

Thanks,
Amir.