Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Gabriel Krisman Bertazi <krisman@suse.de>
To:     Eric Biggers <ebiggers@kernel.org>
Cc:     brauner@kernel.org, tytso@mit.edu,
        linux-f2fs-devel@lists.sourceforge.net, viro@zeniv.linux.org.uk,
        linux-fsdevel@vger.kernel.org, jaegeuk@kernel.org,
        linux-ext4@vger.kernel.org,
        Gabriel Krisman Bertazi <krisman@collabora.com>
Subject: Re: [PATCH v3 3/7] libfs: Validate negative dentries in
 case-insensitive directories
Organization: SUSE
References: <20230719221918.8937-1-krisman@suse.de>
        <20230719221918.8937-4-krisman@suse.de>
        <20230720060657.GB2607@sol.localdomain>
        <20230720064103.GC2607@sol.localdomain>
Date:   Fri, 21 Jul 2023 16:16:30 -0400
In-Reply-To: <20230720064103.GC2607@sol.localdomain> (Eric Biggers's message
        of "Wed, 19 Jul 2023 23:41:03 -0700")
Message-ID: <87bkg53tr5.fsf@suse.de>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/28.2 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

Eric Biggers <ebiggers@kernel.org> 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.

-- 
Gabriel Krisman Bertazi