On Fri, Jul 21, 2023 at 04:16:30PM -0400, Gabriel Krisman Bertazi wrote:
> Eric Biggers <ebiggers@xxxxxxxxxx> writes:
>
> > On Wed, Jul 19, 2023 at 11:06:57PM -0700, Eric Biggers wrote:
> >>
> >> I'm also having trouble understanding exactly when ->d_name is stable here.
> >> AFAICS, unfortunately the VFS has an edge case where a dentry can be moved
> >> without its parent's ->i_rwsem being held. It happens when a subdirectory is
> >> "found" under multiple names. The VFS doesn't support directory hard links, so
> >> if it finds a second link to a directory, it just moves the whole dentry tree to
> >> the new location. This can happen if a filesystem image is corrupted and
> >> contains directory hard links. Coincidentally, it can also happen in an
> >> encrypted directory due to the no-key name => normal name transition...
> >
> > Sorry, I think I got this slightly wrong. The move does happen with the
> > parent's ->i_rwsem held, but it's for read, not for write. First, before
> > ->lookup is called, the ->i_rwsem of the parent directory is taken for read.
> > ->lookup() calls d_splice_alias() which can call __d_unalias() which does the
> > __d_move(). If the old alias is in a different directory (which cannot happen
> > in that fscrypt case, but can happen in the general "directory hard links"
> > case), __d_unalias() takes that directory's ->i_rwsem for read too.
> >
> > So it looks like the parent's ->i_rwsem does indeed exclude moves of child
> > dentries, but only if it's taken for *write*. So I guess you can rely on that;
> > it's just a bit more subtle than it first appears. Though, some of your
> > explanation seems to assume that a read lock is sufficient ("In __lookup_slow,
> > either the parent inode is locked by the caller (lookup_slow) ..."), so maybe
> > there is still a problem.
>
> I think I'm missing something on your clarification. I see your point
> about __d_unalias, and I see in the case where alias->d_parent !=
> dentry->d_parent we acquire the parent inode read lock:
>
> static int __d_unalias(struct inode *inode,
> struct dentry *dentry, struct dentry *alias)
> {
> ...
> m1 = &dentry->d_sb->s_vfs_rename_mutex;
> if (!inode_trylock_shared(alias->d_parent->d_inode))
> goto out_err;
> }
>
> And it seems to use that for __d_move. In this case, __d_move changes
> from under us even with a read lock, which is dangerous. I think I
> agree with your first email more than the clarification.
>
> In the lookup_slow then:
>
> lookup_slow()
> d_lookup()
> d_splice_alias()
> __d_unalias()
> __d_move()
>
> this __d_move Can do a dentry move and race with d_revalidate even
> though it has the parent read lock.
>
> > So it looks like the parent's ->i_rwsem does indeed exclude moves of child
> > dentries, but only if it's taken for *write*. So I guess you can rely on that;
>
> We can get away of it with acquiring the d_lock as you suggested, I
> think. But can you clarify the above? I wanna make sure I didn't miss
> anything. I am indeed relying only on the read lock here, as you can see.
In my first email I thought that __d_move() can be called without the parent
inode's i_rwsem held at all. In my second email I realized that it is always
called with at least a read (shared) lock.
The question is what kind of parent i_rwsem lock is guaranteed to be held by the
caller of ->d_revalidate() when the name comparison is done. Based on the
above, it needs to be at least a write (exclusive) lock to exclude __d_move()
without taking d_lock. However, your analysis (in the commit message of "libfs:
Validate negative dentries in case-insensitive directories") only talks about
i_rwsem being "taken", without saying whether it's for read or write. One thing
you mentioned as taking i_rwsem is lookup_slow, but that only takes it for read.
That seems like a problem, as it makes your analysis not correct.
- Eric
