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 <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/20260220220152.152721-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 4b79cd606276e31c20fa18ef3a099596f50e8a0f..6585bcea731c3e517ff281145b35ad31fc03a46b 100644 (file)
--- a/fs/hfsplus/attributes.c
+++ b/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;
 }

diff --git a/fs/hfsplus/catalog.c b/fs/hfsplus/catalog.c
index 02c1eee4a4b86059ceaab7a7c68ab65adba6fa26..eef7412a4d58bcfb040ada5a9e4bf4df0ed70815 100644 (file)
--- a/fs/hfsplus/catalog.c
+++ b/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:

diff --git a/fs/hfsplus/dir.c b/fs/hfsplus/dir.c
index d559bf8625f853d50fd316d157cf8afe22069565..9f0345bf1b5aa0c870d093eff40a23f5fc5aa9f4 100644 (file)
--- a/fs/hfsplus/dir.c
+++ b/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);
 

diff --git a/fs/hfsplus/extents.c b/fs/hfsplus/extents.c
index 8e886514d27f1e5d4d94be75142f197669e62234..a5f772de9985d954af9ec763c18e41dd551e9ce2 100644 (file)
--- a/fs/hfsplus/extents.c
+++ b/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 @@ out:
        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 @@ out_unlock:
        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);
 }

diff --git a/fs/hfsplus/hfsplus_fs.h b/fs/hfsplus/hfsplus_fs.h
index 5f891b73a64677bfb1d132df94c95beabd0be3cf..122ab57193bbec584b0d91a448753fb4de2070a3 100644 (file)
--- a/fs/hfsplus/hfsplus_fs.h
+++ b/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.

diff --git a/fs/hfsplus/inode.c b/fs/hfsplus/inode.c
index 922ff41df042a83d47364f2d941c45dabda29afb..cdf08393de44e3d9313592130d8600f3bab8a30d 100644 (file)
--- a/fs/hfsplus/inode.c
+++ b/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);

diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c
index 7229a8ae89f9469109b1c3a317ee9b7705a83f8b..2a135c78d0b2dc94520216b8f06fa82a4b2c47e9 100644 (file)
--- a/fs/hfsplus/super.c
+++ b/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);
                }

diff --git a/fs/hfsplus/xattr.c b/fs/hfsplus/xattr.c
index 9904944cbd54e3d326591fa65a5ed678f38ca583..31b6cb9db77083bb522e1cb679f51a2b2c1b4f99 100644 (file)
--- a/fs/hfsplus/xattr.c
+++ b/fs/hfsplus/xattr.c
@@ -236,6 +236,7 @@ check_attr_tree_state_again:
                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");