linux-kernel - Re: [RFC v3 1/1] fs/namespace: remove RCU sync for MNT

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240701-zauber-holst-1ad7cadb02f9@brauner>
Date: Mon, 1 Jul 2024 07:50:24 +0200
From: Christian Brauner <brauner@...nel.org>
To: Ian Kent <ikent@...hat.com>
Cc: Alexander Larsson <alexl@...hat.com>, Jan Kara <jack@...e.cz>, 
	Matthew Wilcox <willy@...radead.org>, Lucas Karpinski <lkarpins@...hat.com>, viro@...iv.linux.org.uk, 
	raven@...maw.net, linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Eric Chanudet <echanude@...hat.com>
Subject: Re: [RFC v3 1/1] fs/namespace: remove RCU sync for MNT_DETACH umount

> I always thought the rcu delay was to ensure concurrent path walks "see" the
> 
> umount not to ensure correct operation of the following mntput()(s).
> 
> 
> Isn't the sequence of operations roughly, resolve path, lock, deatch,
> release
> 
> lock, rcu wait, mntput() subordinate mounts, put path.

The crucial bit is really that synchronize_rcu_expedited() ensures that
the final mntput() won't happen until path walk leaves RCU mode.

This allows caller's like legitimize_mnt() which are called with only
the RCU read-lock during lazy path walk to simple check for
MNT_SYNC_UMOUNT and see that the mnt is about to be killed. If they see
that this mount is MNT_SYNC_UMOUNT then they know that the mount won't
be freed until an RCU grace period is up and so they know that they can
simply put the reference count they took _without having to actually
call mntput()_.

Because if they did have to call mntput() they might end up shutting the
filesystem down instead of umount() and that will cause said EBUSY
errors I mentioned in my earlier mails.

> 
> 
> So the mount gets detached in the critical section, then we wait followed by
> 
> the mntput()(s). The catch is that not waiting might increase the likelyhood
> 
> that concurrent path walks don't see the umount (so that possibly the umount
> 
> goes away before the walks see the umount) but I'm not certain. What looks
> to
> 
> be as much of a problem is mntput() racing with a concurrent mount beacase
> while
> 
> the detach is done in the critical section the super block instance list
> deletion
> 
> is not and the wait will make the race possibility more likely. What's more

Concurrent mounters of the same filesystem will wait for each other via
grab_super(). That has it's own logic based on sb->s_active which goes
to zero when all mounts are gone.

> 
> mntput() delegates the mount cleanup (which deletes the list instance) to a
> 
> workqueue job so this can also occur serially in a following mount command.

No, that only happens when it's a kthread. Regular umount() call goes
via task work which finishes before the caller returns to userspace
(same as closing files work).

> 
> 
> In fact I might have seen exactly this behavior in a recent xfs-tests run
> where I
> 
> was puzzled to see occasional EBUSY return on mounting of mounts that should
> not
> 
> have been in use following their umount.

That's usually very much other bugs. See commit 2ae4db5647d8 ("fs: don't
misleadingly warn during thaw operations") in vfs.fixes for example.

> 
> 
> So I think there are problems here but I don't think the removal of the wait
> for
> 
> lazy umount is the worst of it.
> 
> 
> The question then becomes, to start with, how do we resolve this unjustified
> EBUSY
> 
> return. Perhaps a completion (used between the umount and mount system
> calls) would
> 
> work well here?

Again, this already exists deeper down the stack...