> > > + 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.
