Re: [PATCH] fs: Protect reconfiguration of sb read-write from racing writes

[Dave Chinner <david@fromorbit.com>]

On Fri, Jun 16, 2023 at 06:37:00PM +0200, Jan Kara wrote:
> On Fri 16-06-23 08:36:53, Dave Chinner wrote:
> > On Thu, Jun 15, 2023 at 01:38:48PM +0200, Jan Kara wrote:
> > > The reconfigure / remount code takes a lot of effort to protect
> > > filesystem's reconfiguration code from racing writes on remounting
> > > read-only. However during remounting read-only filesystem to read-write
> > > mode userspace writes can start immediately once we clear SB_RDONLY
> > > flag. This is inconvenient for example for ext4 because we need to do
> > > some writes to the filesystem (such as preparation of quota files)
> > > before we can take userspace writes so we are clearing SB_RDONLY flag
> > > before we are fully ready to accept userpace writes and syzbot has found
> > > a way to exploit this [1]. Also as far as I'm reading the code
> > > the filesystem remount code was protected from racing writes in the
> > > legacy mount path by the mount's MNT_READONLY flag so this is relatively
> > > new problem. It is actually fairly easy to protect remount read-write
> > > from racing writes using sb->s_readonly_remount flag so let's just do
> > > that instead of having to workaround these races in the filesystem code.
> ...
> > > +	} else if (remount_rw) {
> > > +		/*
> > > +		 * We set s_readonly_remount here to protect filesystem's
> > > +		 * reconfigure code from writes from userspace until
> > > +		 * reconfigure finishes.
> > > +		 */
> > > +		sb->s_readonly_remount = 1;
> > > +		smp_wmb();
> > 
> > What does the magic random memory barrier do? What is it ordering,
> > and what is it paired with?
> > 
> > This sort of thing is much better done with small helpers that
> > encapsulate the necessary memory barriers:
> > 
> > sb_set_readonly_remount()
> > sb_clear_readonly_remount()
> > 
> > alongside the helper that provides the read-side check and memory
> > barrier the write barrier is associated with.
> 
> Fair remark. The new code including barrier just copies what happens a few
> lines above for remount read-only case (and what happens ib several other
> places throughout VFS code).

Yes, I saw that you'd copied that magic memory barrier. Good for
consistency, but it doesn't answer any of the above questions
either, so....

> I agree having helpers for this and actually
> documenting how the barriers are matching there is a good cleanup.
> 
> > I don't often ask for code to be cleaned up before a bug fix can be
> > added, but I think this is one of the important cases where it does
> > actually matter - we should never add undocumented memory barriers
> > in the code like this...
> 
> I've talked to Christian and we'll queue this as a separate cleanup. I'll
> post it shortly.

Thanks!

-Dave.
-- 
Dave Chinner
david@fromorbit.com