hfsplus: fix potential Allocation File corruption after fsync

The generic/348 test-case has revealed the issue of
HFS+ volume corruption after simulated power failure:

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/348 _check_generic_filesystem: filesystem on /dev/loop51 is inconsistent
(see xfstests-dev/results//generic/348.full for details)

The fsck tool complains about Allocation File (block bitmap)
corruption as a result of such event. The generic/348 creates
a symlink, fsync its parent directory, power fail and mount
again the filesystem. Currently, HFS+ logic has several flags
HFSPLUS_I_CAT_DIRTY, HFSPLUS_I_EXT_DIRTY, HFSPLUS_I_ATTR_DIRTY,
HFSPLUS_I_ALLOC_DIRTY. If inode operation modified the Catalog
File, Extents Overflow File, Attributes File, or Allocation
File, then inode is marked as dirty and one of the mentioned
flags has been set. When hfsplus_file_fsync() has been called,
then this set of flags is checked and dirty b-tree or/and
block bitmap is flushed. However, block bitmap can be modified
during file's content allocation. It means that if we call
hfsplus_file_fsync() for directory, then we never flush
the modified Allocation File in such case because such inode
cannot receive HFSPLUS_I_ALLOC_DIRTY flag. Moreover, this
inode-centric model is not good at all because Catalog File,
Extents Overflow File, Attributes File, and Allocation File
represent the whole state of file system metadata. This
inode-centric policy is the main reason of the issue.

This patch saves the whole approach of using HFSPLUS_I_CAT_DIRTY,
HFSPLUS_I_EXT_DIRTY, HFSPLUS_I_ATTR_DIRTY, and
HFSPLUS_I_ALLOC_DIRTY flags. But Catalog File, Extents Overflow
File, Attributes File, and Allocation File have associated
inodes. And namely these inodes become the mechanism of
checking the dirty state of metadata. The hfsplus_file_fsync()
method checks the dirtiness of file system metadata by
testing HFSPLUS_I_CAT_DIRTY, HFSPLUS_I_EXT_DIRTY,
HFSPLUS_I_ATTR_DIRTY, and HFSPLUS_I_ALLOC_DIRTY flags of
Catalog File's, Extents Overflow File's, Attributes File's, or
Allocation File's inodes. As a result, even if we call
hfsplus_file_fsync() for parent folder, then dirty Allocation File
will be flushed anyway.

Signed-off-by: Viacheslav Dubeyko <[email protected]>
cc: John Paul Adrian Glaubitz <[email protected]>
cc: Yangtao Li <[email protected]>
cc: [email protected]
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Viacheslav Dubeyko <[email protected]>

Author: dubeyko

fs/hfsplus/attributes.c

@@ -241,6 +241,7 @@ int hfsplus_create_attr_nolock(struct inode *inode, const char *name,
 		return err;
 	}
 
+	hfsplus_mark_inode_dirty(HFSPLUS_ATTR_TREE_I(sb), HFSPLUS_I_ATTR_DIRTY);
 	hfsplus_mark_inode_dirty(inode, HFSPLUS_I_ATTR_DIRTY);
 
 	return 0;
@@ -326,6 +327,8 @@ static int __hfsplus_delete_attr(struct inode *inode, u32 cnid,
 	if (err)
 		return err;
 
+	hfsplus_mark_inode_dirty(HFSPLUS_ATTR_TREE_I(inode->i_sb),
+				 HFSPLUS_I_ATTR_DIRTY);
 	hfsplus_mark_inode_dirty(inode, HFSPLUS_I_ATTR_DIRTY);
 	return err;
 }

fs/hfsplus/catalog.c

@@ -313,6 +313,7 @@ int hfsplus_create_cat(u32 cnid, struct inode *dir,
 	if (S_ISDIR(inode->i_mode))
 		hfsplus_subfolders_inc(dir);
 	inode_set_mtime_to_ts(dir, inode_set_ctime_current(dir));
+	hfsplus_mark_inode_dirty(HFSPLUS_CAT_TREE_I(sb), HFSPLUS_I_CAT_DIRTY);
 	hfsplus_mark_inode_dirty(dir, HFSPLUS_I_CAT_DIRTY);
 
 	hfs_find_exit(&fd);
@@ -418,6 +419,7 @@ int hfsplus_delete_cat(u32 cnid, struct inode *dir, const struct qstr *str)
 	if (type == HFSPLUS_FOLDER)
 		hfsplus_subfolders_dec(dir);
 	inode_set_mtime_to_ts(dir, inode_set_ctime_current(dir));
+	hfsplus_mark_inode_dirty(HFSPLUS_CAT_TREE_I(sb), HFSPLUS_I_CAT_DIRTY);
 	hfsplus_mark_inode_dirty(dir, HFSPLUS_I_CAT_DIRTY);
 
 	if (type == HFSPLUS_FILE || type == HFSPLUS_FOLDER) {
@@ -540,6 +542,7 @@ int hfsplus_rename_cat(u32 cnid,
 	}
 	err = hfs_brec_insert(&dst_fd, &entry, entry_size);
 
+	hfsplus_mark_inode_dirty(HFSPLUS_CAT_TREE_I(sb), HFSPLUS_I_CAT_DIRTY);
 	hfsplus_mark_inode_dirty(dst_dir, HFSPLUS_I_CAT_DIRTY);
 	hfsplus_mark_inode_dirty(src_dir, HFSPLUS_I_CAT_DIRTY);
 out:

fs/hfsplus/dir.c

@@ -478,6 +478,9 @@ static int hfsplus_symlink(struct mnt_idmap *idmap, struct inode *dir,
 	if (!inode)
 		goto out;
 
+	hfs_dbg("dir->i_ino %lu, inode->i_ino %lu\n",
+		dir->i_ino, inode->i_ino);
+
 	res = page_symlink(inode, symname, strlen(symname) + 1);
 	if (res)
 		goto out_err;
@@ -526,6 +529,9 @@ static int hfsplus_mknod(struct mnt_idmap *idmap, struct inode *dir,
 	if (!inode)
 		goto out;
 
+	hfs_dbg("dir->i_ino %lu, inode->i_ino %lu\n",
+		dir->i_ino, inode->i_ino);
+
 	if (S_ISBLK(mode) || S_ISCHR(mode) || S_ISFIFO(mode) || S_ISSOCK(mode))
 		init_special_inode(inode, mode, rdev);
 

fs/hfsplus/extents.c

@@ -121,6 +121,8 @@ static int __hfsplus_ext_write_extent(struct inode *inode,
 	 * redirty the inode.  Instead the callers have to be careful
 	 * to explicily mark the inode dirty, too.
 	 */
+	set_bit(HFSPLUS_I_EXT_DIRTY,
+		&HFSPLUS_I(HFSPLUS_EXT_TREE_I(inode->i_sb))->flags);
 	set_bit(HFSPLUS_I_EXT_DIRTY, &hip->flags);
 
 	return 0;
@@ -513,6 +515,8 @@ int hfsplus_file_extend(struct inode *inode, bool zeroout)
 	if (!res) {
 		hip->alloc_blocks += len;
 		mutex_unlock(&hip->extents_lock);
+		hfsplus_mark_inode_dirty(HFSPLUS_SB(sb)->alloc_file,
+					 HFSPLUS_I_ALLOC_DIRTY);
 		hfsplus_mark_inode_dirty(inode, HFSPLUS_I_ALLOC_DIRTY);
 		return 0;
 	}
@@ -582,6 +586,7 @@ void hfsplus_file_truncate(struct inode *inode)
 		/* XXX: We lack error handling of hfsplus_file_truncate() */
 		return;
 	}
+
 	while (1) {
 		if (alloc_cnt == hip->first_blocks) {
 			mutex_unlock(&fd.tree->tree_lock);
@@ -623,5 +628,7 @@ void hfsplus_file_truncate(struct inode *inode)
 	hip->fs_blocks = (inode->i_size + sb->s_blocksize - 1) >>
 		sb->s_blocksize_bits;
 	inode_set_bytes(inode, hip->fs_blocks << sb->s_blocksize_bits);
+	hfsplus_mark_inode_dirty(HFSPLUS_SB(sb)->alloc_file,
+				 HFSPLUS_I_ALLOC_DIRTY);
 	hfsplus_mark_inode_dirty(inode, HFSPLUS_I_ALLOC_DIRTY);
 }

fs/hfsplus/hfsplus_fs.h

@@ -238,6 +238,13 @@ static inline struct hfsplus_inode_info *HFSPLUS_I(struct inode *inode)
 	return container_of(inode, struct hfsplus_inode_info, vfs_inode);
 }
 
+#define HFSPLUS_CAT_TREE_I(sb) \
+	HFSPLUS_SB(sb)->cat_tree->inode
+#define HFSPLUS_EXT_TREE_I(sb) \
+	HFSPLUS_SB(sb)->ext_tree->inode
+#define HFSPLUS_ATTR_TREE_I(sb) \
+	HFSPLUS_SB(sb)->attr_tree->inode
+
 /*
  * Mark an inode dirty, and also mark the btree in which the
  * specific type of metadata is stored.

fs/hfsplus/inode.c

@@ -324,6 +324,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 super_block *sb = inode->i_sb;
 	struct hfsplus_sb_info *sbi = HFSPLUS_SB(inode->i_sb);
 	struct hfsplus_vh *vhdr = sbi->s_vhdr;
 	int error = 0, error2;
@@ -344,29 +345,39 @@ int hfsplus_file_fsync(struct file *file, loff_t start, loff_t end,
 	/*
 	 * And explicitly write out the btrees.
 	 */
-	if (test_and_clear_bit(HFSPLUS_I_CAT_DIRTY, &hip->flags))
+	if (test_and_clear_bit(HFSPLUS_I_CAT_DIRTY,
+				&HFSPLUS_I(HFSPLUS_CAT_TREE_I(sb))->flags)) {
+		clear_bit(HFSPLUS_I_CAT_DIRTY, &hip->flags);
 		error = filemap_write_and_wait(sbi->cat_tree->inode->i_mapping);
+	}
 
-	if (test_and_clear_bit(HFSPLUS_I_EXT_DIRTY, &hip->flags)) {
+	if (test_and_clear_bit(HFSPLUS_I_EXT_DIRTY,
+				&HFSPLUS_I(HFSPLUS_EXT_TREE_I(sb))->flags)) {
+		clear_bit(HFSPLUS_I_EXT_DIRTY, &hip->flags);
 		error2 =
 			filemap_write_and_wait(sbi->ext_tree->inode->i_mapping);
 		if (!error)
 			error = error2;
 	}
 
-	if (test_and_clear_bit(HFSPLUS_I_ATTR_DIRTY, &hip->flags)) {
-		if (sbi->attr_tree) {
+	if (sbi->attr_tree) {
+		if (test_and_clear_bit(HFSPLUS_I_ATTR_DIRTY,
+				&HFSPLUS_I(HFSPLUS_ATTR_TREE_I(sb))->flags)) {
+			clear_bit(HFSPLUS_I_ATTR_DIRTY, &hip->flags);
 			error2 =
 				filemap_write_and_wait(
 					    sbi->attr_tree->inode->i_mapping);
 			if (!error)
 				error = error2;
-		} else {
-			pr_err("sync non-existent attributes tree\n");
 		}
+	} else {
+		if (test_and_clear_bit(HFSPLUS_I_ATTR_DIRTY, &hip->flags))
+			pr_err("sync non-existent attributes tree\n");
 	}
 
-	if (test_and_clear_bit(HFSPLUS_I_ALLOC_DIRTY, &hip->flags)) {
+	if (test_and_clear_bit(HFSPLUS_I_ALLOC_DIRTY,
+				&HFSPLUS_I(sbi->alloc_file)->flags)) {
+		clear_bit(HFSPLUS_I_ALLOC_DIRTY, &hip->flags);
 		error2 = filemap_write_and_wait(sbi->alloc_file->i_mapping);
 		if (!error)
 			error = error2;
@@ -709,6 +720,8 @@ int hfsplus_cat_write_inode(struct inode *inode)
 					 sizeof(struct hfsplus_cat_file));
 	}
 
+	set_bit(HFSPLUS_I_CAT_DIRTY,
+		&HFSPLUS_I(HFSPLUS_CAT_TREE_I(inode->i_sb))->flags);
 	set_bit(HFSPLUS_I_CAT_DIRTY, &HFSPLUS_I(inode)->flags);
 out:
 	hfs_find_exit(&fd);

fs/hfsplus/super.c

@@ -625,6 +625,8 @@ static int hfsplus_fill_super(struct super_block *sb, struct fs_context *fc)
 			}
 
 			mutex_unlock(&sbi->vh_mutex);
+			hfsplus_mark_inode_dirty(HFSPLUS_CAT_TREE_I(sb),
+						 HFSPLUS_I_CAT_DIRTY);
 			hfsplus_mark_inode_dirty(sbi->hidden_dir,
 						 HFSPLUS_I_CAT_DIRTY);
 		}

fs/hfsplus/xattr.c

@@ -236,6 +236,7 @@ static int hfsplus_create_attributes_file(struct super_block *sb)
 		put_page(page);
 	}
 
+	hfsplus_mark_inode_dirty(HFSPLUS_ATTR_TREE_I(sb), HFSPLUS_I_ATTR_DIRTY);
 	hfsplus_mark_inode_dirty(attr_file, HFSPLUS_I_ATTR_DIRTY);
 
 	sbi->attr_tree = hfs_btree_open(sb, HFSPLUS_ATTR_CNID);
@@ -314,8 +315,11 @@ int __hfsplus_setxattr(struct inode *inode, const char *name,
 				hfs_bnode_write(cat_fd.bnode, &entry,
 					cat_fd.entryoffset,
 					sizeof(struct hfsplus_cat_folder));
-				hfsplus_mark_inode_dirty(inode,
+				hfsplus_mark_inode_dirty(
+						HFSPLUS_CAT_TREE_I(inode->i_sb),
 						HFSPLUS_I_CAT_DIRTY);
+				hfsplus_mark_inode_dirty(inode,
+							 HFSPLUS_I_CAT_DIRTY);
 			} else {
 				err = -ERANGE;
 				goto end_setxattr;
@@ -327,8 +331,11 @@ int __hfsplus_setxattr(struct inode *inode, const char *name,
 				hfs_bnode_write(cat_fd.bnode, &entry,
 					cat_fd.entryoffset,
 					sizeof(struct hfsplus_cat_file));
-				hfsplus_mark_inode_dirty(inode,
+				hfsplus_mark_inode_dirty(
+						HFSPLUS_CAT_TREE_I(inode->i_sb),
 						HFSPLUS_I_CAT_DIRTY);
+				hfsplus_mark_inode_dirty(inode,
+							 HFSPLUS_I_CAT_DIRTY);
 			} else {
 				err = -ERANGE;
 				goto end_setxattr;
@@ -381,6 +388,8 @@ int __hfsplus_setxattr(struct inode *inode, const char *name,
 		hfs_bnode_write_u16(cat_fd.bnode, cat_fd.entryoffset +
 				offsetof(struct hfsplus_cat_folder, flags),
 				cat_entry_flags);
+		hfsplus_mark_inode_dirty(HFSPLUS_CAT_TREE_I(inode->i_sb),
+					 HFSPLUS_I_CAT_DIRTY);
 		hfsplus_mark_inode_dirty(inode, HFSPLUS_I_CAT_DIRTY);
 	} else if (cat_entry_type == HFSPLUS_FILE) {
 		cat_entry_flags = hfs_bnode_read_u16(cat_fd.bnode,
@@ -392,6 +401,8 @@ int __hfsplus_setxattr(struct inode *inode, const char *name,
 		hfs_bnode_write_u16(cat_fd.bnode, cat_fd.entryoffset +
 				    offsetof(struct hfsplus_cat_file, flags),
 				    cat_entry_flags);
+		hfsplus_mark_inode_dirty(HFSPLUS_CAT_TREE_I(inode->i_sb),
+					 HFSPLUS_I_CAT_DIRTY);
 		hfsplus_mark_inode_dirty(inode, HFSPLUS_I_CAT_DIRTY);
 	} else {
 		pr_err("invalid catalog entry type\n");
@@ -862,6 +873,8 @@ static int hfsplus_removexattr(struct inode *inode, const char *name)
 		hfs_bnode_write_u16(cat_fd.bnode, cat_fd.entryoffset +
 				offsetof(struct hfsplus_cat_folder, flags),
 				flags);
+		hfsplus_mark_inode_dirty(HFSPLUS_CAT_TREE_I(inode->i_sb),
+					 HFSPLUS_I_CAT_DIRTY);
 		hfsplus_mark_inode_dirty(inode, HFSPLUS_I_CAT_DIRTY);
 	} else if (cat_entry_type == HFSPLUS_FILE) {
 		flags = hfs_bnode_read_u16(cat_fd.bnode, cat_fd.entryoffset +
@@ -873,6 +886,8 @@ static int hfsplus_removexattr(struct inode *inode, const char *name)
 		hfs_bnode_write_u16(cat_fd.bnode, cat_fd.entryoffset +
 				offsetof(struct hfsplus_cat_file, flags),
 				flags);
+		hfsplus_mark_inode_dirty(HFSPLUS_CAT_TREE_I(inode->i_sb),
+					 HFSPLUS_I_CAT_DIRTY);
 		hfsplus_mark_inode_dirty(inode, HFSPLUS_I_CAT_DIRTY);
 	} else {
 		pr_err("invalid catalog entry type\n");