linux-kernel - Re: [PATCH 13/19] VFS: lock dentry for ->revalidate to avoid races with rename etc

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: <20250208013513.GO1977892@ZenIV>
Date: Sat, 8 Feb 2025 01:35:13 +0000
From: Al Viro <viro@...iv.linux.org.uk>
To: NeilBrown <neilb@...e.de>
Cc: Christian Brauner <brauner@...nel.org>, Jan Kara <jack@...e.cz>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Jeff Layton <jlayton@...nel.org>,
	Dave Chinner <david@...morbit.com>, linux-fsdevel@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 13/19] VFS: lock dentry for ->revalidate to avoid races
 with rename etc

On Sat, Feb 08, 2025 at 01:30:43AM +0000, Al Viro wrote:
> On Thu, Feb 06, 2025 at 04:42:50PM +1100, NeilBrown wrote:
> > When we call ->revalidate we want to be sure we are revalidating the
> > expected name.  As a shared lock on i_rwsem no longer prevents renames
> > we need to lock the dentry and ensure it still has the expected name.
> 
> *blink*
> 
> We never had been guaranteed any lock on the parent - the most common
> call chain doesn't (and didn't) have it taken.
> 
> > So pass parent name to d_revalidate() and be prepared to retry the
> > lookup if it returns -EAGAIN.
> 
> I don't understand that one at all.  What's the point of those retries
> on -EAGAIN?  Rename (or race with d_splice_alias(), for that matter)
> can happen just as we return success from ->d_revalidate(), so we
> don't get anything useful out of that check.
> 
> What's more, why do we need that exclusion in the first place?
> The instance *is* given a stable parent reference and stable name,
> so there's no need for it to even look at ->d_parent or ->d_name.
> 
> It looks like a bad rebase on top of ->d_revalidate() series that
> had landed in -rc1, with the original variant trying to provide the
> guarantees now offered by that series.
> 
> Unless there's something subtle I'm missing here, I would suggest
> dropping that one.  Incidentally, d_update_trylock() would be
> better off in fs/dcache.c - static and with just one argument.

Sorry, lost a sentence here while editing:

The only remaining caller of d_update_trylock() would be the one in
__d_unalias(), just before the call of ->d_unalias_trylock() in there
and it gets NULL/NULL in the last two arguments.

> HOWEVER, if you do not bother with doing that before ->d_unalias_trylock()
> (and there's no reason to do that), the whole thing becomes much simpler -
> you can do the check inside __d_move(), after all locks had been taken.
> 
> After
>         spin_lock_nested(&dentry->d_lock, 2);
>         spin_lock_nested(&target->d_lock, 3);
> you have everything stable.  Just make the sucker return bool instead
> of void, check that crap and have it return false if there's a problem.
> 
> Callers other than __d_unalias() would just do WARN_ON(!__d_move(...))
> instead of their __d_move() calls and __d_unalias() would have
> 	if (__d_move(...))
> 		ret = 0;
> and screw the d_update_trylock/d_update_unlock there.
> 
> All there is to it...