Re: [PATCH v2] hfsplus: validate btree bitmap during mount and handle corruption gracefully

[Shardul Bankar <shardul.b@mpiricsoftware.com>]

On Mon, 2026-01-26 at 22:42 +0000, Viacheslav Dubeyko wrote:
> On Sun, 2026-01-25 at 08:37 +0530, Shardul Bankar wrote:
> > 
> > @@ -176,6 +238,13 @@ struct hfs_btree *hfs_btree_open(struct
> > super_block *sb, u32 id)
> >         tree->max_key_len = be16_to_cpu(head->max_key_len);
> >         tree->depth = be16_to_cpu(head->depth);
> >  
> > +       /* Validate bitmap: node 0 must be marked allocated */
> > +       if (hfsplus_validate_btree_bitmap(tree, head)) {
> > +               struct hfsplus_sb_info *sbi = HFSPLUS_SB(sb);
> > +
> > +               sbi->btree_bitmap_corrupted = true;
> 
> Please, see my comment about this field.
> 
> > +       }
> > +
> >         /* Verify the tree and set the correct compare function */
> >         switch (id) {
> >         case HFSPLUS_EXT_CNID:
> > diff --git a/fs/hfsplus/hfsplus_fs.h b/fs/hfsplus/hfsplus_fs.h
> > index 45fe3a12ecba..b925878333d4 100644
> > --- a/fs/hfsplus/hfsplus_fs.h
> > +++ b/fs/hfsplus/hfsplus_fs.h
> > @@ -154,6 +154,7 @@ struct hfsplus_sb_info {
> >  
> >         int part, session;
> >         unsigned long flags;
> > +       bool btree_bitmap_corrupted;    /* Bitmap corruption
> > detected during btree open */
> 
> This field is completely unnecessary. The hfs_btree_open() can return
> -EROFS
> error code and hfsplus_fill_super() can process it.
>   
Hi Slava,

Thanks for the review.

Regarding the suggestion to convert hfs_btree_open() to return
ERR_PTR(-EROFS):

I reviewed this, but I cannot use ERR_PTR for the corruption case
because it would defeat a purpose of the patch (data recovery).

If hfs_btree_open() returns -EROFS, the caller hfsplus_fill_super()
would receive the error code but would have no tree object to work
with. Without the B-tree structure, we cannot mount the filesystem-even
read-only-making data recovery impossible.

To support recovery, hfs_btree_open() must return a valid tree pointer
even when corruption is detected.

Therefore, for v3, I plan to:

    -Keep the return type as-is to avoid scope creep and ensure easy
backporting.

    -Use sb->s_flags |= SB_RDONLY inside hfs_btree_open() to flag the
safety issue.

    -Drop the bool flag I added in v2 (as you requested) and simply
check sb_rdonly(sb) in fill_super to print the warning.

I will, of course, address your other comments regarding named
constants and pointer arithmetic in v3.

Does this sound acceptable?

Thanks,
Shardul