Re: [PATCH v4 2/2] hfsplus: validate b-tree fork extents at mount time

[Nguyen Ngoc Thang]

Hi Slava,

Thanks again for the review, replies inline, v5 diff (applies on top
of the v5 1/2 I just sent) at the bottom.

> If we return error code for corrupted fork (that makes more sense),
> then we don't need in switch here.
>
> > +     case -EIO:
> > +             pr_err("%s (cnid 0x%x) fork's first extent is
> > corrupt\n",
> > +                     hfs_btree_name(id), id);
> > +             goto free_inode;
> > +     case 1:
>
> I don't see the point returning 1 from the function. It should be error
> code.

Agreed, done. hfsplus_check_fork() now returns 0 (consistent),
-EUCLEAN (corrupt past the first extent, tree still locatable, mount
read-only), or -EIO (first extent corrupt, or no used extent at all --
see below). hfs_btree_open() now just checks the return value with
if/else instead of switching on it.

> Why not struct hfsplus_fork_raw here for check?

I looked into this, but the b-tree's inode only keeps the decoded
first_extents/first_blocks fields (see hfsplus_iget()), not the raw
hfsplus_fork_raw (total_size/clump_size/total_blocks as a struct) --
that only exists transiently while reading the volume header. Passing
the raw fork through would mean plumbing it from hfsplus_fill_super()
into hfs_btree_open() as an extra argument, which felt like a bigger
restructuring than this patch should take on. I'd rather scope that as
a follow-up than guess at it here -- let me know if you disagree and
I'll take a pass at it.

