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). 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.
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
