On Mon, Nov 16, 2020 at 1:17 PM Amir Goldstein <amir73il@xxxxxxxxx> wrote:
>
> > > > + 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.
>
To clarify, as I wrote, there are two options.
The first option, as your patch did, removes the dirty file in
ovl_workdir_cleanup()
and re-creates it in ovl_make_workdir().
The second option, which I prefer, is to keep the dirty file, because until
syncfs was run these workdir/upperdir are dirty and should not be reused
should a crash happen after the dirty file removal.
But the second option means that ovl_workdir_cleanup() returns with
"work" directory not removed and ovl_workdir_create() is not prepared
for that.
My suggestion is to return 1 from ovl_workdir_cleanup,{_recurrsive}
for the case of successful cleanup with remaining and verified
work/incompat dir.
ovl_workdir_cleanup() should not call ovl_cleanup() which prints an
error in case ovl_workdir_cleanup_recurse() returned 1.
ovl_workdir_create() should goto out_unlock in case ovl_workdir_cleanup()
returned 1.
Hope I am not forgetting anything.
Thanks,
Amir.
