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 |
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); > }
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.
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 --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); }
... 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(-)