Al Viro <viro@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxx>
--
Gabriel Krisman Bertazi
