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:

> ... and check the "name might be unstable" predicate
> the right way.
>
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---
>  fs/libfs.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/fs/libfs.c b/fs/libfs.c
> index 748ac5923154..3ad1b1b7fed6 100644
> --- a/fs/libfs.c
> +++ b/fs/libfs.c
> @@ -1789,7 +1789,7 @@ int generic_ci_d_compare(const struct dentry *dentry, unsigned int len,
>  {
>  	const struct dentry *parent;
>  	const struct inode *dir;
> -	char strbuf[DNAME_INLINE_LEN];
> +	union shortname_store strbuf;
>  	struct qstr qstr;
>  
>  	/*
> @@ -1809,22 +1809,23 @@ int generic_ci_d_compare(const struct dentry *dentry, unsigned int len,
>  	if (!dir || !IS_CASEFOLDED(dir))
>  		return 1;
>  
> +	qstr.len = len;
> +	qstr.name = str;
>  	/*
>  	 * 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.

>  	}
> -	qstr.len = len;
> -	qstr.name = str;
>  
>  	return utf8_strncasecmp(dentry->d_sb->s_encoding, name, &qstr);
>  }

-- 
Gabriel Krisman Bertazi