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

[Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>]

On Fri, 2026-01-30 at 15:13 +0530, Shardul Bankar wrote:
> 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.

Yeah, I missed this point. You are correct here.

> 
> 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.

Sounds like a better option.

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

Sounds good.

> 
> I will, of course, address your other comments regarding named
> constants and pointer arithmetic in v3.
> 
> Does this sound acceptable?

Yes, it sounds reasonably well. I like your suggestions.

Thanks,
Slava.