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

[Al Viro]

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.

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...