diff mbox series

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

Message ID 20250116052317.485356-6-viro@zeniv.linux.org.uk (mailing list archive)
State New
Headers show
Series [v2,01/20] make sure that DNAME_INLINE_LEN is a multiple of word size | expand

Commit Message

Al Viro Jan. 16, 2025, 5:23 a.m. UTC
... 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(-)

Comments

Gabriel Krisman Bertazi Jan. 16, 2025, 3:38 p.m. UTC | #1
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);
>  }
Al Viro Jan. 16, 2025, 3:46 p.m. UTC | #2
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.
Gabriel Krisman Bertazi Jan. 16, 2025, 3:53 p.m. UTC | #3
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>
diff mbox series

Patch

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.
+	 * 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();
 	}
-	qstr.len = len;
-	qstr.name = str;
 
 	return utf8_strncasecmp(dentry->d_sb->s_encoding, name, &qstr);
 }