Re: [PATCH v2 06/20] generic_ci_d_compare(): use shortname_storage

[Gabriel Krisman Bertazi <gabriel@krisman.be>]

Al Viro <viro@zeniv.linux.org.uk> writes:

> On Thu, Jan 16, 2025 at 10:38:53AM -0500, Gabriel Krisman Bertazi wrote:
>> >  	 * If the dentry name is stored in-line, then it may be concurrently
>> >  	 * modified by a rename.  If this happens, the VFS will eventually retry
>> >  	 * the lookup, so it doesn't matter what ->d_compare() returns.
>> >  	 * However, it's unsafe to call utf8_strncasecmp() with an unstable
>> >  	 * string.  Therefore, we have to copy the name into a temporary buffer.
>> 
>> This part of the comment needs updating since there is no more copying.
>> 
>> > +	 * As above, len is guaranteed to match str, so the shortname case
>> > +	 * is exactly when str points to ->d_shortname.
>> >  	 */
>> > -	if (len <= DNAME_INLINE_LEN - 1) {
>> > -		memcpy(strbuf, str, len);
>> > -		strbuf[len] = 0;
>> > -		str = strbuf;
>> > +	if (qstr.name == dentry->d_shortname.string) {
>> > +		strbuf = dentry->d_shortname; // NUL is guaranteed to be in there
>> > +		qstr.name = strbuf.string;
>> >  		/* prevent compiler from optimizing out the temporary buffer */
>> >  		barrier();
>> 
>> If I read the code correctly, I admit I don't understand how this
>> guarantees the stability.  Aren't you just assigning qstr.name back the
>> same value it had in case of an inlined name through a bounce pointer?
>> The previous implementation made sense to me, since the memcpy only
>> accessed each character once, and we guaranteed the terminating
>> character explicitly, but I'm having a hard time with this version.
>
> This
> 		strbuf = dentry->d_shortname; // NUL is guaranteed to be in there
> copies the entire array.  No bounce pointers of any sort; we copy
> the array contents, all 40 bytes of it.  And yes, struct (or union,
> in this case) assignment generates better code than manual memcpy()
> here.

Ah. I read that as:

unsigned char *strbuf = &dentry->d_shortname

Thanks for explaining.  Makes sense to me:

Reviewed-by: Gabriel Krisman Bertazi <gabriel@krisman.be>

-- 
Gabriel Krisman Bertazi