Re: [PATCH v2] hfs: prevent MDB and bitmap buffer_head aliasing

[Yue Sun]

On Thu, Jun 4, 2026 at 12:25 AM <slava@xxxxxxxxxxx> wrote:
>
> Could you please take a deeper look into my diff that I've shared with
> you before? I don't see the point to review your suggestion if you
> completely ignored my diff. From my point of view you suggested
> completely the same approach but in more complicated manner.

You are right. I am sorry for the confusion.

I should have looked more carefully at your diff and based my reply on
it. Adding an HFS-level MDB mutex and avoiding holding mdb_bh across the
whole hfs_mdb_commit() path is the right direction. My previous reply
mixed that with extra cleanups and a separate corrupted-layout check,
which made it look like I was proposing a different approach. That was
my mistake.

I am not very familiar with HFS internals, so please treat the following
only as a small suggestion. After re-reading your diff, the only detail
I am still unsure about is the HFS_FLG_ALT_MDB_DIRTY path. In that path,
hfs_inode_write_fork() gets MDB fields as output buffers:

        hfs_inode_write_fork(..., mdb->drXTExtRec,
                             &mdb->drXTFlSize, NULL);
        hfs_inode_write_fork(..., mdb->drCTExtRec,
                             &mdb->drCTFlSize, NULL);

Since mdb points into mdb_bh->b_data, these calls seem to update fields
in the primary MDB buffer. So I was thinking that we could keep your
locking scheme, but move unlock_buffer(mdb_bh) slightly later, after
these primary MDB updates and mark_buffer_dirty(mdb_bh). The alternate
MDB copy/sync and the bitmap writeback would still happen after mdb_bh
is unlocked, so the main deadlock fix remains the same.

If this concern is not valid, or if the current HFS update rules already
make your shorter window sufficient, please ignore this. I defer to your
judgment here.

Below is your diff with only that small adjustment.

Thanks,
Yue

diff --git a/fs/hfs/hfs_fs.h b/fs/hfs/hfs_fs.h
index ac0e83f77a0f..00651f42ba38 100644
--- a/fs/hfs/hfs_fs.h
+++ b/fs/hfs/hfs_fs.h
@@ -66,6 +66,7 @@ struct hfs_inode_info {
  * The HFS-specific part of a Linux (struct super_block)
  */
 struct hfs_sb_info {
+	struct mutex mdb_lock;			/* MDB operations lock */
 	struct buffer_head *mdb_bh;		/* The hfs_buffer
 						   holding the real
 						   superblock (aka VIB
diff --git a/fs/hfs/mdb.c b/fs/hfs/mdb.c
index a97cea35ca2e..d81e3545a045 100644
--- a/fs/hfs/mdb.c
+++ b/fs/hfs/mdb.c
@@ -287,6 +287,7 @@ int hfs_mdb_get(struct super_block *sb)
 void hfs_mdb_commit(struct super_block *sb)
 {
 	struct hfs_mdb *mdb = HFS_SB(sb)->mdb;
+	bool write_backup = false;
 
 	if (sb_rdonly(sb))
 		return;
@@ -314,11 +315,17 @@ void hfs_mdb_commit(struct super_block *sb)
 	 * files grow. */
 	if (test_and_clear_bit(HFS_FLG_ALT_MDB_DIRTY, &HFS_SB(sb)->flags) &&
 	    HFS_SB(sb)->alt_mdb) {
+		write_backup = true;
 		hfs_inode_write_fork(HFS_SB(sb)->ext_tree->inode, mdb->drXTExtRec,
 				     &mdb->drXTFlSize, NULL);
 		hfs_inode_write_fork(HFS_SB(sb)->cat_tree->inode, mdb->drCTExtRec,
 				     &mdb->drCTFlSize, NULL);
 
+		mark_buffer_dirty(HFS_SB(sb)->mdb_bh);
+	}
+	unlock_buffer(HFS_SB(sb)->mdb_bh);
+
+	if (write_backup) {
 		lock_buffer(HFS_SB(sb)->alt_mdb_bh);
 		memcpy(HFS_SB(sb)->alt_mdb, HFS_SB(sb)->mdb, HFS_SECTOR_SIZE);
 		HFS_SB(sb)->alt_mdb->drAtrb |= cpu_to_be16(HFS_SB_ATTRIB_UNMNT);
@@ -360,7 +367,6 @@ void hfs_mdb_commit(struct super_block *sb)
 			size -= len;
 		}
 	}
-	unlock_buffer(HFS_SB(sb)->mdb_bh);
 }
 
 void hfs_mdb_close(struct super_block *sb)
diff --git a/fs/hfs/super.c b/fs/hfs/super.c
index a4f2a2bfa6d3..06ab904b3a42 100644
--- a/fs/hfs/super.c
+++ b/fs/hfs/super.c
@@ -35,7 +35,11 @@ MODULE_LICENSE("GPL");
 static int hfs_sync_fs(struct super_block *sb, int wait)
 {
 	is_hfs_cnid_counts_valid(sb);
+
+	mutex_lock(&HFS_SB(sb)->mdb_lock);
 	hfs_mdb_commit(sb);
+	mutex_unlock(&HFS_SB(sb)->mdb_lock);
+
 	return 0;
 }
 
@@ -68,7 +72,9 @@ static void flush_mdb(struct work_struct *work)
 
 	is_hfs_cnid_counts_valid(sb);
 
+	mutex_lock(&sbi->mdb_lock);
 	hfs_mdb_commit(sb);
+	mutex_unlock(&sbi->mdb_lock);
 }
 
 void hfs_mark_mdb_dirty(struct super_block *sb)
@@ -339,9 +345,12 @@ static int hfs_fill_super(struct super_block *sb, struct fs_context *fc)
 	sb->s_op = &hfs_super_operations;
 	sb->s_xattr = hfs_xattr_handlers;
 	sb->s_flags |= SB_NODIRATIME;
+	mutex_init(&sbi->mdb_lock);
 	mutex_init(&sbi->bitmap_lock);
 
+	mutex_lock(&sbi->mdb_lock);
 	res = hfs_mdb_get(sb);
+	mutex_unlock(&sbi->mdb_lock);
 	if (res) {
 		if (!silent)
 			pr_warn("can't find a HFS filesystem on dev %s\n",