> Ditto. Related to hardcoded value.
>
> > +     for (i = 0; i < 8; i++, ext++) {

Uses HFSPLUS_EXTENT_COUNT now too (same constant added in patch 1/2).

> I think that current logic of check looks complicated. [...] For
> example, fork cannot be completely empty. Could we rework the logic
> to be more clear? Maybe, we need to introduce the function for extent
> check, function for checking the extents are logically contiguous?

Fixed the empty-fork case: hfsplus_check_fork() now tracks seen_used
and returns -EIO if no extent was ever in use. Note hs_btree_open()
already guarded against this indirectly via its existing
`!first_blocks` check right before calling hfsplus_check_fork(), so
this makes the function correct on its own instead of relying on that
caller-side check.

I held off on splitting per-extent-check and contiguity-check into
separate functions -- the loop is short and the two conditions
(garbage in an unused slot vs. a used extent overflowing/following a
hole) share the same start/count/seen_hole state per iteration, so
splitting it looked like it'd add indirection without really
clarifying anything. Happy to revisit if you still think it's worth
it.

> Ditto. Related to 1. I prefer to have error code instead.

Same fix as above (-EUCLEAN).

> I don't want to say that this direction is wrong. However, we have
> flags: [...] Potentially, we can introduce the HFSPLUS_I_CORRUPT_TREE.

Done -- dropped struct hfs_btree.corrupt, added HFSPLUS_I_CORRUPT_TREE
next to the existing HFSPLUS_I_*_DIRTY flags, tested via a new
HFSPLUS_TREE_IS_CORRUPT(tree) helper macro on the tree's own inode.

> Currently, only hfsplus_fill_super() can detect the b-tree
> corruption. Why do we have the check here? Do you mean that xattr
> b-tree can be created and to be corrupted?

No -- corruption is only ever detected once, in hfs_btree_open() at
initial mount. The hfsplus_reconfigure() check isn't detecting
anything new; it's re-reading the flag hfs_btree_open() already set,
so that a remount to rw can't silently clear SB_RDONLY on a volume
that was already known to be corrupt at mount time. Added a short
comment there to make that explicit.

> If we fail to check any b-tree, then logic should stop. Why haven't
> we checked the error code of hfs_btree_open()?

I checked -- hfsplus_fill_super() already does check every
hfs_btree_open() call (out_close_ext_tree / out_close_cat_tree /
out_close_attr_tree gotos) before it ever looks at
HFSPLUS_TREE_IS_CORRUPT(), so no change was needed there.

One more from the previous mail I noticed while redoing this: there
was also a checkpatch --strict alignment nit on the pr_err()
continuation line in hfs_btree_open() itself (not one you'd flagged,
but same category), fixed that too while I was in there.

Thanks again for the thorough review -- v5 below.

---
Changes since v4:
 - hfsplus_check_fork() returns real error codes (0/-EUCLEAN/-EIO)
   instead of 0/1/-EIO; hfs_btree_open() uses if/else instead of a
   switch.
 - A fork with no used extent at all is now treated as corrupt.
 - Use HFSPLUS_EXTENT_COUNT instead of hardcoding 8.
 - Track per-tree corruption as an HFSPLUS_I_CORRUPT_TREE inode flag
   instead of a bool on struct hfs_btree.
 - Comment explaining the corrupt-tree check in hfsplus_reconfigure().
 - Fixed a checkpatch --strict alignment nit in hfs_btree_open().
(all per Slava's review)

diff --git a/fs/hfsplus/btree.c b/fs/hfsplus/btree.c
index 2ea8cd5658e1..2dbbb8096575 100644
--- a/fs/hfsplus/btree.c
+++ b/fs/hfsplus/btree.c
@@ -274,6 +274,7 @@ struct hfs_btree *hfs_btree_open(struct super_block *sb, u32 id)
 	struct inode *inode;
 	struct page *page;
 	unsigned int size;
+	int res;

 	tree = kzalloc_obj(*tree);
 	if (!tree)
@@ -293,6 +294,17 @@ struct hfs_btree *hfs_btree_open(struct super_block *sb, u32 id)
 		goto free_inode;
 	}

+	res = hfsplus_check_fork(sb, HFSPLUS_I(tree->inode)->first_extents);
+	if (res == -EIO) {
+		pr_err("%s (cnid 0x%x) fork's first extent is corrupt\n",
+		       hfs_btree_name(id), id);
+		goto free_inode;
+	} else if (res) {
+		pr_warn("%s (cnid 0x%x) fork has corrupt extents, forcing read-only.\n",
+			hfs_btree_name(id), id);
+		set_bit(HFSPLUS_I_CORRUPT_TREE, &HFSPLUS_I(tree->inode)->flags);
+	}
+
 	mapping = tree->inode->i_mapping;
 	page = read_mapping_page(mapping, 0, NULL);
 	if (IS_ERR(page))
diff --git a/fs/hfsplus/extents.c b/fs/hfsplus/extents.c
index f3a4b8fd567f..d98261c01b13 100644
--- a/fs/hfsplus/extents.c
+++ b/fs/hfsplus/extents.c
@@ -101,6 +101,39 @@ static bool hfsplus_ext_fork_full(struct hfsplus_extent *ext)
 	return true;
 }

