On Tue, 2026-06-16 at 08:51 +0100, David Laight wrote: > On Mon, 15 Jun 2026 23:33:09 -0700 > Viacheslav Dubeyko <[email protected]> wrote: > > > On Wed, 2026-06-10 at 21:18 -0700, Darrick J. Wong wrote: > > > On Wed, Jun 10, 2026 at 08:50:33PM -0700, Viacheslav Dubeyko > > > wrote: > > > > On Mon, 2026-06-08 at 10:55 +0100, > > > > [email protected] wrote: > > > > > From: David Laight <[email protected]> > > > > > > > > > > xattr_name is kmalloc()ed at the (assumed) maximal size and > > > > > then > > > > > the > > > > > prefix > > > > > and name concatenated together. > > > > > Use memcpy() for the prefix - its length is passed and > > > > > strscpy() > > > > > for > > > > > the > > > > > name to ensure it really doesnt overflow. > > > > > > > > > > Prior to bf29e886b242c the buffers were smaller and on-stack. > > > > > (But I cant see the copy in the old code.) > > > > > I am also not sure why the buffer isnt created "just long > > > > > enough". > > > > > > > > > > Signed-off-by: David Laight <[email protected]> > > > > > --- > > > > > This is one of a group of patches that remove potentially > > > > > unbounded > > > > > strcpy() calls. > > > > > > > > > > They are mostly replaced by strscpy() or, when strlen() has > > > > > just > > > > > been > > > > > called, with memcpy() (usually including the '\0'). > > > > > > > > > > Calls with copy string literals into arrays are left > > > > > unchanged. > > > > > They are safe and easily detected as such. > > > > > > > > > > The changes were made by getting the compiler to detect the > > > > > calls > > > > > and > > > > > then fixing the code by hand. > > > > > > > > > > Note that all the changes are only compile tested. > > > > > > > > > > Some Makefiles were changed to allow files to contain > > > > > strcpy(). > > > > > As well as 'difficult to fix' files, this included 'show' > > > > > functions > > > > > as they really need to use sysfs_emit() or seq_printf(). > > > > > > > > > > All the patches are being sent individually to avoid very > > > > > long cc > > > > > lists. > > > > > Apologies for the terse commit messages and likely unexpected > > > > > tags. > > > > > (There are about 100 patches in total.) > > > > > > > > > > fs/hfsplus/xattr.c | 12 ++++++------ > > > > > 1 file changed, 6 insertions(+), 6 deletions(-) > > > > > > > > > > diff --git a/fs/hfsplus/xattr.c b/fs/hfsplus/xattr.c > > > > > index 452a1f9becb2..0b3dd48c28c9 100644 > > > > > --- a/fs/hfsplus/xattr.c > > > > > +++ b/fs/hfsplus/xattr.c > > > > > @@ -550,8 +550,8 @@ int hfsplus_setxattr(struct inode *inode, > > > > > const > > > > > char *name, > > > > > xattr_name = kmalloc(xattr_name_len, GFP_KERNEL); > > > > > if (!xattr_name) > > > > > return -ENOMEM; > > > > > - strcpy(xattr_name, prefix); > > > > > - strcpy(xattr_name + prefixlen, name); > > > > > + memcpy(xattr_name, prefix, prefixlen); > > > > > > > > What's the point to mix memcpy and str*() family of methods? > > > > What's > > > > wrong with str*() method here? Otherwise, if it is wrong to use > > > > str*() > > > > family of methods, then why is it correct to use for second > > > > operation? > > > > > > > > > + strscpy(xattr_name + prefixlen, name, xattr_name_len > > > > > - > > > > > prefixlen); > > > > > > > > Why strscpy() is better than strncpy()? What is the main > > > > argument > > > > here? > > > > > > > > > res = __hfsplus_setxattr(inode, xattr_name, value, > > > > > size, > > > > > flags); > > > > > kfree(xattr_name); > > > > > > > > > > @@ -698,6 +698,7 @@ ssize_t hfsplus_getxattr(struct inode > > > > > *inode, > > > > > const char *name, > > > > > void *value, size_t size, > > > > > const char *prefix, size_t > > > > > prefixlen) > > > > > { > > > > > + size_t xattr_name_len = NLS_MAX_CHARSET_SIZE * > > > > > HFSPLUS_ATTR_MAX_STRLEN + 1; > > > > > > > > Frankly speaking, it looks like a constant that should be > > > > declared > > > > in > > > > hfs_common.h. Even if we would like to declare it here, then it > > > > should > > > > be const size_t, from my point of view. > > > > > > > > > int res; > > > > > char *xattr_name; > > > > > > > > > > @@ -705,13 +706,12 @@ ssize_t hfsplus_getxattr(struct inode > > > > > *inode, > > > > > const char *name, > > > > > inode->i_ino, name ? name : NULL, > > > > > prefix ? prefix : NULL); > > > > > > > > > > - xattr_name = kmalloc(NLS_MAX_CHARSET_SIZE * > > > > > HFSPLUS_ATTR_MAX_STRLEN + 1, > > > > > - GFP_KERNEL); > > > > > + xattr_name = kmalloc(xattr_name_len, GFP_KERNEL); > > > > > > > > Finally, I think kzalloc() should be much better for both > > > > cases. > > > > > > kasprintf()? > > > > > > > > It sounds much better than suggested fix. > > If performance matters here it will be a lot slower. > The snprintf() code itself is slow and kasprintf() has to do it > twice. > (As well as looking at the strings twice.) > > It also only allocates a buffer that is big enough for a single > terminating '\0' - and (at least some versions) of this code zero > the rest of the buffer (possibly to avoid a bug). > > One option would be something like kstrdup() that concatenates two > strings. > >
We are talking about xattr operations that is not very frequent ones. Also, xattr names usually are not very long. So, it means that we need to manage small memory/string blobs. >From my point of view, the benefit of kasprintf() that we can combine the whole operation into one line of code. It sounds like nice cleanup to me. I don't think that kasprintf() can introduce a significant performance degradation for HFS+ xattr functionality support. Thanks, Slava.
