hfsplus: fix generic/020 xfstests failure

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_<number> (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 <slava@dubeyko.com>
cc: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>
cc: Yangtao Li <frank.li@vivo.com>
cc: linux-fsdevel@vger.kernel.org
Link: https://lore.kernel.org/r/20251224002810.1137139-1-slava@dubeyko.com
Signed-off-by: Viacheslav Dubeyko <slava@dubeyko.com>

diff --git a/fs/hfsplus/attributes.c b/fs/hfsplus/attributes.c
index ba26980cc5035ccf41698e3384388f42e44f393e..74b0ca9c4f17b38b21386d322ac194dd707ebc8a 100644 (file)
--- 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 da95a9de9a659f8e9d849e572ac84b680d14fecd..504518a4121287e59023241a5fe65fa922654084 100644 (file)
--- 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 @@ end_setxattr:
        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 @@ end_listxattr:
        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;
 }