diff mbox series

[f2fs-dev,v2,3/7] libfs: Validate negative dentries in case-insensitive directories

Message ID 20230422000310.1802-4-krisman@suse.de (mailing list archive)
State Superseded
Headers show
Series Support negative dentries on case-insensitive ext4 and f2fs | expand

Commit Message

Gabriel Krisman Bertazi April 22, 2023, 12:03 a.m. UTC 
From: Gabriel Krisman Bertazi <krisman@collabora.com>

Introduce a dentry revalidation helper to be used by case-insensitive
filesystems to check if it is safe to reuse a negative dentry.

A negative dentry is safe to be reused on a case-insensitive lookup if
it was created during a case-insensitive lookup and this is not a lookup
that will instantiate a dentry. If this is a creation lookup, we also
need to make sure the name matches sensitively the name under lookup in
order to assure the name preserving semantics.

dentry->d_name is only checked by the case-insensitive d_revalidate hook
in the LOOKUP_CREATE/LOOKUP_RENAME_TARGET case since, for these cases,
d_revalidate is always called with the parent inode locked, and
therefore the name cannot change from under us.

d_revalidate is only called in 4 places: lookup_dcache, __lookup_slow,
lookup_open and lookup_fast:

  - lookup_dcache always calls it with zeroed flags, with the exception
  of when coming from __lookup_hash, which needs the parent locked
  already, for instance in the open/creation path, which is locked in
  open_last_lookups.

  - In __lookup_slow, either the parent inode is locked by the
  caller (lookup_slow), or it is called with no
  flags (lookup_one/lookup_one_len).

  - lookup_open also requires the parent to be locked in the creation
  case, which is done in open_last_lookups.

  - lookup_fast will indeed be called with the parent unlocked, but it
  shouldn't be called with LOOKUP_CREATE.  Either it is called in the
  link_path_walk, where nd->flags doesn't have LOOKUP_CREATE yet or in
  open_last_lookups. But, in this case, it also never has LOOKUP_CREATE,
  because it is only called on the !O_CREAT case, which means op->intent
  doesn't have LOOKUP_CREAT (set in build_open_flags only if O_CREAT is
  set).

Finally, for the LOOKUP_RENAME_TARGET, we are doing a rename, so the
parents inodes are also be locked.

Reviewed-by: Theodore Ts'o <tytso@mit.edu>
Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com>
---
 fs/libfs.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

Comments

Eric Biggers July 14, 2023, 5 a.m. UTC | #1 
On Fri, Apr 21, 2023 at 08:03:06PM -0400, Gabriel Krisman Bertazi wrote:
> diff --git a/fs/libfs.c b/fs/libfs.c
> index 4eda519c3002..f8881e29c5d5 100644
> --- a/fs/libfs.c
> +++ b/fs/libfs.c
> @@ -1467,9 +1467,43 @@ static int generic_ci_d_hash(const struct dentry *dentry, struct qstr *str)
>  	return 0;
>  }
>  
> +static inline int generic_ci_d_revalidate(struct dentry *dentry,
> +					  const struct qstr *name,
> +					  unsigned int flags)
> +{
> +	int is_creation = flags & (LOOKUP_CREATE | LOOKUP_RENAME_TARGET);
> +
> +	if (d_is_negative(dentry)) {
> +		const struct dentry *parent = READ_ONCE(dentry->d_parent);
> +		const struct inode *dir = READ_ONCE(parent->d_inode);
> +
> +		if (dir && needs_casefold(dir)) {
> +			if (!d_is_casefold_lookup(dentry))
> +				return 0;

A comment that explains why the !d_is_casefold_lookup() check is needed would be
helpful.  I know it's in the commit message, but that's not enough.

> +
> +			if (is_creation) {
> +				/*
> +				 * dentry->d_name won't change from under us in
> +				 * the is_creation path only, since d_revalidate
> +				 * during creation and renames is always called
> +				 * with the parent inode locked.  This isn't the
> +				 * case for all lookup callpaths, so it should
> +				 * not be accessed outside
> +				 * (LOOKUP_CREATE|LOOKUP_RENAME_TARGET) context.
> +				 */
> +				if (dentry->d_name.len != name->len ||
> +				    memcmp(dentry->d_name.name, name->name, name->len))
> +					return 0;
> +			}
> +		}
> +	}
> +	return 1;
> +}

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?

- Eric
Gabriel Krisman Bertazi July 18, 2023, 4:47 p.m. UTC | #2 
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.
diff mbox series

Patch

diff --git a/fs/libfs.c b/fs/libfs.c
index 4eda519c3002..f8881e29c5d5 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -1467,9 +1467,43 @@  static int generic_ci_d_hash(const struct dentry *dentry, struct qstr *str)
 	return 0;
 }
 
+static inline int generic_ci_d_revalidate(struct dentry *dentry,
+					  const struct qstr *name,
+					  unsigned int flags)
+{
+	int is_creation = flags & (LOOKUP_CREATE | LOOKUP_RENAME_TARGET);
+
+	if (d_is_negative(dentry)) {
+		const struct dentry *parent = READ_ONCE(dentry->d_parent);
+		const struct inode *dir = READ_ONCE(parent->d_inode);
+
+		if (dir && needs_casefold(dir)) {
+			if (!d_is_casefold_lookup(dentry))
+				return 0;
+
+			if (is_creation) {
+				/*
+				 * dentry->d_name won't change from under us in
+				 * the is_creation path only, since d_revalidate
+				 * during creation and renames is always called
+				 * with the parent inode locked.  This isn't the
+				 * case for all lookup callpaths, so it should
+				 * not be accessed outside
+				 * (LOOKUP_CREATE|LOOKUP_RENAME_TARGET) context.
+				 */
+				if (dentry->d_name.len != name->len ||
+				    memcmp(dentry->d_name.name, name->name, name->len))
+					return 0;
+			}
+		}
+	}
+	return 1;
+}
+
 static const struct dentry_operations generic_ci_dentry_ops = {
 	.d_hash = generic_ci_d_hash,
 	.d_compare = generic_ci_d_compare,
+	.d_revalidate_name = generic_ci_d_revalidate,
 };
 #endif