RE: [PATCH v3] hfs: remove BUG() from hfs_release_folio()/hfs_test_inode()/hfs_write_inode()

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

On Sat, 2025-07-26 at 06:52 +0900, Tetsuo Handa wrote:
> On 2025/07/26 2:47, Viacheslav Dubeyko wrote:
> > > I managed to find the offset of rec->dir.DirID in the filesystem image used by
> > > the reproducer, and confirmed that any 0...15 values except 2..4 shall hit BUG()
> > > in hfs_write_inode().
> > > 
> > > Also, a legitimate filesystem image seems to have rec->dir.DirID == 2.
> > > 
> > > That is, the only approach that can avoid hitting BUG() without removing BUG()
> > > would be to verify that rec.type is HFS_CDR_DIR and rec.dir.DirID is HFS_ROOT_CNID.
> > > 
> > > --- a/fs/hfs/super.c
> > > +++ b/fs/hfs/super.c
> > > @@ -354,7 +354,7 @@ static int hfs_fill_super(struct super_block *sb, struct fs_context *fc)
> > >                         goto bail_hfs_find;
> > >                 }
> > >                 hfs_bnode_read(fd.bnode, &rec, fd.entryoffset, fd.entrylength);
> > > -               if (rec.type != HFS_CDR_DIR)
> > > +               if (rec.type != HFS_CDR_DIR || rec.dir.DirID != cpu_to_be32(HFS_ROOT_CNID))
> > >                         res = -EIO;
> > >         }
> > >         if (res)
> > > 
> > > Is this condition correct?
> 
> Please explicitly answer this question.
> 
> Is this validation correct that rec.dir.DirID has to be HFS_ROOT_CNID ?
> 
>  	res = hfs_cat_find_brec(sb, HFS_ROOT_CNID, &fd);
>  	if (!res) {
>  		if (fd.entrylength != sizeof(rec.dir)) {
>  			res =  -EIO;
>  			goto bail_hfs_find;
>  		}
>  		hfs_bnode_read(fd.bnode, &rec, fd.entryoffset, fd.entrylength);
> -		if (rec.type != HFS_CDR_DIR)
> +		if (rec.type != HFS_CDR_DIR || rec.dir.DirID != cpu_to_be32(HFS_ROOT_CNID))
>  			res = -EIO;
>  	}
> 
> I hope that this validation is correct because the "rec" which hfs_bnode_read()
> reads is controlled by the result of hfs_cat_find_brec(HFS_ROOT_CNID).
> 

I don't see the point of making modifications here. The modification of
hfs_read_inode() should be enough.

Thanks,
Slava.