The xfstests' test-case generic/020 fails to execute correctly: FSTYP -- hfsplus PLATFORM -- Linux/x86_64 hfsplus-testing-0001 6.15.0-rc4+ #8 SMP PREEMPT_DYNAMIC Thu May 1 16:43:22 PDT 2025 MKFS_OPTIONS -- /dev/loop51 MOUNT_OPTIONS -- /dev/loop51 /mnt/scratch generic/020 _check_generic_filesystem: filesystem on /dev/loop50 is inconsistent (see xfstests-dev/results//generic/020.full for details) *** add lots of attributes *** check *** MAX_ATTRS attribute(s) +/mnt/test/attribute_12286: Numerical result out of range *** -1 attribute(s) *** remove lots of attributes ... (Run 'diff -u /xfstests-dev/tests/generic/020.out /xfstests-dev/results//generic/020.out.bad' to see the entire diff) The generic/020 creates more than 100 xattrs and gives its the names user.attribute_ (for example, user.attribute_101). As the next step, listxattr() is called with the goal to check the correctness of xattrs creation. However, it was issue in hfsplus_listxattr() logic. This method re-uses the fd.key->attr.key_name.unicode and strbuf buffers in the loop without re-initialization. As a result, part of the previous name could still remain in the buffers. For example, user.attribute_101 could be processed before user.attribute_54. The issue resulted in formation the name user.attribute_541 instead of user.attribute_54. This patch adds initialization of fd.key->attr.key_name.unicode and strbuf buffers before calling hfs_brec_goto() method that prepare next name in the buffer. HFS+ logic supports only inline xattrs. Such extended attributes can store values not bigger than 3802 bytes [1]. This limitation requires correction of generic/020 logic. Finally, generic/020 can be executed without any issue: sudo ./check generic/020 FSTYP -- hfsplus PLATFORM -- Linux/x86_64 hfsplus-testing-0001 6.19.0-rc1+ #44 SMP PREEMPT_DYNAMIC Mon Dec 22 15:39:00 PST 2025 MKFS_OPTIONS -- /dev/loop51 MOUNT_OPTIONS -- /dev/loop51 /mnt/scratch generic/020 31s ... 38s Ran: generic/020 Passed all 1 tests [1] https://elixir.bootlin.com/linux/v6.19-rc2/source/include/linux/hfs_common.h#L626 Signed-off-by: Viacheslav Dubeyko cc: John Paul Adrian Glaubitz cc: Yangtao Li cc: linux-fsdevel@vger.kernel.org --- fs/hfsplus/attributes.c | 14 +++++++-- fs/hfsplus/xattr.c | 70 ++++++++++++++++++++++++++++++++++------- 2 files changed, 70 insertions(+), 14 deletions(-) diff --git a/fs/hfsplus/attributes.c b/fs/hfsplus/attributes.c index ba26980cc503..74b0ca9c4f17 100644 --- a/fs/hfsplus/attributes.c +++ b/fs/hfsplus/attributes.c @@ -117,8 +117,10 @@ static int hfsplus_attr_build_record(hfsplus_attr_entry *entry, int record_type, entry->inline_data.record_type = cpu_to_be32(record_type); if (size <= HFSPLUS_MAX_INLINE_DATA_SIZE) len = size; - else + else { + hfs_dbg("value size %zu is too big\n", size); return HFSPLUS_INVALID_ATTR_RECORD; + } entry->inline_data.length = cpu_to_be16(len); memcpy(entry->inline_data.raw_bytes, value, len); /* @@ -238,7 +240,11 @@ int hfsplus_create_attr(struct inode *inode, inode->i_ino, value, size); if (entry_size == HFSPLUS_INVALID_ATTR_RECORD) { - err = -EINVAL; + if (size > HFSPLUS_MAX_INLINE_DATA_SIZE) + err = -E2BIG; + else + err = -EINVAL; + hfs_dbg("unable to store value: err %d\n", err); goto failed_create_attr; } @@ -250,8 +256,10 @@ int hfsplus_create_attr(struct inode *inode, } err = hfs_brec_insert(&fd, entry_ptr, entry_size); - if (err) + if (err) { + hfs_dbg("unable to store value: err %d\n", err); goto failed_create_attr; + } hfsplus_mark_inode_dirty(inode, HFSPLUS_I_ATTR_DIRTY); diff --git a/fs/hfsplus/xattr.c b/fs/hfsplus/xattr.c index da95a9de9a65..504518a41212 100644 --- a/fs/hfsplus/xattr.c +++ b/fs/hfsplus/xattr.c @@ -345,8 +345,10 @@ int __hfsplus_setxattr(struct inode *inode, const char *name, if (err) goto end_setxattr; err = hfsplus_create_attr(inode, name, value, size); - if (err) + if (err) { + hfs_dbg("unable to store value: err %d\n", err); goto end_setxattr; + } } else { if (flags & XATTR_REPLACE) { pr_err("cannot replace xattr\n"); @@ -354,8 +356,10 @@ int __hfsplus_setxattr(struct inode *inode, const char *name, goto end_setxattr; } err = hfsplus_create_attr(inode, name, value, size); - if (err) + if (err) { + hfs_dbg("unable to store value: err %d\n", err); goto end_setxattr; + } } cat_entry_type = hfs_bnode_read_u16(cat_fd.bnode, cat_fd.entryoffset); @@ -392,9 +396,9 @@ int __hfsplus_setxattr(struct inode *inode, const char *name, return err; } -static int name_len(const char *xattr_name, int xattr_name_len) +static size_t name_len(const char *xattr_name, size_t xattr_name_len) { - int len = xattr_name_len + 1; + size_t len = xattr_name_len + 1; if (!is_known_namespace(xattr_name)) len += XATTR_MAC_OSX_PREFIX_LEN; @@ -402,15 +406,22 @@ static int name_len(const char *xattr_name, int xattr_name_len) return len; } -static ssize_t copy_name(char *buffer, const char *xattr_name, int name_len) +static ssize_t copy_name(char *buffer, const char *xattr_name, size_t name_len) { ssize_t len; - if (!is_known_namespace(xattr_name)) + memset(buffer, 0, name_len); + + if (!is_known_namespace(xattr_name)) { len = scnprintf(buffer, name_len + XATTR_MAC_OSX_PREFIX_LEN, "%s%s", XATTR_MAC_OSX_PREFIX, xattr_name); - else + } else { len = strscpy(buffer, xattr_name, name_len + 1); + if (len < 0) { + pr_err("fail to copy name: err %zd\n", len); + len = 0; + } + } /* include NUL-byte in length for non-empty name */ if (len >= 0) @@ -423,16 +434,26 @@ int hfsplus_setxattr(struct inode *inode, const char *name, const char *prefix, size_t prefixlen) { char *xattr_name; + size_t xattr_name_len = + NLS_MAX_CHARSET_SIZE * HFSPLUS_ATTR_MAX_STRLEN + 1; int res; - xattr_name = kmalloc(NLS_MAX_CHARSET_SIZE * HFSPLUS_ATTR_MAX_STRLEN + 1, - GFP_KERNEL); + hfs_dbg("ino %lu, name %s, prefix %s, prefixlen %zu, " + "value %p, size %zu\n", + inode->i_ino, name ? name : NULL, + prefix ? prefix : NULL, prefixlen, + value, size); + + xattr_name = kmalloc(xattr_name_len, GFP_KERNEL); if (!xattr_name) return -ENOMEM; strcpy(xattr_name, prefix); strcpy(xattr_name + prefixlen, name); res = __hfsplus_setxattr(inode, xattr_name, value, size, flags); kfree(xattr_name); + + hfs_dbg("finished: res %d\n", res); + return res; } @@ -579,6 +600,10 @@ ssize_t hfsplus_getxattr(struct inode *inode, const char *name, int res; char *xattr_name; + hfs_dbg("ino %lu, name %s, prefix %s\n", + inode->i_ino, name ? name : NULL, + prefix ? prefix : NULL); + xattr_name = kmalloc(NLS_MAX_CHARSET_SIZE * HFSPLUS_ATTR_MAX_STRLEN + 1, GFP_KERNEL); if (!xattr_name) @@ -589,6 +614,9 @@ ssize_t hfsplus_getxattr(struct inode *inode, const char *name, res = __hfsplus_getxattr(inode, xattr_name, value, size); kfree(xattr_name); + + hfs_dbg("finished: res %d\n", res); + return res; } @@ -679,8 +707,11 @@ ssize_t hfsplus_listxattr(struct dentry *dentry, char *buffer, size_t size) struct hfs_find_data fd; struct hfsplus_attr_key attr_key; char *strbuf; + size_t strbuf_size; int xattr_name_len; + hfs_dbg("ino %lu\n", inode->i_ino); + if ((!S_ISREG(inode->i_mode) && !S_ISDIR(inode->i_mode)) || HFSPLUS_IS_RSRC(inode)) @@ -698,8 +729,9 @@ ssize_t hfsplus_listxattr(struct dentry *dentry, char *buffer, size_t size) return err; } - strbuf = kzalloc(NLS_MAX_CHARSET_SIZE * HFSPLUS_ATTR_MAX_STRLEN + - XATTR_MAC_OSX_PREFIX_LEN + 1, GFP_KERNEL); + strbuf_size = NLS_MAX_CHARSET_SIZE * HFSPLUS_ATTR_MAX_STRLEN + + XATTR_MAC_OSX_PREFIX_LEN + 1; + strbuf = kzalloc(strbuf_size, GFP_KERNEL); if (!strbuf) { res = -ENOMEM; goto out; @@ -746,6 +778,9 @@ ssize_t hfsplus_listxattr(struct dentry *dentry, char *buffer, size_t size) res += name_len(strbuf, xattr_name_len); } else if (can_list(strbuf)) { if (size < (res + name_len(strbuf, xattr_name_len))) { + pr_err("size %zu, res %zd, name_len %zu\n", + size, res, + name_len(strbuf, xattr_name_len)); res = -ERANGE; goto end_listxattr; } else @@ -753,6 +788,10 @@ ssize_t hfsplus_listxattr(struct dentry *dentry, char *buffer, size_t size) strbuf, xattr_name_len); } + memset(fd.key->attr.key_name.unicode, 0, + sizeof(fd.key->attr.key_name.unicode)); + memset(strbuf, 0, strbuf_size); + if (hfs_brec_goto(&fd, 1)) goto end_listxattr; } @@ -761,6 +800,9 @@ ssize_t hfsplus_listxattr(struct dentry *dentry, char *buffer, size_t size) kfree(strbuf); out: hfs_find_exit(&fd); + + hfs_dbg("finished: res %zd\n", res); + return res; } @@ -773,6 +815,9 @@ static int hfsplus_removexattr(struct inode *inode, const char *name) int is_xattr_acl_deleted; int is_all_xattrs_deleted; + hfs_dbg("ino %lu, name %s\n", + inode->i_ino, name ? name : NULL); + if (!HFSPLUS_SB(inode->i_sb)->attr_tree) return -EOPNOTSUPP; @@ -833,6 +878,9 @@ static int hfsplus_removexattr(struct inode *inode, const char *name) end_removexattr: hfs_find_exit(&cat_fd); + + hfs_dbg("finished: err %d\n", err); + return err; } -- 2.43.0