* [PATCH AUTOSEL 6.18-5.10] hfsplus: Verify inode mode when loading from disk
[not found] <20251206140252.645973-1-sashal@kernel.org>
@ 2025-12-06 14:02 ` Sasha Levin
2025-12-06 14:02 ` [PATCH AUTOSEL 6.18] hfsplus: fix volume corruption issue for generic/101 Sasha Levin
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Sasha Levin @ 2025-12-06 14:02 UTC (permalink / raw)
To: patches, stable
Cc: Tetsuo Handa, syzbot, Viacheslav Dubeyko, Sasha Levin, frank.li,
linux-fsdevel
From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
[ Upstream commit 005d4b0d33f6b4a23d382b7930f7a96b95b01f39 ]
syzbot is reporting that S_IFMT bits of inode->i_mode can become bogus when
the S_IFMT bits of the 16bits "mode" field loaded from disk are corrupted.
According to [1], the permissions field was treated as reserved in Mac OS
8 and 9. According to [2], the reserved field was explicitly initialized
with 0, and that field must remain 0 as long as reserved. Therefore, when
the "mode" field is not 0 (i.e. no longer reserved), the file must be
S_IFDIR if dir == 1, and the file must be one of S_IFREG/S_IFLNK/S_IFCHR/
S_IFBLK/S_IFIFO/S_IFSOCK if dir == 0.
Reported-by: syzbot <syzbot+895c23f6917da440ed0d@syzkaller.appspotmail.com>
Closes: https://syzkaller.appspot.com/bug?extid=895c23f6917da440ed0d
Link: https://developer.apple.com/library/archive/technotes/tn/tn1150.html#HFSPlusPermissions [1]
Link: https://developer.apple.com/library/archive/technotes/tn/tn1150.html#ReservedAndPadFields [2]
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Reviewed-by: Viacheslav Dubeyko <slava@dubeyko.com>
Signed-off-by: Viacheslav Dubeyko <slava@dubeyko.com>
Link: https://lore.kernel.org/r/04ded9f9-73fb-496c-bfa5-89c4f5d1d7bb@I-love.SAKURA.ne.jp
Signed-off-by: Viacheslav Dubeyko <slava@dubeyko.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
LLM Generated explanations, may be completely bogus:
## Analysis of hfsplus: Verify inode mode when loading from disk
### 1. COMMIT MESSAGE ANALYSIS
**Key indicators:**
- **Reported-by: syzbot** - This is a real bug found by automated
fuzzing
- **Closes:** link to syzkaller bug report confirms this is a genuine
issue
- **Reviewed-by:** present from the HFS+ maintainer (Viacheslav Dubeyko)
- **No "Cc: stable@vger.kernel.org"** tag
- **No "Fixes:" tag** - bug appears to exist since original HFS+
implementation
The commit describes that corrupted S_IFMT bits in the on-disk "mode"
field can cause inode->i_mode to become bogus when loaded from disk. The
commit message references Apple technical documentation explaining the
expected values for the mode field.
### 2. CODE CHANGE ANALYSIS
The fix modifies `hfsplus_get_perms()` in two ways:
**a) Adds validation logic (the core fix):**
```c
if (dir) {
if (mode && !S_ISDIR(mode))
goto bad_type;
} else if (mode) {
switch (mode & S_IFMT) {
case S_IFREG:
case S_IFLNK:
case S_IFCHR:
case S_IFBLK:
case S_IFIFO:
case S_IFSOCK:
break;
default:
goto bad_type;
}
}
```
This validates that:
- For directories (`dir=1`): mode must be 0 or actually be a directory
type
- For files (`dir=0`): mode must be 0 or one of the valid file types
(regular, symlink, char/block device, FIFO, socket)
**b) Changes return type from `void` to `int`:**
- Returns -EIO on invalid mode with an error message
- Callers (`hfsplus_cat_read_inode`) now check the return value and
propagate errors
**Root cause:** The original code blindly trusted the mode field from
disk without validating that the S_IFMT bits are consistent with the
directory flag.
### 3. CLASSIFICATION
- **Type:** Bug fix (input validation)
- **Security relevance:** Yes - crafted filesystem images could trigger
this
- **Category:** Filesystem robustness/hardening against corrupted data
### 4. SCOPE AND RISK ASSESSMENT
| Aspect | Assessment |
|--------|------------|
| Lines changed | ~30+ additions, moderate size |
| Files touched | 1 file (fs/hfsplus/inode.c) |
| Complexity | Low - straightforward validation logic |
| Regression risk | **LOW** - only rejects clearly invalid data |
The validation is conservative and follows Apple's official HFS+
specification. It only rejects modes that are definitively wrong.
### 5. USER IMPACT
- **Affected users:** Those mounting HFS+ filesystems (macOS external
drives, dual-boot setups)
- **Trigger:** Mounting a corrupted or maliciously crafted HFS+
filesystem image
- **Impact of bug:** Bogus inode mode can lead to undefined kernel
behavior when processing the inode
- **Impact of fix:** Graceful rejection with -EIO instead of corrupted
internal state
### 6. STABILITY INDICATORS
- Reviewed by subsystem maintainer ✓
- Clean, standalone fix with no dependencies ✓
- The modified functions exist in older stable kernels ✓
- No unusual code patterns or risky constructs ✓
### 7. DEPENDENCY CHECK
This is a standalone fix. The `hfsplus_get_perms` and
`hfsplus_cat_read_inode` functions exist in all stable trees where HFS+
is supported.
---
## Summary
**What it fixes:** Prevents corrupted or maliciously crafted HFS+
filesystem images from causing bogus inode modes to be loaded into the
kernel.
**Why it matters for stable:** This is a defensive fix that prevents
accepting corrupted data, which could lead to undefined behavior. syzbot
found this bug, indicating it can be triggered by crafted input - a
potential security concern.
**Meets stable criteria:**
- ✓ Obviously correct (validates according to Apple's HFS+
specification)
- ✓ Fixes a real bug that affects users (syzbot found it with crafted
images)
- ✓ Small and contained (single file, ~30 lines of validation)
- ✓ Low regression risk (only rejects clearly invalid data)
- ✗ No explicit stable tag (author didn't request it)
**Risk vs Benefit:**
- **Risk:** Very low - adds validation that only rejects invalid data
- **Benefit:** Prevents kernel from operating on corrupted inode modes
from crafted/corrupted filesystems
The lack of explicit stable tags suggests the author may not have
intended this for stable, but the fix clearly meets stable criteria as a
filesystem robustness fix against corrupted input. Filesystem fixes for
handling crafted/corrupted data are generally appropriate for stable
trees.
**YES**
fs/hfsplus/inode.c | 32 ++++++++++++++++++++++++++++----
1 file changed, 28 insertions(+), 4 deletions(-)
diff --git a/fs/hfsplus/inode.c b/fs/hfsplus/inode.c
index b51a411ecd237..e290e417ed3a7 100644
--- a/fs/hfsplus/inode.c
+++ b/fs/hfsplus/inode.c
@@ -180,13 +180,29 @@ const struct dentry_operations hfsplus_dentry_operations = {
.d_compare = hfsplus_compare_dentry,
};
-static void hfsplus_get_perms(struct inode *inode,
- struct hfsplus_perm *perms, int dir)
+static int hfsplus_get_perms(struct inode *inode,
+ struct hfsplus_perm *perms, int dir)
{
struct hfsplus_sb_info *sbi = HFSPLUS_SB(inode->i_sb);
u16 mode;
mode = be16_to_cpu(perms->mode);
+ if (dir) {
+ if (mode && !S_ISDIR(mode))
+ goto bad_type;
+ } else if (mode) {
+ switch (mode & S_IFMT) {
+ case S_IFREG:
+ case S_IFLNK:
+ case S_IFCHR:
+ case S_IFBLK:
+ case S_IFIFO:
+ case S_IFSOCK:
+ break;
+ default:
+ goto bad_type;
+ }
+ }
i_uid_write(inode, be32_to_cpu(perms->owner));
if ((test_bit(HFSPLUS_SB_UID, &sbi->flags)) || (!i_uid_read(inode) && !mode))
@@ -212,6 +228,10 @@ static void hfsplus_get_perms(struct inode *inode,
inode->i_flags |= S_APPEND;
else
inode->i_flags &= ~S_APPEND;
+ return 0;
+bad_type:
+ pr_err("invalid file type 0%04o for inode %lu\n", mode, inode->i_ino);
+ return -EIO;
}
static int hfsplus_file_open(struct inode *inode, struct file *file)
@@ -516,7 +536,9 @@ int hfsplus_cat_read_inode(struct inode *inode, struct hfs_find_data *fd)
}
hfs_bnode_read(fd->bnode, &entry, fd->entryoffset,
sizeof(struct hfsplus_cat_folder));
- hfsplus_get_perms(inode, &folder->permissions, 1);
+ res = hfsplus_get_perms(inode, &folder->permissions, 1);
+ if (res)
+ goto out;
set_nlink(inode, 1);
inode->i_size = 2 + be32_to_cpu(folder->valence);
inode_set_atime_to_ts(inode, hfsp_mt2ut(folder->access_date));
@@ -545,7 +567,9 @@ int hfsplus_cat_read_inode(struct inode *inode, struct hfs_find_data *fd)
hfsplus_inode_read_fork(inode, HFSPLUS_IS_RSRC(inode) ?
&file->rsrc_fork : &file->data_fork);
- hfsplus_get_perms(inode, &file->permissions, 0);
+ res = hfsplus_get_perms(inode, &file->permissions, 0);
+ if (res)
+ goto out;
set_nlink(inode, 1);
if (S_ISREG(inode->i_mode)) {
if (file->permissions.dev)
--
2.51.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH AUTOSEL 6.18] hfsplus: fix volume corruption issue for generic/101
[not found] <20251206140252.645973-1-sashal@kernel.org>
2025-12-06 14:02 ` [PATCH AUTOSEL 6.18-5.10] hfsplus: Verify inode mode when loading from disk Sasha Levin
@ 2025-12-06 14:02 ` Sasha Levin
2025-12-06 14:02 ` [PATCH AUTOSEL 6.18-5.10] hfsplus: fix missing hfs_bnode_get() in __hfs_bnode_create Sasha Levin
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Sasha Levin @ 2025-12-06 14:02 UTC (permalink / raw)
To: patches, stable
Cc: Viacheslav Dubeyko, John Paul Adrian Glaubitz, Yangtao Li,
linux-fsdevel, Sasha Levin
From: Viacheslav Dubeyko <slava@dubeyko.com>
[ Upstream commit 3f04ee216bc1406cb6214ceaa7e544114108e0fa ]
The xfstests' test-case generic/101 leaves HFS+ volume
in corrupted state:
sudo ./check generic/101
FSTYP -- hfsplus
PLATFORM -- Linux/x86_64 hfsplus-testing-0001 6.17.0-rc1+ #4 SMP PREEMPT_DYNAMIC Wed Oct 1 15:02:44 PDT 2025
MKFS_OPTIONS -- /dev/loop51
MOUNT_OPTIONS -- /dev/loop51 /mnt/scratch
generic/101 _check_generic_filesystem: filesystem on /dev/loop51 is inconsistent
(see XFSTESTS-2/xfstests-dev/results//generic/101.full for details)
Ran: generic/101
Failures: generic/101
Failed 1 of 1 tests
sudo fsck.hfsplus -d /dev/loop51
** /dev/loop51
Using cacheBlockSize=32K cacheTotalBlock=1024 cacheSize=32768K.
Executing fsck_hfs (version 540.1-Linux).
** Checking non-journaled HFS Plus Volume.
The volume name is untitled
** Checking extents overflow file.
** Checking catalog file.
** Checking multi-linked files.
** Checking catalog hierarchy.
** Checking extended attributes file.
** Checking volume bitmap.
** Checking volume information.
Invalid volume free block count
(It should be 2614350 instead of 2614382)
Verify Status: VIStat = 0x8000, ABTStat = 0x0000 EBTStat = 0x0000
CBTStat = 0x0000 CatStat = 0x00000000
** Repairing volume.
** Rechecking volume.
** Checking non-journaled HFS Plus Volume.
The volume name is untitled
** Checking extents overflow file.
** Checking catalog file.
** Checking multi-linked files.
** Checking catalog hierarchy.
** Checking extended attributes file.
** Checking volume bitmap.
** Checking volume information.
** The volume untitled was repaired successfully.
This test executes such steps: "Test that if we truncate a file
to a smaller size, then truncate it to its original size or
a larger size, then fsyncing it and a power failure happens,
the file will have the range [first_truncate_size, last_size[ with
all bytes having a value of 0x00 if we read it the next time
the filesystem is mounted.".
HFS+ keeps volume's free block count in the superblock.
However, hfsplus_file_fsync() doesn't store superblock's
content. As a result, superblock contains not correct
value of free blocks if a power failure happens.
This patch adds functionality of saving superblock's
content during hfsplus_file_fsync() call.
sudo ./check generic/101
FSTYP -- hfsplus
PLATFORM -- Linux/x86_64 hfsplus-testing-0001 6.18.0-rc3+ #96 SMP PREEMPT_DYNAMIC Wed Nov 19 12:47:37 PST 2025
MKFS_OPTIONS -- /dev/loop51
MOUNT_OPTIONS -- /dev/loop51 /mnt/scratch
generic/101 32s ... 30s
Ran: generic/101
Passed all 1 tests
sudo fsck.hfsplus -d /dev/loop51
** /dev/loop51
Using cacheBlockSize=32K cacheTotalBlock=1024 cacheSize=32768K.
Executing fsck_hfs (version 540.1-Linux).
** Checking non-journaled HFS Plus Volume.
The volume name is untitled
** Checking extents overflow file.
** Checking catalog file.
** Checking multi-linked files.
** Checking catalog hierarchy.
** Checking extended attributes file.
** Checking volume bitmap.
** Checking volume information.
** The volume untitled appears to be OK.
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/20251119223219.1824434-1-slava@dubeyko.com
Signed-off-by: Viacheslav Dubeyko <slava@dubeyko.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
LLM Generated explanations, may be completely bogus:
fs/hfsplus/hfsplus_fs.h | 2 +
fs/hfsplus/inode.c | 9 +++++
fs/hfsplus/super.c | 87 +++++++++++++++++++++++++----------------
3 files changed, 65 insertions(+), 33 deletions(-)
diff --git a/fs/hfsplus/hfsplus_fs.h b/fs/hfsplus/hfsplus_fs.h
index 89e8b19c127b0..de801942ae471 100644
--- a/fs/hfsplus/hfsplus_fs.h
+++ b/fs/hfsplus/hfsplus_fs.h
@@ -477,6 +477,8 @@ int hfs_part_find(struct super_block *sb, sector_t *part_start,
/* super.c */
struct inode *hfsplus_iget(struct super_block *sb, unsigned long ino);
void hfsplus_mark_mdb_dirty(struct super_block *sb);
+void hfsplus_prepare_volume_header_for_commit(struct hfsplus_vh *vhdr);
+int hfsplus_commit_superblock(struct super_block *sb);
/* tables.c */
extern u16 hfsplus_case_fold_table[];
diff --git a/fs/hfsplus/inode.c b/fs/hfsplus/inode.c
index e290e417ed3a7..7ae6745ca7ae1 100644
--- a/fs/hfsplus/inode.c
+++ b/fs/hfsplus/inode.c
@@ -325,6 +325,7 @@ int hfsplus_file_fsync(struct file *file, loff_t start, loff_t end,
struct inode *inode = file->f_mapping->host;
struct hfsplus_inode_info *hip = HFSPLUS_I(inode);
struct hfsplus_sb_info *sbi = HFSPLUS_SB(inode->i_sb);
+ struct hfsplus_vh *vhdr = sbi->s_vhdr;
int error = 0, error2;
error = file_write_and_wait_range(file, start, end);
@@ -368,6 +369,14 @@ int hfsplus_file_fsync(struct file *file, loff_t start, loff_t end,
error = error2;
}
+ mutex_lock(&sbi->vh_mutex);
+ hfsplus_prepare_volume_header_for_commit(vhdr);
+ mutex_unlock(&sbi->vh_mutex);
+
+ error2 = hfsplus_commit_superblock(inode->i_sb);
+ if (!error)
+ error = error2;
+
if (!test_bit(HFSPLUS_SB_NOBARRIER, &sbi->flags))
blkdev_issue_flush(inode->i_sb->s_bdev);
diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c
index 16bc4abc67e08..67a7a2a093476 100644
--- a/fs/hfsplus/super.c
+++ b/fs/hfsplus/super.c
@@ -187,40 +187,15 @@ static void hfsplus_evict_inode(struct inode *inode)
}
}
-static int hfsplus_sync_fs(struct super_block *sb, int wait)
+int hfsplus_commit_superblock(struct super_block *sb)
{
struct hfsplus_sb_info *sbi = HFSPLUS_SB(sb);
struct hfsplus_vh *vhdr = sbi->s_vhdr;
int write_backup = 0;
- int error, error2;
-
- if (!wait)
- return 0;
+ int error = 0, error2;
hfs_dbg("starting...\n");
- /*
- * Explicitly write out the special metadata inodes.
- *
- * While these special inodes are marked as hashed and written
- * out peridocically by the flusher threads we redirty them
- * during writeout of normal inodes, and thus the life lock
- * prevents us from getting the latest state to disk.
- */
- error = filemap_write_and_wait(sbi->cat_tree->inode->i_mapping);
- error2 = filemap_write_and_wait(sbi->ext_tree->inode->i_mapping);
- if (!error)
- error = error2;
- if (sbi->attr_tree) {
- error2 =
- filemap_write_and_wait(sbi->attr_tree->inode->i_mapping);
- if (!error)
- error = error2;
- }
- error2 = filemap_write_and_wait(sbi->alloc_file->i_mapping);
- if (!error)
- error = error2;
-
mutex_lock(&sbi->vh_mutex);
mutex_lock(&sbi->alloc_mutex);
vhdr->free_blocks = cpu_to_be32(sbi->free_blocks);
@@ -249,11 +224,52 @@ static int hfsplus_sync_fs(struct super_block *sb, int wait)
sbi->part_start + sbi->sect_count - 2,
sbi->s_backup_vhdr_buf, NULL, REQ_OP_WRITE);
if (!error)
- error2 = error;
+ error = error2;
out:
mutex_unlock(&sbi->alloc_mutex);
mutex_unlock(&sbi->vh_mutex);
+ hfs_dbg("finished: err %d\n", error);
+
+ return error;
+}
+
+static int hfsplus_sync_fs(struct super_block *sb, int wait)
+{
+ struct hfsplus_sb_info *sbi = HFSPLUS_SB(sb);
+ int error, error2;
+
+ if (!wait)
+ return 0;
+
+ hfs_dbg("starting...\n");
+
+ /*
+ * Explicitly write out the special metadata inodes.
+ *
+ * While these special inodes are marked as hashed and written
+ * out peridocically by the flusher threads we redirty them
+ * during writeout of normal inodes, and thus the life lock
+ * prevents us from getting the latest state to disk.
+ */
+ error = filemap_write_and_wait(sbi->cat_tree->inode->i_mapping);
+ error2 = filemap_write_and_wait(sbi->ext_tree->inode->i_mapping);
+ if (!error)
+ error = error2;
+ if (sbi->attr_tree) {
+ error2 =
+ filemap_write_and_wait(sbi->attr_tree->inode->i_mapping);
+ if (!error)
+ error = error2;
+ }
+ error2 = filemap_write_and_wait(sbi->alloc_file->i_mapping);
+ if (!error)
+ error = error2;
+
+ error2 = hfsplus_commit_superblock(sb);
+ if (!error)
+ error = error2;
+
if (!test_bit(HFSPLUS_SB_NOBARRIER, &sbi->flags))
blkdev_issue_flush(sb->s_bdev);
@@ -395,6 +411,15 @@ static const struct super_operations hfsplus_sops = {
.show_options = hfsplus_show_options,
};
+void hfsplus_prepare_volume_header_for_commit(struct hfsplus_vh *vhdr)
+{
+ vhdr->last_mount_vers = cpu_to_be32(HFSP_MOUNT_VERSION);
+ vhdr->modify_date = hfsp_now2mt();
+ be32_add_cpu(&vhdr->write_count, 1);
+ vhdr->attributes &= cpu_to_be32(~HFSPLUS_VOL_UNMNT);
+ vhdr->attributes |= cpu_to_be32(HFSPLUS_VOL_INCNSTNT);
+}
+
static int hfsplus_fill_super(struct super_block *sb, struct fs_context *fc)
{
struct hfsplus_vh *vhdr;
@@ -562,11 +587,7 @@ static int hfsplus_fill_super(struct super_block *sb, struct fs_context *fc)
* H+LX == hfsplusutils, H+Lx == this driver, H+lx is unused
* all three are registered with Apple for our use
*/
- vhdr->last_mount_vers = cpu_to_be32(HFSP_MOUNT_VERSION);
- vhdr->modify_date = hfsp_now2mt();
- be32_add_cpu(&vhdr->write_count, 1);
- vhdr->attributes &= cpu_to_be32(~HFSPLUS_VOL_UNMNT);
- vhdr->attributes |= cpu_to_be32(HFSPLUS_VOL_INCNSTNT);
+ hfsplus_prepare_volume_header_for_commit(vhdr);
hfsplus_sync_fs(sb, 1);
if (!sbi->hidden_dir) {
--
2.51.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH AUTOSEL 6.18-5.10] hfsplus: fix missing hfs_bnode_get() in __hfs_bnode_create
[not found] <20251206140252.645973-1-sashal@kernel.org>
2025-12-06 14:02 ` [PATCH AUTOSEL 6.18-5.10] hfsplus: Verify inode mode when loading from disk Sasha Levin
2025-12-06 14:02 ` [PATCH AUTOSEL 6.18] hfsplus: fix volume corruption issue for generic/101 Sasha Levin
@ 2025-12-06 14:02 ` Sasha Levin
2025-12-06 14:02 ` [PATCH AUTOSEL 6.18-5.10] hfsplus: fix volume corruption issue for generic/070 Sasha Levin
2025-12-06 14:02 ` [PATCH AUTOSEL 6.18-5.10] hfsplus: fix volume corruption issue for generic/073 Sasha Levin
4 siblings, 0 replies; 6+ messages in thread
From: Sasha Levin @ 2025-12-06 14:02 UTC (permalink / raw)
To: patches, stable
Cc: Yang Chenzhi, syzbot+005d2a9ecd9fbf525f6a, Viacheslav Dubeyko,
Sasha Levin, frank.li, linux-fsdevel
From: Yang Chenzhi <yang.chenzhi@vivo.com>
[ Upstream commit 152af114287851583cf7e0abc10129941f19466a ]
When sync() and link() are called concurrently, both threads may
enter hfs_bnode_find() without finding the node in the hash table
and proceed to create it.
Thread A:
hfsplus_write_inode()
-> hfsplus_write_system_inode()
-> hfs_btree_write()
-> hfs_bnode_find(tree, 0)
-> __hfs_bnode_create(tree, 0)
Thread B:
hfsplus_create_cat()
-> hfs_brec_insert()
-> hfs_bnode_split()
-> hfs_bmap_alloc()
-> hfs_bnode_find(tree, 0)
-> __hfs_bnode_create(tree, 0)
In this case, thread A creates the bnode, sets refcnt=1, and hashes it.
Thread B also tries to create the same bnode, notices it has already
been inserted, drops its own instance, and uses the hashed one without
getting the node.
```
node2 = hfs_bnode_findhash(tree, cnid);
if (!node2) { <- Thread A
hash = hfs_bnode_hash(cnid);
node->next_hash = tree->node_hash[hash];
tree->node_hash[hash] = node;
tree->node_hash_cnt++;
} else { <- Thread B
spin_unlock(&tree->hash_lock);
kfree(node);
wait_event(node2->lock_wq,
!test_bit(HFS_BNODE_NEW, &node2->flags));
return node2;
}
```
However, hfs_bnode_find() requires each call to take a reference.
Here both threads end up setting refcnt=1. When they later put the node,
this triggers:
BUG_ON(!atomic_read(&node->refcnt))
In this scenario, Thread B in fact finds the node in the hash table
rather than creating a new one, and thus must take a reference.
Fix this by calling hfs_bnode_get() when reusing a bnode newly created by
another thread to ensure the refcount is updated correctly.
A similar bug was fixed in HFS long ago in commit
a9dc087fd3c4 ("fix missing hfs_bnode_get() in __hfs_bnode_create")
but the same issue remained in HFS+ until now.
Reported-by: syzbot+005d2a9ecd9fbf525f6a@syzkaller.appspotmail.com
Signed-off-by: Yang Chenzhi <yang.chenzhi@vivo.com>
Signed-off-by: Viacheslav Dubeyko <slava@dubeyko.com>
Link: https://lore.kernel.org/r/20250829093912.611853-1-yang.chenzhi@vivo.com
Signed-off-by: Viacheslav Dubeyko <slava@dubeyko.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
LLM Generated explanations, may be completely bogus:
## Analysis Summary
### 1. COMMIT MESSAGE ANALYSIS
**Subject:** "hfsplus: fix missing hfs_bnode_get() in
__hfs_bnode_create"
**Key signals:**
- Clear "fix" keyword indicating bug fix
- Reported-by syzbot - reproducible crash bug
- Detailed race condition explanation with call stacks
- References identical HFS fix from 2022 (commit a9dc087fd3c4)
- No explicit `Cc: stable` tag, but no `Fixes:` tag either since the bug
exists from the file's creation
### 2. CODE CHANGE ANALYSIS
**The Bug:** When `sync()` and `link()` are called concurrently, both
threads may race into `__hfs_bnode_create()`:
- Thread A creates a bnode with `refcnt=1` and inserts it into the hash
table
- Thread B finds the hash table entry, but returns the node **without
incrementing refcnt**
- Both threads believe they own a reference, but only one reference
exists
- When both call `hfs_bnode_put()`, the second call triggers:
`BUG_ON(!atomic_read(&node->refcnt))`
**The Fix:** Single line addition at `fs/hfsplus/bnode.c:484`:
```c
} else {
+ hfs_bnode_get(node2); // <-- Missing refcount increment added
spin_unlock(&tree->hash_lock);
kfree(node);
```
**Why it's correct:** `hfs_bnode_get()` simply does
`atomic_inc(&node->refcnt)` (line 658), ensuring correct reference
counting when reusing a shared bnode.
### 3. CLASSIFICATION
- **Bug fix:** YES - fixes a crash (BUG_ON kernel panic)
- **Feature addition:** NO
- **Security consideration:** Crash can be triggered by normal
operations - potential DoS vector
### 4. SCOPE AND RISK ASSESSMENT
| Metric | Value |
|--------|-------|
| Lines changed | 1 |
| Files touched | 1 |
| Complexity | Very low |
| Subsystem | HFS+ filesystem |
| Regression risk | Very low |
The fix is a **single function call** that mirrors a proven fix from HFS
(commit a9dc087fd3c4) that has been stable since December 2022.
### 5. USER IMPACT
- **Affected users:** Anyone using HFS+ filesystems (common for Mac disk
compatibility, external drives, dual-boot systems)
- **Trigger condition:** Concurrent sync() and link() operations - can
occur in normal workloads
- **Severity:** **KERNEL CRASH** (BUG_ON triggers panic)
### 6. STABILITY INDICATORS
- **syzbot reported:** Bug is reproducible
- **Maintainer signed:** Yes (Viacheslav Dubeyko, HFS+ maintainer)
- **LKML link:** Present
- **Precedent:** Identical fix applied to HFS in 2022 with no
regressions
### 7. DEPENDENCY CHECK
- **Dependencies:** None - completely self-contained
- **Applies to stable:** The affected code pattern has existed unchanged
for many years in stable trees
## Final Assessment
**This commit should be backported to stable kernels.**
**Rationale:**
1. **Fixes a real crash** - BUG_ON triggers kernel panic in a
reproducible race condition
2. **Minimal and surgical** - Single line change adding one function
call
3. **Obviously correct** - Adds missing reference count increment,
matching HFS pattern
4. **Proven safe** - Identical fix in HFS has been stable for 2+ years
5. **No dependencies** - Will apply cleanly to all stable kernels
6. **Real user impact** - HFS+ is commonly used for Mac disk
compatibility
The lack of explicit `Cc: stable` tag does not preclude backporting when
all other stable criteria are clearly met. This is a textbook case of a
small, obviously correct fix for a real crash bug.
**YES**
fs/hfsplus/bnode.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/fs/hfsplus/bnode.c b/fs/hfsplus/bnode.c
index edf7e27e1e375..482a6c5faa197 100644
--- a/fs/hfsplus/bnode.c
+++ b/fs/hfsplus/bnode.c
@@ -481,6 +481,7 @@ static struct hfs_bnode *__hfs_bnode_create(struct hfs_btree *tree, u32 cnid)
tree->node_hash[hash] = node;
tree->node_hash_cnt++;
} else {
+ hfs_bnode_get(node2);
spin_unlock(&tree->hash_lock);
kfree(node);
wait_event(node2->lock_wq,
--
2.51.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH AUTOSEL 6.18-5.10] hfsplus: fix volume corruption issue for generic/070
[not found] <20251206140252.645973-1-sashal@kernel.org>
` (2 preceding siblings ...)
2025-12-06 14:02 ` [PATCH AUTOSEL 6.18-5.10] hfsplus: fix missing hfs_bnode_get() in __hfs_bnode_create Sasha Levin
@ 2025-12-06 14:02 ` Sasha Levin
2025-12-06 14:02 ` [PATCH AUTOSEL 6.18-5.10] hfsplus: fix volume corruption issue for generic/073 Sasha Levin
4 siblings, 0 replies; 6+ messages in thread
From: Sasha Levin @ 2025-12-06 14:02 UTC (permalink / raw)
To: patches, stable
Cc: Viacheslav Dubeyko, John Paul Adrian Glaubitz, Yangtao Li,
linux-fsdevel, Sasha Levin
From: Viacheslav Dubeyko <slava@dubeyko.com>
[ Upstream commit ed490f36f439b877393c12a2113601e4145a5a56 ]
The xfstests' test-case generic/070 leaves HFS+ volume
in corrupted state:
sudo ./check generic/070
FSTYP -- hfsplus
PLATFORM -- Linux/x86_64 hfsplus-testing-0001 6.17.0-rc1+ #4 SMP PREEMPT_DYNAMIC Wed Oct 1 15:02:44 PDT 2025
MKFS_OPTIONS -- /dev/loop51
MOUNT_OPTIONS -- /dev/loop51 /mnt/scratch
generic/070 _check_generic_filesystem: filesystem on /dev/loop50 is inconsistent
(see xfstests-dev/results//generic/070.full for details)
Ran: generic/070
Failures: generic/070
Failed 1 of 1 tests
sudo fsck.hfsplus -d /dev/loop50
** /dev/loop50
Using cacheBlockSize=32K cacheTotalBlock=1024 cacheSize=32768K.
Executing fsck_hfs (version 540.1-Linux).
** Checking non-journaled HFS Plus Volume.
The volume name is test
** Checking extents overflow file.
Unused node is not erased (node = 1)
** Checking catalog file.
** Checking multi-linked files.
** Checking catalog hierarchy.
** Checking extended attributes file.
** Checking volume bitmap.
** Checking volume information.
Verify Status: VIStat = 0x0000, ABTStat = 0x0000 EBTStat = 0x0004
CBTStat = 0x0000 CatStat = 0x00000000
** Repairing volume.
** Rechecking volume.
** Checking non-journaled HFS Plus Volume.
The volume name is test
** Checking extents overflow file.
** Checking catalog file.
** Checking multi-linked files.
** Checking catalog hierarchy.
** Checking extended attributes file.
** Checking volume bitmap.
** Checking volume information.
** The volume test was repaired successfully.
It is possible to see that fsck.hfsplus detected not
erased and unused node for the case of extents overflow file.
The HFS+ logic has special method that defines if the node
should be erased:
bool hfs_bnode_need_zeroout(struct hfs_btree *tree)
{
struct super_block *sb = tree->inode->i_sb;
struct hfsplus_sb_info *sbi = HFSPLUS_SB(sb);
const u32 volume_attr = be32_to_cpu(sbi->s_vhdr->attributes);
return tree->cnid == HFSPLUS_CAT_CNID &&
volume_attr & HFSPLUS_VOL_UNUSED_NODE_FIX;
}
However, it is possible to see that this method works
only for the case of catalog file. But debugging of the issue
has shown that HFSPLUS_VOL_UNUSED_NODE_FIX attribute has been
requested for the extents overflow file too:
catalog file
kernel: hfsplus: node 4, num_recs 0, flags 0x10
kernel: hfsplus: tree->cnid 4, volume_attr 0x80000800
extents overflow file
kernel: hfsplus: node 1, num_recs 0, flags 0x10
kernel: hfsplus: tree->cnid 3, volume_attr 0x80000800
This patch modifies the hfs_bnode_need_zeroout() by checking
only volume_attr but not the b-tree ID because node zeroing
can be requested for all HFS+ b-tree types.
sudo ./check generic/070
FSTYP -- hfsplus
PLATFORM -- Linux/x86_64 hfsplus-testing-0001 6.18.0-rc3+ #79 SMP PREEMPT_DYNAMIC Fri Oct 31 16:07:42 PDT 2025
MKFS_OPTIONS -- /dev/loop51
MOUNT_OPTIONS -- /dev/loop51 /mnt/scratch
generic/070 33s ... 34s
Ran: generic/070
Passed all 1 tests
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/20251101001229.247432-1-slava@dubeyko.com
Signed-off-by: Viacheslav Dubeyko <slava@dubeyko.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
LLM Generated explanations, may be completely bogus:
## Final Assessment
### Summary of Findings
**Bug Type**: Filesystem corruption (data integrity issue)
**Technical Root Cause**: The `hfs_bnode_need_zeroout()` function
incorrectly restricts unused node zeroing to only the catalog B-tree
(HFSPLUS_CAT_CNID). However, when the volume attribute
`HFSPLUS_VOL_UNUSED_NODE_FIX` is set, it applies to ALL HFS+ B-tree
types including the extents overflow file. This causes fsck.hfsplus to
detect corruption: "Unused node is not erased (node = 1)".
**The Fix**: Removes the catalog-only check, allowing node zeroing for
all B-tree types when the volume attribute is set.
### Stable Kernel Criteria Evaluation
| Criterion | Status |
|-----------|--------|
| Fixes real bug | ✅ Filesystem corruption verified by fsck |
| Obviously correct | ✅ Simple condition removal, matches Apple spec |
| Small and contained | ✅ 1 line change, single file |
| No new features | ✅ No new functionality added |
| Tested | ✅ xfstests generic/070 passes |
| Exists in stable | ✅ Function introduced in kernel 3.16 (2014) |
### Risk vs Benefit
**Risk**: Very LOW
- The change makes code more conservative (zeros more nodes, not fewer)
- Only two call sites, both appropriately handle the result
- No new code paths, just relaxing an incorrect restriction
**Benefit**: HIGH
- Fixes filesystem corruption that users can actually hit
- Reproducible with standard xfstests suite
- Prevents data integrity issues on HFS+ volumes
### Concerns
1. **No explicit stable tags**: Missing "Cc: stable" and "Fixes:" tags.
However, filesystem corruption fixes are exactly what stable is for.
2. **Stale comment**: The comment still says "if this is the catalog
tree" but this is documentation debt, not a functional issue.
3. **Limited user base**: HFS+ is less commonly used on Linux than other
filesystems, but users who do use it deserve working support.
### Conclusion
This commit fixes a real, reproducible filesystem corruption issue with
an extremely small, safe change. The fix is obviously correct - it
aligns behavior with the HFS+ specification where
`HFSPLUS_VOL_UNUSED_NODE_FIX` applies to all B-trees, not just the
catalog. The change is conservative (does more work, not less)
minimizing regression risk. The affected code has existed since kernel
3.16, making it applicable to all active stable trees.
Despite the missing explicit stable tags, this is clearly appropriate
stable material - a surgical fix for data corruption that meets all the
technical criteria.
**YES**
fs/hfsplus/bnode.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/fs/hfsplus/bnode.c b/fs/hfsplus/bnode.c
index 63e652ad1e0de..edf7e27e1e375 100644
--- a/fs/hfsplus/bnode.c
+++ b/fs/hfsplus/bnode.c
@@ -704,6 +704,5 @@ bool hfs_bnode_need_zeroout(struct hfs_btree *tree)
struct hfsplus_sb_info *sbi = HFSPLUS_SB(sb);
const u32 volume_attr = be32_to_cpu(sbi->s_vhdr->attributes);
- return tree->cnid == HFSPLUS_CAT_CNID &&
- volume_attr & HFSPLUS_VOL_UNUSED_NODE_FIX;
+ return volume_attr & HFSPLUS_VOL_UNUSED_NODE_FIX;
}
--
2.51.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH AUTOSEL 6.18-5.10] hfsplus: fix volume corruption issue for generic/073
[not found] <20251206140252.645973-1-sashal@kernel.org>
` (3 preceding siblings ...)
2025-12-06 14:02 ` [PATCH AUTOSEL 6.18-5.10] hfsplus: fix volume corruption issue for generic/070 Sasha Levin
@ 2025-12-06 14:02 ` Sasha Levin
4 siblings, 0 replies; 6+ messages in thread
From: Sasha Levin @ 2025-12-06 14:02 UTC (permalink / raw)
To: patches, stable
Cc: Viacheslav Dubeyko, John Paul Adrian Glaubitz, Yangtao Li,
linux-fsdevel, Sasha Levin
From: Viacheslav Dubeyko <slava@dubeyko.com>
[ Upstream commit 24e17a29cf7537f0947f26a50f85319abd723c6c ]
The xfstests' test-case generic/073 leaves HFS+ volume
in corrupted state:
sudo ./check generic/073
FSTYP -- hfsplus
PLATFORM -- Linux/x86_64 hfsplus-testing-0001 6.17.0-rc1+ #4 SMP PREEMPT_DYNAMIC Wed Oct 1 15:02:44 PDT 2025
MKFS_OPTIONS -- /dev/loop51
MOUNT_OPTIONS -- /dev/loop51 /mnt/scratch
generic/073 _check_generic_filesystem: filesystem on /dev/loop51 is inconsistent
(see XFSTESTS-2/xfstests-dev/results//generic/073.full for details)
Ran: generic/073
Failures: generic/073
Failed 1 of 1 tests
sudo fsck.hfsplus -d /dev/loop51
** /dev/loop51
Using cacheBlockSize=32K cacheTotalBlock=1024 cacheSize=32768K.
Executing fsck_hfs (version 540.1-Linux).
** Checking non-journaled HFS Plus Volume.
The volume name is untitled
** Checking extents overflow file.
** Checking catalog file.
** Checking multi-linked files.
** Checking catalog hierarchy.
Invalid directory item count
(It should be 1 instead of 0)
** Checking extended attributes file.
** Checking volume bitmap.
** Checking volume information.
Verify Status: VIStat = 0x0000, ABTStat = 0x0000 EBTStat = 0x0000
CBTStat = 0x0000 CatStat = 0x00004000
** Repairing volume.
** Rechecking volume.
** Checking non-journaled HFS Plus Volume.
The volume name is untitled
** Checking extents overflow file.
** Checking catalog file.
** Checking multi-linked files.
** Checking catalog hierarchy.
** Checking extended attributes file.
** Checking volume bitmap.
** Checking volume information.
** The volume untitled was repaired successfully.
The test is doing these steps on final phase:
mv $SCRATCH_MNT/testdir_1/bar $SCRATCH_MNT/testdir_2/bar
$XFS_IO_PROG -c "fsync" $SCRATCH_MNT/testdir_1
$XFS_IO_PROG -c "fsync" $SCRATCH_MNT/foo
So, we move file bar from testdir_1 into testdir_2 folder. It means that HFS+
logic decrements the number of entries in testdir_1 and increments number of
entries in testdir_2. Finally, we do fsync only for testdir_1 and foo but not
for testdir_2. As a result, this is the reason why fsck.hfsplus detects the
volume corruption afterwards.
This patch fixes the issue by means of adding the
hfsplus_cat_write_inode() call for old_dir and new_dir in
hfsplus_rename() after the successful ending of
hfsplus_rename_cat(). This method makes modification of in-core
inode objects for old_dir and new_dir but it doesn't save these
modifications in Catalog File's entries. It was expected that
hfsplus_write_inode() will save these modifications afterwards.
However, because generic/073 does fsync only for testdir_1 and foo
then testdir_2 modification hasn't beed saved into Catalog File's
entry and it was flushed without this modification. And it was
detected by fsck.hfsplus. Now, hfsplus_rename() stores in Catalog
File all modified entries and correct state of Catalog File will
be flushed during hfsplus_file_fsync() call. Finally, it makes
fsck.hfsplus happy.
sudo ./check generic/073
FSTYP -- hfsplus
PLATFORM -- Linux/x86_64 hfsplus-testing-0001 6.18.0-rc3+ #93 SMP PREEMPT_DYNAMIC Wed Nov 12 14:37:49 PST 2025
MKFS_OPTIONS -- /dev/loop51
MOUNT_OPTIONS -- /dev/loop51 /mnt/scratch
generic/073 32s ... 32s
Ran: generic/073
Passed all 1 tests
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/20251112232522.814038-1-slava@dubeyko.com
Signed-off-by: Viacheslav Dubeyko <slava@dubeyko.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
LLM Generated explanations, may be completely bogus:
Based on my thorough analysis, here is my assessment:
---
## COMMIT ANALYSIS
### 1. PROBLEM DESCRIPTION
This commit fixes a **volume corruption issue** in HFS+ where renaming a
file between directories can lead to an inconsistent catalog file if
only one of the directories is subsequently fsync'd.
**The bug mechanism:**
1. When `hfsplus_rename()` moves a file from `old_dir` to `new_dir`, it
calls `hfsplus_rename_cat()`
2. `hfsplus_rename_cat()` modifies both directories in memory:
- `dst_dir->i_size++` (destination gains an entry)
- `src_dir->i_size--` (source loses an entry)
- Marks both dirty with `hfsplus_mark_inode_dirty()`
3. Marking dirty does NOT write to disk - it just flags for later
writeback
4. If only one directory is fsync'd (as in xfstests generic/073), the
other's changes may be lost
5. This creates a mismatch: the catalog file shows incorrect directory
entry counts
**User-visible symptom:** `fsck.hfsplus` reports "Invalid directory item
count"
### 2. CODE CHANGE ANALYSIS
The fix is **extremely small and surgical** - only 5 lines added:
```c
- if (!res)
+ if (!res) {
new_dentry->d_fsdata = old_dentry->d_fsdata;
+
+ res = hfsplus_cat_write_inode(old_dir);
+ if (!res)
+ res = hfsplus_cat_write_inode(new_dir);
+ }
```
**What it does:** After a successful rename, explicitly calls
`hfsplus_cat_write_inode()` for both directories, which writes their
catalog entries (including the valence/entry count) to the catalog file
immediately.
**Why it's correct:** `hfsplus_cat_write_inode()` is the established
function for writing directory catalog entries in HFS+. The fix ensures
both directories' updated entry counts are persisted immediately after
the rename operation.
### 3. CLASSIFICATION
| Criteria | Assessment |
|----------|------------|
| Bug type | **Filesystem corruption** - data integrity issue |
| Security | Not a CVE, but data corruption is serious |
| Cc: stable tag | **No** - maintainer didn't explicitly request
backport |
| Fixes: tag | **No** - no specific commit cited |
| User impact | HIGH for HFS+ users - volume corruption can cause data
loss |
### 4. SCOPE AND RISK ASSESSMENT
- **Lines changed:** 5 lines added
- **Files touched:** 1 file (`fs/hfsplus/dir.c`)
- **Complexity:** Very low - adds two well-established function calls
- **Bug age:** Since 2013 (commit `892f6668f3a70` introduced
`hfsplus_rename`)
- **Risk of regression:** **LOW** - the function being called is already
used throughout HFS+ codebase
### 5. USER IMPACT
- **Who uses HFS+?** Mac users dual-booting, legacy Apple volumes, some
embedded systems
- **Severity:** HIGH - filesystem corruption affects data integrity
- **Reproducibility:** Easily triggered by xfstests generic/073
### 6. DEPENDENCY CHECK
- `hfsplus_cat_write_inode()` has existed since the early days of
hfsplus
- No dependency on other recent commits
- Should apply cleanly to all stable trees that include hfsplus
---
## VERDICT
**This commit SHOULD be backported** because:
1. **Fixes a real, serious bug:** Volume corruption is a significant
data integrity issue
2. **Obviously correct:** The fix ensures both directories are written
after rename - this is fundamental filesystem consistency
3. **Small and contained:** Only 5 lines in a single file, using
existing functions
4. **Low regression risk:** Uses well-established
`hfsplus_cat_write_inode()` function
5. **Long-standing bug:** Has affected users since 2013
6. **Reproducible:** Standard xfstests test case triggers the bug
7. **Follows established patterns:** Similar to how other filesystems
handle rename consistency
While the commit lacks explicit "Cc: stable" and "Fixes:" tags, the
stable kernel rules clearly allow important bug fixes for data
corruption issues to be backported. The fix is minimal, well-understood,
and addresses a real problem that affects HFS+ users.
Sources:
- [xfstests generic test for fsync after renaming
directory](https://patchwork.kernel.org/project/linux-
btrfs/patch/1459330722-3227-1-git-send-email-fdmanana@kernel.org/)
- [xfstests GitHub repository](https://github.com/kdave/xfstests)
**YES**
fs/hfsplus/dir.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/fs/hfsplus/dir.c b/fs/hfsplus/dir.c
index 1b3e27a0d5e03..cadf0b5f93422 100644
--- a/fs/hfsplus/dir.c
+++ b/fs/hfsplus/dir.c
@@ -552,8 +552,13 @@ static int hfsplus_rename(struct mnt_idmap *idmap,
res = hfsplus_rename_cat((u32)(unsigned long)old_dentry->d_fsdata,
old_dir, &old_dentry->d_name,
new_dir, &new_dentry->d_name);
- if (!res)
+ if (!res) {
new_dentry->d_fsdata = old_dentry->d_fsdata;
+
+ res = hfsplus_cat_write_inode(old_dir);
+ if (!res)
+ res = hfsplus_cat_write_inode(new_dir);
+ }
return res;
}
--
2.51.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH AUTOSEL 6.18-5.10] hfsplus: fix missing hfs_bnode_get() in __hfs_bnode_create
[not found] <20251209001610.611575-1-sashal@kernel.org>
@ 2025-12-09 0:15 ` Sasha Levin
0 siblings, 0 replies; 6+ messages in thread
From: Sasha Levin @ 2025-12-09 0:15 UTC (permalink / raw)
To: patches, stable
Cc: Yang Chenzhi, syzbot+005d2a9ecd9fbf525f6a, Viacheslav Dubeyko,
Sasha Levin, frank.li, linux-fsdevel
From: Yang Chenzhi <yang.chenzhi@vivo.com>
[ Upstream commit 152af114287851583cf7e0abc10129941f19466a ]
When sync() and link() are called concurrently, both threads may
enter hfs_bnode_find() without finding the node in the hash table
and proceed to create it.
Thread A:
hfsplus_write_inode()
-> hfsplus_write_system_inode()
-> hfs_btree_write()
-> hfs_bnode_find(tree, 0)
-> __hfs_bnode_create(tree, 0)
Thread B:
hfsplus_create_cat()
-> hfs_brec_insert()
-> hfs_bnode_split()
-> hfs_bmap_alloc()
-> hfs_bnode_find(tree, 0)
-> __hfs_bnode_create(tree, 0)
In this case, thread A creates the bnode, sets refcnt=1, and hashes it.
Thread B also tries to create the same bnode, notices it has already
been inserted, drops its own instance, and uses the hashed one without
getting the node.
```
node2 = hfs_bnode_findhash(tree, cnid);
if (!node2) { <- Thread A
hash = hfs_bnode_hash(cnid);
node->next_hash = tree->node_hash[hash];
tree->node_hash[hash] = node;
tree->node_hash_cnt++;
} else { <- Thread B
spin_unlock(&tree->hash_lock);
kfree(node);
wait_event(node2->lock_wq,
!test_bit(HFS_BNODE_NEW, &node2->flags));
return node2;
}
```
However, hfs_bnode_find() requires each call to take a reference.
Here both threads end up setting refcnt=1. When they later put the node,
this triggers:
BUG_ON(!atomic_read(&node->refcnt))
In this scenario, Thread B in fact finds the node in the hash table
rather than creating a new one, and thus must take a reference.
Fix this by calling hfs_bnode_get() when reusing a bnode newly created by
another thread to ensure the refcount is updated correctly.
A similar bug was fixed in HFS long ago in commit
a9dc087fd3c4 ("fix missing hfs_bnode_get() in __hfs_bnode_create")
but the same issue remained in HFS+ until now.
Reported-by: syzbot+005d2a9ecd9fbf525f6a@syzkaller.appspotmail.com
Signed-off-by: Yang Chenzhi <yang.chenzhi@vivo.com>
Signed-off-by: Viacheslav Dubeyko <slava@dubeyko.com>
Link: https://lore.kernel.org/r/20250829093912.611853-1-yang.chenzhi@vivo.com
Signed-off-by: Viacheslav Dubeyko <slava@dubeyko.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
LLM Generated explanations, may be completely bogus:
## Analysis Summary
### 1. COMMIT MESSAGE ANALYSIS
**Subject:** "hfsplus: fix missing hfs_bnode_get() in
__hfs_bnode_create"
**Key signals:**
- Clear "fix" keyword indicating bug fix
- Reported-by syzbot - reproducible crash bug
- Detailed race condition explanation with call stacks
- References identical HFS fix from 2022 (commit a9dc087fd3c4)
- No explicit `Cc: stable` tag, but no `Fixes:` tag either since the bug
exists from the file's creation
### 2. CODE CHANGE ANALYSIS
**The Bug:** When `sync()` and `link()` are called concurrently, both
threads may race into `__hfs_bnode_create()`:
- Thread A creates a bnode with `refcnt=1` and inserts it into the hash
table
- Thread B finds the hash table entry, but returns the node **without
incrementing refcnt**
- Both threads believe they own a reference, but only one reference
exists
- When both call `hfs_bnode_put()`, the second call triggers:
`BUG_ON(!atomic_read(&node->refcnt))`
**The Fix:** Single line addition at `fs/hfsplus/bnode.c:484`:
```c
} else {
+ hfs_bnode_get(node2); // <-- Missing refcount increment added
spin_unlock(&tree->hash_lock);
kfree(node);
```
**Why it's correct:** `hfs_bnode_get()` simply does
`atomic_inc(&node->refcnt)` (line 658), ensuring correct reference
counting when reusing a shared bnode.
### 3. CLASSIFICATION
- **Bug fix:** YES - fixes a crash (BUG_ON kernel panic)
- **Feature addition:** NO
- **Security consideration:** Crash can be triggered by normal
operations - potential DoS vector
### 4. SCOPE AND RISK ASSESSMENT
| Metric | Value |
|--------|-------|
| Lines changed | 1 |
| Files touched | 1 |
| Complexity | Very low |
| Subsystem | HFS+ filesystem |
| Regression risk | Very low |
The fix is a **single function call** that mirrors a proven fix from HFS
(commit a9dc087fd3c4) that has been stable since December 2022.
### 5. USER IMPACT
- **Affected users:** Anyone using HFS+ filesystems (common for Mac disk
compatibility, external drives, dual-boot systems)
- **Trigger condition:** Concurrent sync() and link() operations - can
occur in normal workloads
- **Severity:** **KERNEL CRASH** (BUG_ON triggers panic)
### 6. STABILITY INDICATORS
- **syzbot reported:** Bug is reproducible
- **Maintainer signed:** Yes (Viacheslav Dubeyko, HFS+ maintainer)
- **LKML link:** Present
- **Precedent:** Identical fix applied to HFS in 2022 with no
regressions
### 7. DEPENDENCY CHECK
- **Dependencies:** None - completely self-contained
- **Applies to stable:** The affected code pattern has existed unchanged
for many years in stable trees
## Final Assessment
**This commit should be backported to stable kernels.**
**Rationale:**
1. **Fixes a real crash** - BUG_ON triggers kernel panic in a
reproducible race condition
2. **Minimal and surgical** - Single line change adding one function
call
3. **Obviously correct** - Adds missing reference count increment,
matching HFS pattern
4. **Proven safe** - Identical fix in HFS has been stable for 2+ years
5. **No dependencies** - Will apply cleanly to all stable kernels
6. **Real user impact** - HFS+ is commonly used for Mac disk
compatibility
The lack of explicit `Cc: stable` tag does not preclude backporting when
all other stable criteria are clearly met. This is a textbook case of a
small, obviously correct fix for a real crash bug.
**YES**
fs/hfsplus/bnode.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/fs/hfsplus/bnode.c b/fs/hfsplus/bnode.c
index 63e652ad1e0de..63768cf0cb1ba 100644
--- a/fs/hfsplus/bnode.c
+++ b/fs/hfsplus/bnode.c
@@ -481,6 +481,7 @@ static struct hfs_bnode *__hfs_bnode_create(struct hfs_btree *tree, u32 cnid)
tree->node_hash[hash] = node;
tree->node_hash_cnt++;
} else {
+ hfs_bnode_get(node2);
spin_unlock(&tree->hash_lock);
kfree(node);
wait_event(node2->lock_wq,
--
2.51.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-12-09 0:17 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20251206140252.645973-1-sashal@kernel.org>
2025-12-06 14:02 ` [PATCH AUTOSEL 6.18-5.10] hfsplus: Verify inode mode when loading from disk Sasha Levin
2025-12-06 14:02 ` [PATCH AUTOSEL 6.18] hfsplus: fix volume corruption issue for generic/101 Sasha Levin
2025-12-06 14:02 ` [PATCH AUTOSEL 6.18-5.10] hfsplus: fix missing hfs_bnode_get() in __hfs_bnode_create Sasha Levin
2025-12-06 14:02 ` [PATCH AUTOSEL 6.18-5.10] hfsplus: fix volume corruption issue for generic/070 Sasha Levin
2025-12-06 14:02 ` [PATCH AUTOSEL 6.18-5.10] hfsplus: fix volume corruption issue for generic/073 Sasha Levin
[not found] <20251209001610.611575-1-sashal@kernel.org>
2025-12-09 0:15 ` [PATCH AUTOSEL 6.18-5.10] hfsplus: fix missing hfs_bnode_get() in __hfs_bnode_create Sasha Levin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).