Re: [PATCH v5 06/10] libfs: Validate negative dentries in case-insensitive directories

[Gabriel Krisman Bertazi <krisman@suse.de>]

Eric Biggers <ebiggers@kernel.org> writes:

> On Fri, Aug 11, 2023 at 08:41:42PM -0400, Gabriel Krisman Bertazi wrote:
>> +	/*
>> +	 * Filesystems will call into d_revalidate without setting
>> +	 * LOOKUP_ flags even for file creation (see lookup_one*
>> +	 * variants).  Reject negative dentries in this case, since we
>> +	 * can't know for sure it won't be used for creation.
>> +	 */
>> +	if (!flags)
>> +		return 0;
>> +
>> +	/*
>> +	 * If the lookup is for creation, then a negative dentry can
>> +	 * only be reused if it's a case-sensitive match, not just a
>> +	 * case-insensitive one.  This is needed to make the new file be
>> +	 * created with the name the user specified, preserving case.
>> +	 */
>> +	if (flags & (LOOKUP_CREATE | LOOKUP_RENAME_TARGET)) {
>> +		/*
>> +		 * ->d_name won't change from under us in the creation
>> +		 * path only, since d_revalidate during creation and
>> +		 * renames is always called with the parent inode
>> +		 * locked.  It isn't the case for all lookup callpaths,
>> +		 * so ->d_name must not be touched 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;
>> +	}
>
> This is still really confusing to me.  Can you consider the below?  The code is
> the same except for the reordering, but the explanation is reworked to be much
> clearer (IMO).  Anything I am misunderstanding?
>
> 	/*
> 	 * If the lookup is for creation, then a negative dentry can only be
> 	 * reused if it's a case-sensitive match, not just a case-insensitive
> 	 * one.  This is needed to make the new file be created with the name
> 	 * the user specified, preserving case.
> 	 *
> 	 * LOOKUP_CREATE or LOOKUP_RENAME_TARGET cover most creations.  In these
> 	 * cases, ->d_name is stable and can be compared to 'name' without
> 	 * taking ->d_lock because the caller holds dir->i_rwsem for write.
> 	 * (This is because the directory lock blocks the dentry from being
> 	 * concurrently instantiated, and negative dentries are never moved.)
> 	 *
> 	 * All other creations actually use flags==0.  These come from the edge
> 	 * case of filesystems calling functions like lookup_one() that do a
> 	 * lookup without setting the lookup flags at all.  Such lookups might
> 	 * or might not be for creation, and if not don't guarantee stable
> 	 * ->d_name.  Therefore, invalidate all negative dentries when flags==0.
> 	 */
> 	if (flags & (LOOKUP_CREATE | LOOKUP_RENAME_TARGET)) {
> 		if (dentry->d_name.len != name->len ||
> 		    memcmp(dentry->d_name.name, name->name, name->len))
> 			return 0;
> 	}
> 	if (!flags)
> 		return 0;

I don't see it as particularly better or less confusing than the
original. but I also don't mind taking it into the next iteration.

-- 
Gabriel Krisman Bertazi