diff mbox series

[3/7] libfs: Validate negative dentries in case-insensitive directories

Message ID 20220622194603.102655-4-krisman@collabora.com (mailing list archive)
State New, archived
Headers show
Series Support negative dentries on case-insensitive directories | expand

Commit Message

Gabriel Krisman Bertazi June 22, 2022, 7:45 p.m. UTC
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.

Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com>
---
 fs/libfs.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

Comments

Theodore Ts'o March 23, 2023, 2:36 p.m. UTC | #1
On Wed, Jun 22, 2022 at 03:45:59PM -0400, Gabriel Krisman Bertazi wrote:
> 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.
> 
> Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com>

Reviewed-by: Theodore Ts'o <tytso@mit.edu>
Al Viro March 26, 2023, 4:46 a.m. UTC | #2
On Wed, Jun 22, 2022 at 03:45:59PM -0400, Gabriel Krisman Bertazi wrote:

> +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;

	In which conditions does that happen?

> +			if (is_creation &&
> +			    (dentry->d_name.len != name->len ||
> +			     memcmp(dentry->d_name.name, name->name, name->len)))
> +				return 0;
> +		}
> +	}
> +	return 1;
> +}

	Analysis of stability of ->d_name, please.  It's *probably* safe, but
the details are subtle and IMO should be accompanied by several asserts.
E.g. "we never get LOOKUP_CREATE in op->intent without O_CREAT in op->open_flag
for such and such reasons, and we verify that in such and such place"...

	A part of that would be "the call in lookup_dcache() can only get there
with non-zero flags when coming from __lookup_hash(), and that has parent locked,
stabilizing the name; the same goes for the call in __lookup_slow(), with the
only call chain with possibly non-zero flags is through lookup_slow(), where we
have the parent locked".  However, lookup_fast() and lookup_open() have the
flags come from nd->flags, and LOOKUP_CREATE can be found there in several areas.
I _think_ we are guaranteed the parent locked in all such call chains, but that
is definitely worth at least a comment.
Gabriel Krisman Bertazi March 31, 2023, 3:31 p.m. UTC | #3
Al Viro <viro@zeniv.linux.org.uk> writes:

> On Wed, Jun 22, 2022 at 03:45:59PM -0400, Gabriel Krisman Bertazi wrote:
>
>> +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;
>
> 	In which conditions does that happen?

Hi Al,

This can happen right after a case-sensitive directory is converted to
case-insensitive. A previous case-sensitive lookup could have left a
negative dentry in the dcache that we need to reject, because it doesn't
have the same assurance of absence of all-variation of names as a
negative dentry created during a case-insensitive lookup.

>> +			if (is_creation &&
>> +			    (dentry->d_name.len != name->len ||
>> +			     memcmp(dentry->d_name.name, name->name, name->len)))
>> +				return 0;
>> +		}
>> +	}
>> +	return 1;
>> +}
>
> 	Analysis of stability of ->d_name, please.  It's *probably* safe, but
> the details are subtle and IMO should be accompanied by several asserts.
> E.g. "we never get LOOKUP_CREATE in op->intent without O_CREAT in op->open_flag
> for such and such reasons, and we verify that in such and such place"...
>
> 	A part of that would be "the call in lookup_dcache() can only get there
> with non-zero flags when coming from __lookup_hash(), and that has parent locked,
> stabilizing the name; the same goes for the call in __lookup_slow(), with the
> only call chain with possibly non-zero flags is through lookup_slow(), where we
> have the parent locked".  However, lookup_fast() and lookup_open() have the
> flags come from nd->flags, and LOOKUP_CREATE can be found there in several areas.
> I _think_ we are guaranteed the parent locked in all such call chains, but that
> is definitely worth at least a comment.

Thanks for the example of the analysis what you are looking for here.
That will help me quite a bit.  I wrote this code a while ago and I
don't recall the exact details.  I will go through the code again and
send a new version with the detailed analysis.
diff mbox series

Patch

diff --git a/fs/libfs.c b/fs/libfs.c
index 31b0ddf01c31..de43f3f585f1 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -1450,9 +1450,33 @@  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.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