+/*
+ * Validate a fork's extents. Returns 0 if consistent, -EUCLEAN if only
+ * extents past the first are corrupt (safe to mount read-only), or
+ * -EIO if the first extent is corrupt or the fork has no used extent.
+ */
+int hfsplus_check_fork(struct super_block *sb, struct hfsplus_extent *ext)
+{
+	struct hfsplus_sb_info *sbi = HFSPLUS_SB(sb);
+	bool seen_hole = false;
+	bool seen_used = false;
+	int i;
+
+	for (i = 0; i < HFSPLUS_EXTENT_COUNT; i++, ext++) {
+		u32 start = be32_to_cpu(ext->start_block);
+		u32 count = be32_to_cpu(ext->block_count);
+		bool bad;
+
+		if (!count) {
+			bad = start != 0;
+			seen_hole = true;
+		} else {
+			bad = seen_hole || start + count < start ||
+			      start + count > sbi->total_blocks;
+			seen_used = true;
+		}
+
+		if (bad)
+			return i ? -EUCLEAN : -EIO;
+	}
+
+	return seen_used ? 0 : -EIO;
+}
+
 static int __hfsplus_ext_write_extent(struct inode *inode,
 		struct hfs_find_data *fd)
 {
diff --git a/fs/hfsplus/hfsplus_fs.h b/fs/hfsplus/hfsplus_fs.h
index 7c53832f2784..3290812c0fea 100644
--- a/fs/hfsplus/hfsplus_fs.h
+++ b/fs/hfsplus/hfsplus_fs.h
@@ -230,10 +230,15 @@ struct hfsplus_inode_info {
 #define HFSPLUS_I_EXT_DIRTY	2	/* has changes in the extent tree */
 #define HFSPLUS_I_ALLOC_DIRTY	3	/* has changes in the allocation file */
 #define HFSPLUS_I_ATTR_DIRTY	4	/* has changes in the attributes tree */
+#define HFSPLUS_I_CORRUPT_TREE	5	/* tree's fork had corrupt extents at open time */

 #define HFSPLUS_IS_RSRC(inode) \
 	test_bit(HFSPLUS_I_RSRC, &HFSPLUS_I(inode)->flags)

+/* Test HFSPLUS_I_CORRUPT_TREE on the tree's own inode */
+#define HFSPLUS_TREE_IS_CORRUPT(tree) \
+	test_bit(HFSPLUS_I_CORRUPT_TREE, &HFSPLUS_I((tree)->inode)->flags)
+
 static inline struct hfsplus_inode_info *HFSPLUS_I(struct inode *inode)
 {
 	return container_of(inode, struct hfsplus_inode_info, vfs_inode);
@@ -443,6 +448,7 @@ int hfsplus_free_fork(struct super_block *sb, u32 cnid,
 		      struct hfsplus_fork_raw *fork, int type);
 int hfsplus_file_extend(struct inode *inode, bool zeroout);
 void hfsplus_file_truncate(struct inode *inode);
+int hfsplus_check_fork(struct super_block *sb, struct hfsplus_extent *ext);

 /* inode.c */
 extern const struct address_space_operations hfsplus_aops;
diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c
index ff7d6b3336a6..a1669bd45701 100644
--- a/fs/hfsplus/super.c
+++ b/fs/hfsplus/super.c
@@ -400,6 +400,14 @@ static int hfsplus_reconfigure(struct fs_context *fc)
 			pr_warn("filesystem is marked journaled, leaving read-only.\n");
 			sb->s_flags |= SB_RDONLY;
 			fc->sb_flags |= SB_RDONLY;
+		} else if (HFSPLUS_TREE_IS_CORRUPT(sbi->ext_tree) ||
+				HFSPLUS_TREE_IS_CORRUPT(sbi->cat_tree) ||
+				(sbi->attr_tree &&
+				 HFSPLUS_TREE_IS_CORRUPT(sbi->attr_tree))) {
+			/* Re-checks the flag hfs_btree_open() set at mount */
+			pr_warn("a b-tree fork was corrupt at mount time, leaving read-only.\n");
+			sb->s_flags |= SB_RDONLY;
+			fc->sb_flags |= SB_RDONLY;
 		}
 	}
 	return 0;
@@ -564,6 +572,11 @@ static int hfsplus_fill_super(struct super_block *sb, struct fs_context *fc)
 	}
 	sb->s_xattr = hfsplus_xattr_handlers;

+	if (HFSPLUS_TREE_IS_CORRUPT(sbi->ext_tree) ||
+	    HFSPLUS_TREE_IS_CORRUPT(sbi->cat_tree) ||
+	    (sbi->attr_tree && HFSPLUS_TREE_IS_CORRUPT(sbi->attr_tree)))
+		sb->s_flags |= SB_RDONLY;
+
 	inode = hfsplus_iget(sb, HFSPLUS_ALLOC_CNID);
 	if (IS_ERR(inode)) {
 		pr_err("failed to load allocation file\n");
--
Thanks,
Nguyen Ngoc Thang