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

[Viacheslav Dubeyko]

On Fri, 2026-06-05 at 02:02 +0800, Yue Sun wrote:
> 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);


I've started to think that, maybe, the location of these
hfs_inode_write_fork() calls is not correct. We are trying to save the
current size of Catalog File and Extents Overflow File. OK, we need to
update this if size is changed. But we can save the sizes during every
call of hfs_mdb_commit(). It's logically incorrect to call these
methods in the alternative MDB section because it is the content of
primary MDB. I suggest to move these two hfs_inode_write_fork() to
primary MDB writing section:

lock_buffer(HFS_SB(sb)->mdb_bh);
		/* These parameters may have been modified, so write
them back */
		mdb->drLsMod = hfs_mtime();
		mdb->drFreeBks = cpu_to_be16(HFS_SB(sb)-
>free_ablocks);
		mdb->drNxtCNID =
			cpu_to_be32((u32)atomic64_read(&HFS_SB(sb)-
>next_id));
		mdb->drNmFls = cpu_to_be16(HFS_SB(sb)->root_files);
		mdb->drNmRtDirs = cpu_to_be16(HFS_SB(sb)->root_dirs);
		mdb->drFilCnt =
			cpu_to_be32((u32)atomic64_read(&HFS_SB(sb)-
>file_count));
		mdb->drDirCnt =
			cpu_to_be32((u32)atomic64_read(&HFS_SB(sb)-
>folder_count));

		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);

		/* write MDB to disk */
		mark_buffer_dirty(HFS_SB(sb)->mdb_bh);
unlock_buffer(HFS_SB(sb)->mdb_bh);

In this case, we don;'t need to overlap the locks. What do you think?
Sounds like reasonable solution?

Thanks,
Slava.


> 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",