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
Subject: Re: [f2fs-dev] [PATCH v2 3/7] libfs: Validate negative dentries in
 case-insensitive directories
References: <20230422000310.1802-1-krisman@suse.de>
        <20230422000310.1802-4-krisman@suse.de>
        <20230714050028.GC913@sol.localdomain>
Date:   Tue, 18 Jul 2023 12:47:25 -0400
In-Reply-To: <20230714050028.GC913@sol.localdomain> (Eric Biggers's message of
        "Thu, 13 Jul 2023 22:00:28 -0700")
Message-ID: <87pm4p5fqa.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:

> I notice that the existing vfat_revalidate_ci() in fs/fat/namei_vfat.c behaves
> differently in the 'flags == 0' case:
>
>
> 	/*
> 	 * This may be nfsd (or something), anyway, we can't see the
> 	 * intent of this. So, since this can be for creation, drop it.
> 	 */
> 	if (!flags)
> 		return 0;
>
> I don't know whether that's really needed, but have you thought about this?

Hi Eric,

I didn't look much into it before because, as you know, the vfat
case-insensitive implementation is completely different than the
ext4/f2fs code. But I think you are on to something.

The original intent of this check was to safeguard against the case
where d_revalidate would be called without nameidata from the filesystem
helpers. The filesystems don't give the purpose of the lookup
(nd->flags) so there is no way to tell if the dentry is being used for
creation, and therefore we can't rely on the negative dentry for ci. The
path is like this:

lookup_one_len(...)
  __lookup_hash(..., nd = NULL)
     cached_lookup(...)
       do_revalidate(parent, name, nd)
         dentry->d_op->d_revalidate(parent, nd);

Then !nd was dropped to pass flags directly around 2012, which
overloaded the flags meaning. Which means, d_revalidate can
still be called for creation without (LOOKUP_CREATE|...). For
instance, in nfsd_create.  I wasn't considering this.

This sucks, because we don't have enough information to avoid the name
check in this case, so we'd also need memcmp there.  Except it won't be
safe. because callers won't necessarily hold the parent lock in the path
below.

 lookup_one_unlocked()
   lookup_dcache()
      d_revalidate()  // called unlocked

Thus, I'll have to add a similar:

  if (!flags)
    return 0;

Ahead of the is_creation check.  It will solve it.

But i think the issue is in VFS.  the lookup_one_* functions should have
proper lookup flags, such that d_revalidate can tell the purpose of the
lookup.

-- 
Gabriel Krisman Bertazi