[PATCH] hfs: don't use BUG() when we can continue

[PATCH] hfs: don't use BUG() when we can continue

[Tetsuo Handa]

syzkaller can mount crafted filesystem images.
Don't crash the kernel when we can continue.

Reported-by: syzbot+97e301b4b82ae803d21b@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=97e301b4b82ae803d21b
Tested-by: syzbot+97e301b4b82ae803d21b@syzkaller.appspotmail.com
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 fs/hfs/inode.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/hfs/inode.c b/fs/hfs/inode.c
index a81ce7a740b9..794d710c3ae0 100644
--- a/fs/hfs/inode.c
+++ b/fs/hfs/inode.c
@@ -81,7 +81,7 @@ static bool hfs_release_folio(struct folio *folio, gfp_t mask)
 		tree = HFS_SB(sb)->cat_tree;
 		break;
 	default:
-		BUG();
+		pr_warn("unexpected inode %lu at %s()\n", inode->i_ino, __func__);
 		return false;
 	}
 
@@ -305,7 +305,7 @@ static int hfs_test_inode(struct inode *inode, void *data)
 	case HFS_CDR_FIL:
 		return inode->i_ino == be32_to_cpu(rec->file.FlNum);
 	default:
-		BUG();
+		pr_warn("unexpected type %u at %s()\n", rec->type, __func__);
 		return 1;
 	}
 }
@@ -441,7 +441,7 @@ int hfs_write_inode(struct inode *inode, struct writeback_control *wbc)
 			hfs_btree_write(HFS_SB(inode->i_sb)->cat_tree);
 			return 0;
 		default:
-			BUG();
+			pr_warn("unexpected inode %lu at %s()\n", inode->i_ino, __func__);
 			return -EIO;
 		}
 	}
-- 
2.47.0

[PATCH (REPOST)] hfs: don't use BUG() when we can continue

[Tetsuo Handa]

syzkaller can mount crafted filesystem images.
Don't crash the kernel when we can continue.

Reported-by: syzbot+97e301b4b82ae803d21b@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=97e301b4b82ae803d21b
Tested-by: syzbot+97e301b4b82ae803d21b@syzkaller.appspotmail.com
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 fs/hfs/inode.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/hfs/inode.c b/fs/hfs/inode.c
index a81ce7a740b9..794d710c3ae0 100644
--- a/fs/hfs/inode.c
+++ b/fs/hfs/inode.c
@@ -81,7 +81,7 @@ static bool hfs_release_folio(struct folio *folio, gfp_t mask)
 		tree = HFS_SB(sb)->cat_tree;
 		break;
 	default:
-		BUG();
+		pr_warn("unexpected inode %lu at %s()\n", inode->i_ino, __func__);
 		return false;
 	}
 
@@ -305,7 +305,7 @@ static int hfs_test_inode(struct inode *inode, void *data)
 	case HFS_CDR_FIL:
 		return inode->i_ino == be32_to_cpu(rec->file.FlNum);
 	default:
-		BUG();
+		pr_warn("unexpected type %u at %s()\n", rec->type, __func__);
 		return 1;
 	}
 }
@@ -441,7 +441,7 @@ int hfs_write_inode(struct inode *inode, struct writeback_control *wbc)
 			hfs_btree_write(HFS_SB(inode->i_sb)->cat_tree);
 			return 0;
 		default:
-			BUG();
+			pr_warn("unexpected inode %lu at %s()\n", inode->i_ino, __func__);
 			return -EIO;
 		}
 	}
-- 
2.47.0

Re: [PATCH (REPOST)] hfs: don't use BUG() when we can continue

[Matthew Wilcox]

On Thu, Dec 05, 2024 at 10:45:11PM +0900, Tetsuo Handa wrote:
> syzkaller can mount crafted filesystem images.
> Don't crash the kernel when we can continue.

The one in hfs_test_inode() makes sense, but we should never get to
hfs_release_folio() or hfs_write_inode() with a bad inode, even with a
corrupted fs.  Right?

> +++ b/fs/hfs/inode.c
> @@ -81,7 +81,7 @@ static bool hfs_release_folio(struct folio *folio, gfp_t mask)
>  		tree = HFS_SB(sb)->cat_tree;
>  		break;
>  	default:
> -		BUG();
> +		pr_warn("unexpected inode %lu at %s()\n", inode->i_ino, __func__);
>  		return false;
>  	}
>  
> @@ -305,7 +305,7 @@ static int hfs_test_inode(struct inode *inode, void *data)
>  	case HFS_CDR_FIL:
>  		return inode->i_ino == be32_to_cpu(rec->file.FlNum);
>  	default:
> -		BUG();
> +		pr_warn("unexpected type %u at %s()\n", rec->type, __func__);
>  		return 1;
>  	}
>  }
> @@ -441,7 +441,7 @@ int hfs_write_inode(struct inode *inode, struct writeback_control *wbc)
>  			hfs_btree_write(HFS_SB(inode->i_sb)->cat_tree);
>  			return 0;
>  		default:
> -			BUG();
> +			pr_warn("unexpected inode %lu at %s()\n", inode->i_ino, __func__);
>  			return -EIO;
>  		}
>  	}
> -- 
> 2.47.0
> 
> 

Re: [PATCH (REPOST)] hfs: don't use BUG() when we can continue

[Tetsuo Handa]

On 2024/12/05 22:59, Matthew Wilcox wrote:
> On Thu, Dec 05, 2024 at 10:45:11PM +0900, Tetsuo Handa wrote:
>> syzkaller can mount crafted filesystem images.
>> Don't crash the kernel when we can continue.
> 
> The one in hfs_test_inode() makes sense, but we should never get to
> hfs_release_folio() or hfs_write_inode() with a bad inode, even with a
> corrupted fs.  Right?

Unfortunately, https://syzkaller.appspot.com/bug?extid=97e301b4b82ae803d21b
reports that we can reach hfs_write_inode() with a corrupted filesystem.
If we should never get to hfs_write_inode(), how do we want to handle
this problem? Is someone familiar with hfs enough to perform an in-kernel
fsck upon mounting?

Re: [PATCH (REPOST)] hfs: don't use BUG() when we can continue

[Tetsuo Handa]

Matthew, any comments?

On 2024/12/05 23:14, Tetsuo Handa wrote:
> On 2024/12/05 22:59, Matthew Wilcox wrote:
>> On Thu, Dec 05, 2024 at 10:45:11PM +0900, Tetsuo Handa wrote:
>>> syzkaller can mount crafted filesystem images.
>>> Don't crash the kernel when we can continue.
>>
>> The one in hfs_test_inode() makes sense, but we should never get to
>> hfs_release_folio() or hfs_write_inode() with a bad inode, even with a
>> corrupted fs.  Right?
> 
> Unfortunately, https://syzkaller.appspot.com/bug?extid=97e301b4b82ae803d21b
> reports that we can reach hfs_write_inode() with a corrupted filesystem.
> If we should never get to hfs_write_inode(), how do we want to handle
> this problem? Is someone familiar with hfs enough to perform an in-kernel
> fsck upon mounting?
> 

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

[Tetsuo Handa]

Since syzkaller can mount crafted filesystem images with inode->ino == 0
(which is not listed as "Some special File ID numbers" in fs/hfs/hfs.h ),
replace BUG() with pr_warn().

Reported-by: syzbot+97e301b4b82ae803d21b@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=97e301b4b82ae803d21b
Tested-by: syzbot+97e301b4b82ae803d21b@syzkaller.appspotmail.com
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 fs/hfs/inode.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/hfs/inode.c b/fs/hfs/inode.c
index a81ce7a740b9..794d710c3ae0 100644
--- a/fs/hfs/inode.c
+++ b/fs/hfs/inode.c
@@ -81,7 +81,7 @@ static bool hfs_release_folio(struct folio *folio, gfp_t mask)
 		tree = HFS_SB(sb)->cat_tree;
 		break;
 	default:
-		BUG();
+		pr_warn("unexpected inode %lu at %s()\n", inode->i_ino, __func__);
 		return false;
 	}
 
@@ -305,7 +305,7 @@ static int hfs_test_inode(struct inode *inode, void *data)
 	case HFS_CDR_FIL:
 		return inode->i_ino == be32_to_cpu(rec->file.FlNum);
 	default:
-		BUG();
+		pr_warn("unexpected type %u at %s()\n", rec->type, __func__);
 		return 1;
 	}
 }
@@ -441,7 +441,7 @@ int hfs_write_inode(struct inode *inode, struct writeback_control *wbc)
 			hfs_btree_write(HFS_SB(inode->i_sb)->cat_tree);
 			return 0;
 		default:
-			BUG();
+			pr_warn("unexpected inode %lu at %s()\n", inode->i_ino, __func__);
 			return -EIO;
 		}
 	}
-- 
2.50.1

Re: [PATCH v2] hfs: remove BUG() from hfs_release_folio()/hfs_test_inode()/hfs_write_inode()

[Viacheslav Dubeyko]

On Tue, 2025-07-15 at 15:51 +0900, Tetsuo Handa wrote:
> Since syzkaller can mount crafted filesystem images with inode->ino
> == 0
> (which is not listed as "Some special File ID numbers" in
> fs/hfs/hfs.h ),
> replace BUG() with pr_warn().
> 
> Reported-by: syzbot+97e301b4b82ae803d21b@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=97e301b4b82ae803d21b
> Tested-by: syzbot+97e301b4b82ae803d21b@syzkaller.appspotmail.com
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> ---
>  fs/hfs/inode.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/hfs/inode.c b/fs/hfs/inode.c
> index a81ce7a740b9..794d710c3ae0 100644
> --- a/fs/hfs/inode.c
> +++ b/fs/hfs/inode.c
> @@ -81,7 +81,7 @@ static bool hfs_release_folio(struct folio *folio,
> gfp_t mask)
>  		tree = HFS_SB(sb)->cat_tree;
>  		break;
>  	default:
> -		BUG();
> +		pr_warn("unexpected inode %lu at %s()\n", inode-
> >i_ino, __func__);

I don't think that it makes sense to add the function name here. I
understand that you would like to be informative here. But, usually,
HFS code doesn't show the the function name in error messages.

By the way, why are you using pr_warn() but not pr_err()? Any
particular reason to use namely pr_warn()?

We had BUG() here before and, potentially, we could use pr_warn() +
dump_stack() to be really informative here.

>  		return false;
>  	}
>  
> @@ -305,7 +305,7 @@ static int hfs_test_inode(struct inode *inode,
> void *data)
>  	case HFS_CDR_FIL:
>  		return inode->i_ino == be32_to_cpu(rec->file.FlNum);
>  	default:
> -		BUG();
> +		pr_warn("unexpected type %u at %s()\n", rec->type,
> __func__);

Ditto.

>  		return 1;
>  	}
>  }
> @@ -441,7 +441,7 @@ int hfs_write_inode(struct inode *inode, struct
> writeback_control *wbc)
>  			hfs_btree_write(HFS_SB(inode->i_sb)-
> >cat_tree);
>  			return 0;
>  		default:
> -			BUG();
> +			pr_warn("unexpected inode %lu at %s()\n",
> inode->i_ino, __func__);

Ditto.

Thanks,
Slava.

>  			return -EIO;
>  		}
>  	}

Re: [PATCH v2] hfs: remove BUG() from hfs_release_folio()/hfs_test_inode()/hfs_write_inode()

[Tetsuo Handa]

On 2025/07/16 4:20, Viacheslav Dubeyko wrote:
> I don't think that it makes sense to add the function name here. I
> understand that you would like to be informative here. But, usually,
> HFS code doesn't show the the function name in error messages.
> 
> By the way, why are you using pr_warn() but not pr_err()? Any
> particular reason to use namely pr_warn()?

Simply mimicked

  pr_warn("filesystem was not cleanly unmounted, running fsck.hfs is recommended.  mounting read-only.\n");
  pr_warn("filesystem was not cleanly unmounted, running fsck.hfs is recommended.  leaving read-only.\n");

messages. But stronger level (i.e. pr_err()) is OK for locations
which should not occur.

> We had BUG() here before and, potentially, we could use pr_warn() +
> dump_stack() to be really informative here.

Since printing a lot of messages causes stalls, I'd like to keep minimum.

Although fsck.hfs cannot fix all problems in the filesystem image used by the
reproducer ( https://syzkaller.appspot.com/text?tag=ReproC&x=111450f0580000 ),
updating this patch to suggest running fsck.hfs might be helpful.

  ** /dev/loop0
     Executing fsck_hfs (version 540.1-Linux).
  ** Checking HFS volume.
     Invalid extent entry
  (3, 0)
  ** The volume   could not be verified completely.

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

[Tetsuo Handa]

Since syzkaller can mount crafted filesystem images with inode->i_ino == 0
(which is not listed as "Some special File ID numbers" in fs/hfs/hfs.h ),
replace BUG() with pr_err().

Reported-by: syzbot+97e301b4b82ae803d21b@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=97e301b4b82ae803d21b
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 fs/hfs/inode.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/fs/hfs/inode.c b/fs/hfs/inode.c
index a81ce7a740b9..9bbf2883bb8f 100644
--- a/fs/hfs/inode.c
+++ b/fs/hfs/inode.c
@@ -81,7 +81,8 @@ static bool hfs_release_folio(struct folio *folio, gfp_t mask)
 		tree = HFS_SB(sb)->cat_tree;
 		break;
 	default:
-		BUG();
+		pr_err("detected unknown inode %lu, running fsck.hfs is recommended.\n",
+		       inode->i_ino);
 		return false;
 	}
 
@@ -305,7 +306,7 @@ static int hfs_test_inode(struct inode *inode, void *data)
 	case HFS_CDR_FIL:
 		return inode->i_ino == be32_to_cpu(rec->file.FlNum);
 	default:
-		BUG();
+		pr_err("detected unknown type %u, running fsck.hfs is recommended.\n", rec->type);
 		return 1;
 	}
 }
@@ -441,7 +442,8 @@ int hfs_write_inode(struct inode *inode, struct writeback_control *wbc)
 			hfs_btree_write(HFS_SB(inode->i_sb)->cat_tree);
 			return 0;
 		default:
-			BUG();
+			pr_err("detected unknown inode %lu, running fsck.hfs is recommended.\n",
+			       inode->i_ino);
 			return -EIO;
 		}
 	}
-- 
2.50.1

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

[Viacheslav Dubeyko]

On Fri, 2025-07-18 at 00:32 +0900, Tetsuo Handa wrote:
> Since syzkaller can mount crafted filesystem images with inode->i_ino == 0
> (which is not listed as "Some special File ID numbers" in fs/hfs/hfs.h ),
> replace BUG() with pr_err().
> 
> Reported-by: syzbot+97e301b4b82ae803d21b@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=97e301b4b82ae803d21b  
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> ---
>  fs/hfs/inode.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/hfs/inode.c b/fs/hfs/inode.c
> index a81ce7a740b9..9bbf2883bb8f 100644
> --- a/fs/hfs/inode.c
> +++ b/fs/hfs/inode.c
> @@ -81,7 +81,8 @@ static bool hfs_release_folio(struct folio *folio, gfp_t mask)
>  		tree = HFS_SB(sb)->cat_tree;
>  		break;
>  	default:
> -		BUG();
> +		pr_err("detected unknown inode %lu, running fsck.hfs is recommended.\n",
> +		       inode->i_ino);
>  		return false;
>  	}
>  
> @@ -305,7 +306,7 @@ static int hfs_test_inode(struct inode *inode, void *data)
>  	case HFS_CDR_FIL:
>  		return inode->i_ino == be32_to_cpu(rec->file.FlNum);
>  	default:
> -		BUG();
> +		pr_err("detected unknown type %u, running fsck.hfs is recommended.\n", rec->type);
>  		return 1;
>  	}
>  }
> @@ -441,7 +442,8 @@ int hfs_write_inode(struct inode *inode, struct writeback_control *wbc)
>  			hfs_btree_write(HFS_SB(inode->i_sb)->cat_tree);
>  			return 0;
>  		default:
> -			BUG();
> +			pr_err("detected unknown inode %lu, running fsck.hfs is recommended.\n",
> +			       inode->i_ino);
>  			return -EIO;
>  		}
>  	}

Looks good!

Reviewed-by: Viacheslav Dubeyko <slava@dubeyko.com>

Thanks,
Slava.

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

[Matthew Wilcox]

On Fri, Jul 18, 2025 at 12:32:46AM +0900, Tetsuo Handa wrote:
> +++ b/fs/hfs/inode.c
> @@ -81,7 +81,8 @@ static bool hfs_release_folio(struct folio *folio, gfp_t mask)
>  		tree = HFS_SB(sb)->cat_tree;
>  		break;
>  	default:
> -		BUG();
> +		pr_err("detected unknown inode %lu, running fsck.hfs is recommended.\n",
> +		       inode->i_ino);

As I asked the first time, how can we get here?  In order to release a
folio, we have to first populate the pagecache of the inode with folios.
How did we manage to do that for an inode with a bogus i_ino?

> @@ -441,7 +442,8 @@ int hfs_write_inode(struct inode *inode, struct writeback_control *wbc)
>  			hfs_btree_write(HFS_SB(inode->i_sb)->cat_tree);
>  			return 0;
>  		default:
> -			BUG();
> +			pr_err("detected unknown inode %lu, running fsck.hfs is recommended.\n",
> +			       inode->i_ino);

Similarly here, how did we manage to mark a bad inode as dirty?

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

[Viacheslav Dubeyko]

On Thu, 2025-07-17 at 20:35 +0100, Matthew Wilcox wrote:
> On Fri, Jul 18, 2025 at 12:32:46AM +0900, Tetsuo Handa wrote:
> > +++ b/fs/hfs/inode.c
> > @@ -81,7 +81,8 @@ static bool hfs_release_folio(struct folio *folio, gfp_t mask)
> >  		tree = HFS_SB(sb)->cat_tree;
> >  		break;
> >  	default:
> > -		BUG();
> > +		pr_err("detected unknown inode %lu, running fsck.hfs is recommended.\n",
> > +		       inode->i_ino);
> 
> As I asked the first time, how can we get here?  In order to release a
> folio, we have to first populate the pagecache of the inode with folios.
> How did we manage to do that for an inode with a bogus i_ino?
> 

Probably, we have not checked the inode ID at first place in hfs_read_inode()
[1]. So, it makes sense to rework the logic here.

> > @@ -441,7 +442,8 @@ int hfs_write_inode(struct inode *inode, struct writeback_control *wbc)
> >  			hfs_btree_write(HFS_SB(inode->i_sb)->cat_tree);
> >  			return 0;
> >  		default:
> > -			BUG();
> > +			pr_err("detected unknown inode %lu, running fsck.hfs is recommended.\n",
> > +			       inode->i_ino);
> 
> Similarly here, how did we manage to mark a bad inode as dirty?

I assume if we created the inode as normal with i_ino == 0, then we can make it
as a dirty. Because, inode will be made as bad inode here [2] only if rec->type
is invalid. But if it is valid, then we can create the normal inode even with
i_ino == 0.

It is my current understanding of the situation here. Tetsuo, please, correct me
if I am wrong.

Thanks,
Slava.

[1] https://elixir.bootlin.com/linux/v6.16-rc6/source/fs/hfs/inode.c#L350
[2] https://elixir.bootlin.com/linux/v6.16-rc6/source/fs/hfs/inode.c#L373

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

[Tetsuo Handa]

On 2025/07/18 4:49, Viacheslav Dubeyko wrote:
> I assume if we created the inode as normal with i_ino == 0, then we can make it
> as a dirty. Because, inode will be made as bad inode here [2] only if rec->type
> is invalid. But if it is valid, then we can create the normal inode even with
> i_ino == 0.

You are right. The crafted filesystem image in the reproducer is hitting HFS_CDR_DIR with
inode->i_ino = 0 ( https://elixir.bootlin.com/linux/v6.16-rc6/source/fs/hfs/inode.c#L363 ).

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

[Viacheslav Dubeyko]

On Fri, 2025-07-18 at 07:08 +0900, Tetsuo Handa wrote:
> On 2025/07/18 4:49, Viacheslav Dubeyko wrote:
> > I assume if we created the inode as normal with i_ino == 0, then we can make it
> > as a dirty. Because, inode will be made as bad inode here [2] only if rec->type
> > is invalid. But if it is valid, then we can create the normal inode even with
> > i_ino == 0.
> 
> You are right. The crafted filesystem image in the reproducer is hitting HFS_CDR_DIR with
> inode->i_ino = 0 ( https://elixir.bootlin.com/linux/v6.16-rc6/source/fs/hfs/inode.c#L363   ).

So, any plans to rework the patch?

Thanks,
Slava.

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

[Tetsuo Handa]

On 2025/07/22 2:04, Viacheslav Dubeyko wrote:
> On Fri, 2025-07-18 at 07:08 +0900, Tetsuo Handa wrote:
>> On 2025/07/18 4:49, Viacheslav Dubeyko wrote:
>>> I assume if we created the inode as normal with i_ino == 0, then we can make it
>>> as a dirty. Because, inode will be made as bad inode here [2] only if rec->type
>>> is invalid. But if it is valid, then we can create the normal inode even with
>>> i_ino == 0.
>>
>> You are right. The crafted filesystem image in the reproducer is hitting HFS_CDR_DIR with
>> inode->i_ino = 0 ( https://elixir.bootlin.com/linux/v6.16-rc6/source/fs/hfs/inode.c#L363   ).
> 
> So, any plans to rework the patch?
> 

What do you mean by "rework"?

I can update patch description if you have one, but I don't plan to try something like below.

@@ -393,20 +393,30 @@ struct inode *hfs_iget(struct super_block *sb, struct hfs_cat_key *key, hfs_cat_
        switch (rec->type) {
        case HFS_CDR_DIR:
                cnid = be32_to_cpu(rec->dir.DirID);
                break;
        case HFS_CDR_FIL:
                cnid = be32_to_cpu(rec->file.FlNum);
                break;
        default:
                return NULL;
        }
+       if (cnid < HFS_FIRSTUSER_CNID) {
+               switch (cnid) {
+               case HFS_ROOT_CNID:
+               case HFS_EXT_CNID:
+               case HFS_CAT_CNID:
+                       break;
+               default:
+                       return NULL;
+               }
+       }
        inode = iget5_locked(sb, cnid, hfs_test_inode, hfs_read_inode, &data);
        if (inode && (inode->i_state & I_NEW))
                unlock_new_inode(inode);
        return inode;
 }

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

[Matthew Wilcox]

On Tue, Jul 22, 2025 at 07:42:35PM +0900, Tetsuo Handa wrote:
> I can update patch description if you have one, but I don't plan to try something like below.

Why not?  Papering over the underlying problem is what I rejected in v1,
and here we are months later with you trying a v4.

> @@ -393,20 +393,30 @@ struct inode *hfs_iget(struct super_block *sb, struct hfs_cat_key *key, hfs_cat_
>         switch (rec->type) {
>         case HFS_CDR_DIR:
>                 cnid = be32_to_cpu(rec->dir.DirID);
>                 break;
>         case HFS_CDR_FIL:
>                 cnid = be32_to_cpu(rec->file.FlNum);
>                 break;
>         default:
>                 return NULL;
>         }
> +       if (cnid < HFS_FIRSTUSER_CNID) {
> +               switch (cnid) {
> +               case HFS_ROOT_CNID:
> +               case HFS_EXT_CNID:
> +               case HFS_CAT_CNID:
> +                       break;
> +               default:
> +                       return NULL;
> +               }
> +       }
>         inode = iget5_locked(sb, cnid, hfs_test_inode, hfs_read_inode, &data);
>         if (inode && (inode->i_state & I_NEW))
>                 unlock_new_inode(inode);
>         return inode;
>  }
> 

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

[Tetsuo Handa]

On 2025/07/22 22:30, Matthew Wilcox wrote:
> On Tue, Jul 22, 2025 at 07:42:35PM +0900, Tetsuo Handa wrote:
>> I can update patch description if you have one, but I don't plan to try something like below.
> 
> Why not?  Papering over the underlying problem is what I rejected in v1,
> and here we are months later with you trying a v4.

Because I don't know how HFS/HFS+ filesystems work.
I just want to close these nearly 1000 days old bugs.

You can write your patches.

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

[Matthew Wilcox]

On Tue, Jul 22, 2025 at 11:04:30PM +0900, Tetsuo Handa wrote:
> On 2025/07/22 22:30, Matthew Wilcox wrote:
> > On Tue, Jul 22, 2025 at 07:42:35PM +0900, Tetsuo Handa wrote:
> >> I can update patch description if you have one, but I don't plan to try something like below.
> > 
> > Why not?  Papering over the underlying problem is what I rejected in v1,
> > and here we are months later with you trying a v4.
> 
> Because I don't know how HFS/HFS+ filesystems work.
> I just want to close these nearly 1000 days old bugs.
> 
> You can write your patches.

I don't understand this attitude at all.  Are you in QA and being paid
by "number of bugs closed per week"?

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

[Viacheslav Dubeyko]

On Tue, 2025-07-22 at 15:22 +0100, Matthew Wilcox wrote:
> On Tue, Jul 22, 2025 at 11:04:30PM +0900, Tetsuo Handa wrote:
> > On 2025/07/22 22:30, Matthew Wilcox wrote:
> > > On Tue, Jul 22, 2025 at 07:42:35PM +0900, Tetsuo Handa wrote:
> > > > I can update patch description if you have one, but I don't plan to try something like below.
> > > 
> > > Why not?  Papering over the underlying problem is what I rejected in v1,
> > > and here we are months later with you trying a v4.
> > 
> > Because I don't know how HFS/HFS+ filesystems work.

The patch definitely should be rework. The phrase "I don't know how it works"
cannot be accepted as excuse. :)

> > I just want to close these nearly 1000 days old bugs.
> > 

We are not in a hurry. We must fix the reason of the bug but
not try to hide the real reason of the issue. 

> > You can write your patches.
> 
> I don't understand this attitude at all.  Are you in QA and being paid
> by "number of bugs closed per week"?

OK. Let's return to hfs_read_inode() again [1]. We have such logic here:

switch (rec->type) {
	case HFS_CDR_FIL:
<skipped>
		inode->i_ino = be32_to_cpu(rec->file.FlNum);
<skipped>
		break;
	case HFS_CDR_DIR:
		inode->i_ino = be32_to_cpu(rec->dir.DirID);
<skipped>
		break;
	default:
		make_bad_inode(inode);
}

So, if rec->type is OK (HFS_CDR_FIL, HFS_CDR_DIR) then we process
a particular type of record, otherwise, we create the bad inode. So, we simply
need to extend this logic. If rec->file.FlNum or rec->dir.DirID is equal or
bigger than HFS_FIRSTUSER_CNID, then we can create normal inode. Otherwise,
we need to create the bad inode. We simply need to add the checking logic
here. Tetsuo, does it make sense to you? :) Because, if we have corrupted value
of rec->file.FlNum or rec->dir.DirID, then it doesn't make sense to create
the normal inode with invalid i_ino. Simply, take a look here [2]:

/* Some special File ID numbers */
#define HFS_POR_CNID		1	/* Parent Of the Root */
#define HFS_ROOT_CNID		2	/* ROOT directory */
#define HFS_EXT_CNID		3	/* EXTents B-tree */
#define HFS_CAT_CNID		4	/* CATalog B-tree */
#define HFS_BAD_CNID		5	/* BAD blocks file */
#define HFS_ALLOC_CNID		6	/* ALLOCation file (HFS+) */
#define HFS_START_CNID		7	/* STARTup file (HFS+) */
#define HFS_ATTR_CNID		8	/* ATTRibutes file (HFS+) */
#define HFS_EXCH_CNID		15	/* ExchangeFiles temp id */
#define HFS_FIRSTUSER_CNID	16

Zero inode ID is completely invalid. And values from 1 - 15 are reserved
for HFS metadata structures.

Thanks,
Slava.

[1] https://elixir.bootlin.com/linux/v6.16-rc6/source/fs/hfs/inode.c#L350
[2] https://elixir.bootlin.com/linux/v6.16-rc6/source/fs/hfs/hfs.h#L40

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

[Tetsuo Handa]

On 2025/07/23 3:08, Viacheslav Dubeyko wrote:
> So, if rec->type is OK (HFS_CDR_FIL, HFS_CDR_DIR) then we process
> a particular type of record, otherwise, we create the bad inode. So, we simply
> need to extend this logic. If rec->file.FlNum or rec->dir.DirID is equal or
> bigger than HFS_FIRSTUSER_CNID, then we can create normal inode. Otherwise,
> we need to create the bad inode. We simply need to add the checking logic
> here. Tetsuo, does it make sense to you? :) Because, if we have corrupted value
> of rec->file.FlNum or rec->dir.DirID, then it doesn't make sense to create
> the normal inode with invalid i_ino. Simply, take a look here [2]:

Something is wrong with below change; legitimate HFS filesystem images can no longer be mounted.
I guess that several reserved IDs have to be excluded from make_bad_inode() conditions.

# hformat testfile.img
# mount -t hfs -o loop testfile.img /mnt/
mount: /mnt: filesystem was mounted, but any subsequent operation failed: Operation not permitted.

--- a/fs/hfs/inode.c
+++ b/fs/hfs/inode.c
@@ -358,6 +358,8 @@ static int hfs_read_inode(struct inode *inode, void *data)
                inode->i_op = &hfs_file_inode_operations;
                inode->i_fop = &hfs_file_operations;
                inode->i_mapping->a_ops = &hfs_aops;
+               if (unlikely(inode->i_ino < HFS_FIRSTUSER_CNID))
+                       make_bad_inode(inode);
                break;
        case HFS_CDR_DIR:
                inode->i_ino = be32_to_cpu(rec->dir.DirID);
@@ -368,6 +370,8 @@ static int hfs_read_inode(struct inode *inode, void *data)
                                      inode_set_atime_to_ts(inode, inode_set_ctime_to_ts(inode, hfs_m_to_utime(rec->dir.MdDat))));
                inode->i_op = &hfs_dir_inode_operations;
                inode->i_fop = &hfs_dir_operations;
+               if (unlikely(inode->i_ino < HFS_FIRSTUSER_CNID))
+                       make_bad_inode(inode);
                break;
        default:
                make_bad_inode(inode);

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

[Tetsuo Handa]

With below change, legitimate HFS filesystem images can be mounted.

But is crafted HFS filesystem images can not be mounted expected result?

  # losetup -a
  /dev/loop0: [0001]:7185 (/memfd:syzkaller (deleted))
  # mount -t hfs /dev/loop0 /mnt/
  mount: /mnt: filesystem was mounted, but any subsequent operation failed: Operation not permitted.
  # fsck.hfs /dev/loop0
  ** /dev/loop0
     Executing fsck_hfs (version 540.1-Linux).
  ** Checking HFS volume.
     Invalid extent entry
  (3, 0)
  ** The volume   could not be verified completely.
  # mount -t hfs /dev/loop0 /mnt/
  mount: /mnt: filesystem was mounted, but any subsequent operation failed: Operation not permitted.

Also, are IDs which should be excluded from make_bad_inode() conditions
same for HFS_CDR_FIL and HFS_CDR_DIR ?


--- a/fs/hfs/inode.c
+++ b/fs/hfs/inode.c
@@ -358,6 +358,8 @@ static int hfs_read_inode(struct inode *inode, void *data)
                inode->i_op = &hfs_file_inode_operations;
                inode->i_fop = &hfs_file_operations;
                inode->i_mapping->a_ops = &hfs_aops;
+               if (inode->i_ino < HFS_FIRSTUSER_CNID)
+                       goto check_reserved_ino;
                break;
        case HFS_CDR_DIR:
                inode->i_ino = be32_to_cpu(rec->dir.DirID);
@@ -368,6 +370,24 @@ static int hfs_read_inode(struct inode *inode, void *data)
                                      inode_set_atime_to_ts(inode, inode_set_ctime_to_ts(inode, hfs_m_to_utime(rec->dir.MdDat))));
                inode->i_op = &hfs_dir_inode_operations;
                inode->i_fop = &hfs_dir_operations;
+               if (inode->i_ino < HFS_FIRSTUSER_CNID)
+                       goto check_reserved_ino;
+               break;
+       default:
+               make_bad_inode(inode);
+       }
+       return 0;
+check_reserved_ino:
+       switch (inode->i_ino) {
+       case HFS_POR_CNID:
+       case HFS_ROOT_CNID:
+       case HFS_EXT_CNID:
+       case HFS_CAT_CNID:
+       case HFS_BAD_CNID:
+       case HFS_ALLOC_CNID:
+       case HFS_START_CNID:
+       case HFS_ATTR_CNID:
+       case HFS_EXCH_CNID:
                break;
        default:
                make_bad_inode(inode);

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

[Viacheslav Dubeyko]

On Wed, 2025-07-23 at 11:16 +0900, Tetsuo Handa wrote:
> With below change, legitimate HFS filesystem images can be mounted.
> 
> But is crafted HFS filesystem images can not be mounted expected result?
> 
>   # losetup -a
>   /dev/loop0: [0001]:7185 (/memfd:syzkaller (deleted))
>   # mount -t hfs /dev/loop0 /mnt/
>   mount: /mnt: filesystem was mounted, but any subsequent operation failed: Operation not permitted.
>   # fsck.hfs /dev/loop0
>   ** /dev/loop0
>      Executing fsck_hfs (version 540.1-Linux).
>   ** Checking HFS volume.
>      Invalid extent entry
>   (3, 0)
>   ** The volume   could not be verified completely.
>   # mount -t hfs /dev/loop0 /mnt/
>   mount: /mnt: filesystem was mounted, but any subsequent operation failed: Operation not permitted.
> 
> Also, are IDs which should be excluded from make_bad_inode() conditions
> same for HFS_CDR_FIL and HFS_CDR_DIR ?
> 
> 
> --- a/fs/hfs/inode.c
> +++ b/fs/hfs/inode.c
> @@ -358,6 +358,8 @@ static int hfs_read_inode(struct inode *inode, void *data)
>                 inode->i_op = &hfs_file_inode_operations;
>                 inode->i_fop = &hfs_file_operations;
>                 inode->i_mapping->a_ops = &hfs_aops;
> +               if (inode->i_ino < HFS_FIRSTUSER_CNID)
> +                       goto check_reserved_ino;
>                 break;
>         case HFS_CDR_DIR:
>                 inode->i_ino = be32_to_cpu(rec->dir.DirID);
> @@ -368,6 +370,24 @@ static int hfs_read_inode(struct inode *inode, void *data)
>                                       inode_set_atime_to_ts(inode, inode_set_ctime_to_ts(inode, hfs_m_to_utime(rec->dir.MdDat))));
>                 inode->i_op = &hfs_dir_inode_operations;
>                 inode->i_fop = &hfs_dir_operations;
> +               if (inode->i_ino < HFS_FIRSTUSER_CNID)
> +                       goto check_reserved_ino;
> +               break;
> +       default:
> +               make_bad_inode(inode);
> +       }
> +       return 0;
> +check_reserved_ino:
> +       switch (inode->i_ino) {
> +       case HFS_POR_CNID:
> +       case HFS_ROOT_CNID:
> +       case HFS_EXT_CNID:
> +       case HFS_CAT_CNID:
> +       case HFS_BAD_CNID:
> +       case HFS_ALLOC_CNID:
> +       case HFS_START_CNID:
> +       case HFS_ATTR_CNID:
> +       case HFS_EXCH_CNID:
>                 break;
>         default:
>                 make_bad_inode(inode);

I have missed that this list contains [1]:

#define HFS_POR_CNID		1	/* Parent Of the Root */
#define HFS_ROOT_CNID		2	/* ROOT directory */

Of course, hfs_read_inode() can be called for the root directory and parent of
the root cases. So, HFS_POR_CNID and HFS_ROOT_CNID are legitimate values.
However, the other constants cannot be used because they should be described in
superblock (MDB) and Catalog File cannot have the records for them.

Thanks,
Slava.

[1] https://elixir.bootlin.com/linux/v6.16-rc7/source/fs/hfs/hfs.h#L41

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

[Viacheslav Dubeyko]

On Wed, 2025-07-23 at 18:19 +0000, Viacheslav Dubeyko wrote:
> On Wed, 2025-07-23 at 11:16 +0900, Tetsuo Handa wrote:
> > With below change, legitimate HFS filesystem images can be mounted.
> > 
> > But is crafted HFS filesystem images can not be mounted expected result?
> > 
> >   # losetup -a
> >   /dev/loop0: [0001]:7185 (/memfd:syzkaller (deleted))
> >   # mount -t hfs /dev/loop0 /mnt/
> >   mount: /mnt: filesystem was mounted, but any subsequent operation failed: Operation not permitted.
> >   # fsck.hfs /dev/loop0
> >   ** /dev/loop0
> >      Executing fsck_hfs (version 540.1-Linux).
> >   ** Checking HFS volume.
> >      Invalid extent entry
> >   (3, 0)
> >   ** The volume   could not be verified completely.
> >   # mount -t hfs /dev/loop0 /mnt/
> >   mount: /mnt: filesystem was mounted, but any subsequent operation failed: Operation not permitted.
> > 
> > Also, are IDs which should be excluded from make_bad_inode() conditions
> > same for HFS_CDR_FIL and HFS_CDR_DIR ?
> > 
> > 
> > --- a/fs/hfs/inode.c
> > +++ b/fs/hfs/inode.c
> > @@ -358,6 +358,8 @@ static int hfs_read_inode(struct inode *inode, void *data)
> >                 inode->i_op = &hfs_file_inode_operations;
> >                 inode->i_fop = &hfs_file_operations;
> >                 inode->i_mapping->a_ops = &hfs_aops;
> > +               if (inode->i_ino < HFS_FIRSTUSER_CNID)
> > +                       goto check_reserved_ino;
> >                 break;
> >         case HFS_CDR_DIR:
> >                 inode->i_ino = be32_to_cpu(rec->dir.DirID);
> > @@ -368,6 +370,24 @@ static int hfs_read_inode(struct inode *inode, void *data)
> >                                       inode_set_atime_to_ts(inode, inode_set_ctime_to_ts(inode, hfs_m_to_utime(rec->dir.MdDat))));
> >                 inode->i_op = &hfs_dir_inode_operations;
> >                 inode->i_fop = &hfs_dir_operations;
> > +               if (inode->i_ino < HFS_FIRSTUSER_CNID)
> > +                       goto check_reserved_ino;
> > +               break;
> > +       default:
> > +               make_bad_inode(inode);
> > +       }
> > +       return 0;
> > +check_reserved_ino:
> > +       switch (inode->i_ino) {
> > +       case HFS_POR_CNID:
> > +       case HFS_ROOT_CNID:
> > +       case HFS_EXT_CNID:
> > +       case HFS_CAT_CNID:
> > +       case HFS_BAD_CNID:
> > +       case HFS_ALLOC_CNID:
> > +       case HFS_START_CNID:
> > +       case HFS_ATTR_CNID:
> > +       case HFS_EXCH_CNID:
> >                 break;
> >         default:
> >                 make_bad_inode(inode);
> 
> I have missed that this list contains [1]:
> 
> #define HFS_POR_CNID		1	/* Parent Of the Root */
> #define HFS_ROOT_CNID		2	/* ROOT directory */
> 
> Of course, hfs_read_inode() can be called for the root directory and parent of
> the root cases. So, HFS_POR_CNID and HFS_ROOT_CNID are legitimate values.
> However, the other constants cannot be used because they should be described in
> superblock (MDB) and Catalog File cannot have the records for them.
> 

I have checked the HFS specification. So, additional corrections:

#define HFS_POR_CNID		1	/* Parent Of the Root */
#define HFS_ROOT_CNID		2	/* ROOT directory */

These values are legitimate values.

#define HFS_EXT_CNID		3	/* EXTents B-tree */
#define HFS_CAT_CNID		4	/* CATalog B-tree */

This metadata structures are defined in MDB.

#define HFS_BAD_CNID		5	/* BAD blocks file */

This could be defined in Catalog File because MDB has nothing for this metadata
structure. However, it's ancient technology.

#define HFS_ALLOC_CNID		6	/* ALLOCation file (HFS+) */
#define HFS_START_CNID		7	/* STARTup file (HFS+) */
#define HFS_ATTR_CNID		8	/* ATTRibutes file (HFS+) */

These value are invalid for HFS.

#define HFS_EXCH_CNID		15	/* ExchangeFiles temp id */

This could be defined in Catalog File (maybe not). I didn't find anything
related to this in HFS specification.

> Thanks,
> Slava.
> 
> [1] https://elixir.bootlin.com/linux/v6.16-rc7/source/fs/hfs/hfs.h#L41  

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

[Tetsuo Handa]

Then, something like below change?

--- a/fs/hfs/inode.c
+++ b/fs/hfs/inode.c
@@ -318,6 +318,9 @@ static int hfs_read_inode(struct inode *inode, void *data)
        struct hfs_iget_data *idata = data;
        struct hfs_sb_info *hsb = HFS_SB(inode->i_sb);
        hfs_cat_rec *rec;
+       /* https://developer.apple.com/library/archive/technotes/tn/tn1150.html#CNID */
+       static const u16 bad_cnid_list = (1 << 0) | (1 << 6) | (1 << 7) | (1 << 8) |
+               (1 << 9) | (1 << 10) | (1 << 11) | (1 << 12) | (1 << 13);

        HFS_I(inode)->flags = 0;
        HFS_I(inode)->rsrc_inode = NULL;
@@ -358,6 +361,8 @@ static int hfs_read_inode(struct inode *inode, void *data)
                inode->i_op = &hfs_file_inode_operations;
                inode->i_fop = &hfs_file_operations;
                inode->i_mapping->a_ops = &hfs_aops;
+               if (inode->i_ino < HFS_FIRSTUSER_CNID && ((1U << inode->i_ino) & bad_cnid_list))
+                       make_bad_inode(inode);
                break;
        case HFS_CDR_DIR:
                inode->i_ino = be32_to_cpu(rec->dir.DirID);
@@ -368,6 +373,8 @@ static int hfs_read_inode(struct inode *inode, void *data)
                                      inode_set_atime_to_ts(inode, inode_set_ctime_to_ts(inode, hfs_m_to_utime(rec->dir.MdDat))));
                inode->i_op = &hfs_dir_inode_operations;
                inode->i_fop = &hfs_dir_operations;
+               if (inode->i_ino < HFS_FIRSTUSER_CNID && ((1U << inode->i_ino) & bad_cnid_list))
+                       make_bad_inode(inode);
                break;
        default:
                make_bad_inode(inode);



But I can't be convinced that above change is sufficient, for if I do

+		static u8 serial;
+               if (inode->i_ino < HFS_FIRSTUSER_CNID && ((1U << inode->i_ino) & bad_cnid_list))
+                       inode->i_ino = (serial++) % 16;

instead of

+               if (inode->i_ino < HFS_FIRSTUSER_CNID && ((1U << inode->i_ino) & bad_cnid_list))
+                       make_bad_inode(inode);

, the reproducer still hits BUG() for 0, 1, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14 and 15
because hfs_write_inode() handles only 2, 3 and 4.

        if (inode->i_ino < HFS_FIRSTUSER_CNID) {
                switch (inode->i_ino) {
                case HFS_ROOT_CNID:
                        break;
                case HFS_EXT_CNID:
                        hfs_btree_write(HFS_SB(inode->i_sb)->ext_tree);
                        return 0;
                case HFS_CAT_CNID:
                        hfs_btree_write(HFS_SB(inode->i_sb)->cat_tree);
                        return 0;
                default:
                        BUG();
                        return -EIO;
                }
        }

Unless this is because I'm modifying in-kernel memory than filesystem image,
we will have to remove BUG() line.

On 2025/07/24 3:43, Viacheslav Dubeyko wrote:
> This could be defined in Catalog File (maybe not). I didn't find anything
> related to this in HFS specification.

https://developer.apple.com/library/archive/technotes/tn/tn1150.html#CNID
says "the CNID of zero is never used and serves as a nil value." That is,
I think we can reject inode->i_ino == 0 case.

But I'm not sure for other values up to 15, expect values noted as "introduced
with HFS Plus". We could filter values in bad_cnid_list bitmap, but filtering
undefined values might not be sufficient for preserving BUG() line.

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

[Viacheslav Dubeyko]

On Thu, 2025-07-24 at 15:55 +0900, Tetsuo Handa wrote:
> Then, something like below change?
> 
> --- a/fs/hfs/inode.c
> +++ b/fs/hfs/inode.c
> @@ -318,6 +318,9 @@ static int hfs_read_inode(struct inode *inode, void *data)
>         struct hfs_iget_data *idata = data;
>         struct hfs_sb_info *hsb = HFS_SB(inode->i_sb);
>         hfs_cat_rec *rec;
> +       /* https://developer.apple.com/library/archive/technotes/tn/tn1150.html#CNID   */

We already have all declarations in hfs.h:

/* Some special File ID numbers */
#define HFS_POR_CNID		1	/* Parent Of the Root */
#define HFS_ROOT_CNID		2	/* ROOT directory */
#define HFS_EXT_CNID		3	/* EXTents B-tree */
#define HFS_CAT_CNID		4	/* CATalog B-tree */
#define HFS_BAD_CNID		5	/* BAD blocks file */
#define HFS_ALLOC_CNID		6	/* ALLOCation file (HFS+) */
#define HFS_START_CNID		7	/* STARTup file (HFS+) */
#define HFS_ATTR_CNID		8	/* ATTRibutes file (HFS+) */
#define HFS_EXCH_CNID		15	/* ExchangeFiles temp id */
#define HFS_FIRSTUSER_CNID	16

So, adding the link here doesn't make any sense.

> +       static const u16 bad_cnid_list = (1 << 0) | (1 << 6) | (1 << 7) | (1 << 8) |
> +               (1 << 9) | (1 << 10) | (1 << 11) | (1 << 12) | (1 << 13);

I don't see any sense to introduce flags here. First of all, please, don't use
hardcoded values but you should use declared constants from hfs.h (for example,
HFS_EXT_CNID instead of 3). Secondly, you can simply compare the i_ino with
constants, for example:

bool is_inode_id_invalid(u64 ino) {
      switch (inode->i_ino) {
      case HFS_EXT_CNID:
      ...
          return true;

      }

      return false;
}

Thirdly, you can introduce an inline function that can do such check. And it
make sense to introduce constant for the case of zero value.

Why have you missed HFS_EXT_CNID, HFS_CAT_CNID? These values cannot used in
hfs_read_inode().

> 
>         HFS_I(inode)->flags = 0;
>         HFS_I(inode)->rsrc_inode = NULL;
> @@ -358,6 +361,8 @@ static int hfs_read_inode(struct inode *inode, void *data)
>                 inode->i_op = &hfs_file_inode_operations;
>                 inode->i_fop = &hfs_file_operations;
>                 inode->i_mapping->a_ops = &hfs_aops;
> +               if (inode->i_ino < HFS_FIRSTUSER_CNID && ((1U << inode->i_ino) & bad_cnid_list))
> +                       make_bad_inode(inode);

It looks pretty complicated. You can simply use one above-mentioned function
with the check:

if (is_inode_id_invalid(be32_to_cpu(rec->dir.DirID)))
     <goto to make bad inode>

We can simply check the the inode ID in the beginning of the whole action:

<Make the check here>
		inode->i_ino = be32_to_cpu(rec->file.FlNum);
		inode->i_mode = S_IRUGO | S_IXUGO;
		if (!(rec->file.Flags & HFS_FIL_LOCK))
			inode->i_mode |= S_IWUGO;
		inode->i_mode &= ~hsb->s_file_umask;
		inode->i_mode |= S_IFREG;
		inode_set_mtime_to_ts(inode,
				      inode_set_atime_to_ts(inode,
inode_set_ctime_to_ts(inode, hfs_m_to_utime(rec->file.MdDat))));
		inode->i_op = &hfs_file_inode_operations;
		inode->i_fop = &hfs_file_operations;
		inode->i_mapping->a_ops = &hfs_aops;

It doesn't make any sense to construct inode if we will make in bad inode,
finally. Don't waste computational power. :)

>                 break;
>         case HFS_CDR_DIR:
>                 inode->i_ino = be32_to_cpu(rec->dir.DirID);
> @@ -368,6 +373,8 @@ static int hfs_read_inode(struct inode *inode, void *data)
>                                       inode_set_atime_to_ts(inode, inode_set_ctime_to_ts(inode, hfs_m_to_utime(rec->dir.MdDat))));
>                 inode->i_op = &hfs_dir_inode_operations;
>                 inode->i_fop = &hfs_dir_operations;
> +               if (inode->i_ino < HFS_FIRSTUSER_CNID && ((1U << inode->i_ino) & bad_cnid_list))
> +                       make_bad_inode(inode);

We already have make_bad_inode(inode) as default action. So, simply jump there.

>                 break;
>         default:
>                 make_bad_inode(inode);
> 
> 
> 
> But I can't be convinced that above change is sufficient, for if I do
> 
> +		static u8 serial;
> +               if (inode->i_ino < HFS_FIRSTUSER_CNID && ((1U << inode->i_ino) & bad_cnid_list))
> +                       inode->i_ino = (serial++) % 16;

I don't see the point in flags introduction. It makes logic very complicated.

> 
> instead of
> 
> +               if (inode->i_ino < HFS_FIRSTUSER_CNID && ((1U << inode->i_ino) & bad_cnid_list))
> +                       make_bad_inode(inode);
> 
> , the reproducer still hits BUG() for 0, 1, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14 and 15
> because hfs_write_inode() handles only 2, 3 and 4.
> 

How can we go into hfs_write_inode() if we created the bad inode for invalid
inode ID? How is it possible?

Thanks,
Slava.

>         if (inode->i_ino < HFS_FIRSTUSER_CNID) {
>                 switch (inode->i_ino) {
>                 case HFS_ROOT_CNID:
>                         break;
>                 case HFS_EXT_CNID:
>                         hfs_btree_write(HFS_SB(inode->i_sb)->ext_tree);
>                         return 0;
>                 case HFS_CAT_CNID:
>                         hfs_btree_write(HFS_SB(inode->i_sb)->cat_tree);
>                         return 0;
>                 default:
>                         BUG();
>                         return -EIO;
>                 }
>         }
> 
> Unless this is because I'm modifying in-kernel memory than filesystem image,
> we will have to remove BUG() line.
> 
> On 2025/07/24 3:43, Viacheslav Dubeyko wrote:
> > This could be defined in Catalog File (maybe not). I didn't find anything
> > related to this in HFS specification.
> 
> https://developer.apple.com/library/archive/technotes/tn/tn1150.html#CNID  
> says "the CNID of zero is never used and serves as a nil value." That is,
> I think we can reject inode->i_ino == 0 case.
> 
> But I'm not sure for other values up to 15, expect values noted as "introduced
> with HFS Plus". We could filter values in bad_cnid_list bitmap, but filtering
> undefined values might not be sufficient for preserving BUG() line.

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

[Tetsuo Handa]

On 2025/07/25 4:49, Viacheslav Dubeyko wrote:
> On Thu, 2025-07-24 at 15:55 +0900, Tetsuo Handa wrote:
>> Then, something like below change?
>>
>> --- a/fs/hfs/inode.c
>> +++ b/fs/hfs/inode.c
>> @@ -318,6 +318,9 @@ static int hfs_read_inode(struct inode *inode, void *data)
>>         struct hfs_iget_data *idata = data;
>>         struct hfs_sb_info *hsb = HFS_SB(inode->i_sb);
>>         hfs_cat_rec *rec;
>> +       /* https://developer.apple.com/library/archive/technotes/tn/tn1150.html#CNID   */
> 
> We already have all declarations in hfs.h:
> 
> /* Some special File ID numbers */
> #define HFS_POR_CNID		1	/* Parent Of the Root */
> #define HFS_ROOT_CNID		2	/* ROOT directory */
> #define HFS_EXT_CNID		3	/* EXTents B-tree */
> #define HFS_CAT_CNID		4	/* CATalog B-tree */
> #define HFS_BAD_CNID		5	/* BAD blocks file */
> #define HFS_ALLOC_CNID		6	/* ALLOCation file (HFS+) */
> #define HFS_START_CNID		7	/* STARTup file (HFS+) */
> #define HFS_ATTR_CNID		8	/* ATTRibutes file (HFS+) */
> #define HFS_EXCH_CNID		15	/* ExchangeFiles temp id */
> #define HFS_FIRSTUSER_CNID	16

These declarations does not define 14, and some flags are never used despite
being declared here.

> 
> So, adding the link here doesn't make any sense.
> 
>> +       static const u16 bad_cnid_list = (1 << 0) | (1 << 6) | (1 << 7) | (1 << 8) |
>> +               (1 << 9) | (1 << 10) | (1 << 11) | (1 << 12) | (1 << 13);

Some of values in this constant are not declared.

> 
> I don't see any sense to introduce flags here. First of all, please, don't use
> hardcoded values but you should use declared constants from hfs.h (for example,
> HFS_EXT_CNID instead of 3). Secondly, you can simply compare the i_ino with
> constants, for example:

This will save a lot of computational power compared to switch().

> 
> bool is_inode_id_invalid(u64 ino) {
>       switch (inode->i_ino) {
>       case HFS_EXT_CNID:
>       ...
>           return true;
> 
>       }
> 
>       return false;
> }
> 
> Thirdly, you can introduce an inline function that can do such check. And it
> make sense to introduce constant for the case of zero value.
> 
> Why have you missed HFS_EXT_CNID, HFS_CAT_CNID? These values cannot used in
> hfs_read_inode().

Is hfs_read_inode() never called for HFS_EXT_CNID and HFS_CAT_CNID ?

> 
>>
>>         HFS_I(inode)->flags = 0;
>>         HFS_I(inode)->rsrc_inode = NULL;
>> @@ -358,6 +361,8 @@ static int hfs_read_inode(struct inode *inode, void *data)
>>                 inode->i_op = &hfs_file_inode_operations;
>>                 inode->i_fop = &hfs_file_operations;
>>                 inode->i_mapping->a_ops = &hfs_aops;
>> +               if (inode->i_ino < HFS_FIRSTUSER_CNID && ((1U << inode->i_ino) & bad_cnid_list))
>> +                       make_bad_inode(inode);
> 
> It looks pretty complicated. You can simply use one above-mentioned function
> with the check:
> 
> if (is_inode_id_invalid(be32_to_cpu(rec->dir.DirID)))
>      <goto to make bad inode>
> 
> We can simply check the the inode ID in the beginning of the whole action:
> 
> <Make the check here>
> 		inode->i_ino = be32_to_cpu(rec->file.FlNum);
> 		inode->i_mode = S_IRUGO | S_IXUGO;
> 		if (!(rec->file.Flags & HFS_FIL_LOCK))
> 			inode->i_mode |= S_IWUGO;
> 		inode->i_mode &= ~hsb->s_file_umask;
> 		inode->i_mode |= S_IFREG;
> 		inode_set_mtime_to_ts(inode,
> 				      inode_set_atime_to_ts(inode,
> inode_set_ctime_to_ts(inode, hfs_m_to_utime(rec->file.MdDat))));
> 		inode->i_op = &hfs_file_inode_operations;
> 		inode->i_fop = &hfs_file_operations;
> 		inode->i_mapping->a_ops = &hfs_aops;
> 
> It doesn't make any sense to construct inode if we will make in bad inode,
> finally. Don't waste computational power. :)
> 
>>                 break;
>>         case HFS_CDR_DIR:
>>                 inode->i_ino = be32_to_cpu(rec->dir.DirID);
>> @@ -368,6 +373,8 @@ static int hfs_read_inode(struct inode *inode, void *data)
>>                                       inode_set_atime_to_ts(inode, inode_set_ctime_to_ts(inode, hfs_m_to_utime(rec->dir.MdDat))));
>>                 inode->i_op = &hfs_dir_inode_operations;
>>                 inode->i_fop = &hfs_dir_operations;
>> +               if (inode->i_ino < HFS_FIRSTUSER_CNID && ((1U << inode->i_ino) & bad_cnid_list))
>> +                       make_bad_inode(inode);
> 
> We already have make_bad_inode(inode) as default action. So, simply jump there.
> 
>>                 break;
>>         default:
>>                 make_bad_inode(inode);
>>
>>
>>
>> But I can't be convinced that above change is sufficient, for if I do
>>
>> +		static u8 serial;
>> +               if (inode->i_ino < HFS_FIRSTUSER_CNID && ((1U << inode->i_ino) & bad_cnid_list))
>> +                       inode->i_ino = (serial++) % 16;
> 
> I don't see the point in flags introduction. It makes logic very complicated.

The point of this change is to excecise inode->i_ino for all values between 0 and 15.
Some of values between 0 and 15 must be valid as inode->i_ino , doesn't these? Then,

> 
>>
>> instead of
>>
>> +               if (inode->i_ino < HFS_FIRSTUSER_CNID && ((1U << inode->i_ino) & bad_cnid_list))
>> +                       make_bad_inode(inode);
>>
>> , the reproducer still hits BUG() for 0, 1, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14 and 15
>> because hfs_write_inode() handles only 2, 3 and 4.
>>
> 
> How can we go into hfs_write_inode() if we created the bad inode for invalid
> inode ID? How is it possible?

are all of 0, 1, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14 and 15 invalid value for hfs_read_inode() ?

If all of 0, 1, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14 and 15 are invalid value for hfs_read_inode(),
and 3 and 4 are also invalid value for hfs_read_inode(), hfs_read_inode() would accept only 2.
Something is crazily wrong.

Can we really filter some of values between 0 and 15 at hfs_read_inode() ?

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

[Tetsuo Handa]

On 2025/07/25 7:05, Tetsuo Handa wrote:
>>> But I can't be convinced that above change is sufficient, for if I do
>>>
>>> +	            static u8 serial;
>>> +               if (inode->i_ino < HFS_FIRSTUSER_CNID && ((1U << inode->i_ino) & bad_cnid_list))
>>> +                       inode->i_ino = (serial++) % 16;
>>
>> I don't see the point in flags introduction. It makes logic very complicated.
> 
> The point of this change is to excecise inode->i_ino for all values between 0 and 15.
> Some of values between 0 and 15 must be valid as inode->i_ino , doesn't these? Then,

Background: I assume that the value of rec->dir.DirID comes from the hfs filesystem image in the
reproducer (i.e. memfd file associated with /dev/loop0 ). But since I don't know the offset to modify
the value if I want the reproducer to pass rec->dir.DirID == 1...15 instead of rec->dir.DirID == 0,
I am modifying inode->i_ino here when rec->dir.DirID == 0.

> 
>>
>>>
>>> instead of
>>>
>>> +               if (inode->i_ino < HFS_FIRSTUSER_CNID && ((1U << inode->i_ino) & bad_cnid_list))
>>> +                       make_bad_inode(inode);
>>>
>>> , the reproducer still hits BUG() for 0, 1, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14 and 15
>>> because hfs_write_inode() handles only 2, 3 and 4.
>>>
>>
>> How can we go into hfs_write_inode() if we created the bad inode for invalid
>> inode ID? How is it possible?

Calling make_bad_inode() for some of values between 0...15 at hfs_read_inode() will prevent
that inode from going into hfs_write_inode(). But regarding the values between 0...15 which
were not calling make_bad_inode() at hfs_read_inode() will not prevent that inode from going
into hfs_write_inode().

Since hfs_write_inode() calls BUG() for values 0...15 except 2...4, any values between 0...15
except 2...4 which were not calling make_bad_inode() at hfs_read_inode() will hit BUG().

If we don't remove BUG(), the values which hfs_read_inode() does not need to call
make_bad_inode() will be limited to only 2...4.

And since you say that hfs_read_inode() should call make_bad_inode() for 3...4, the only value
hfs_read_inode() can accept (from the point of view of avoid hitting BUG() in hfs_write_inode())
will be 2.

> 
> are all of 0, 1, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14 and 15 invalid value for hfs_read_inode() ?
> 
> If all of 0, 1, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14 and 15 are invalid value for hfs_read_inode(),
> and 3 and 4 are also invalid value for hfs_read_inode(), hfs_read_inode() would accept only 2.
> Something is crazily wrong.
> 
> Can we really filter some of values between 0 and 15 at hfs_read_inode() ?
> 

Can the attempt to filter some of values between 0 and 15 at hfs_read_inode() make sense,
without the attempt to remove BUG() from hfs_write_inode() ?

I think that we need to remove BUG() from hfs_write_inode(), even if you try to filter
at hfs_read_inode().

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

[Tetsuo Handa]

On 2025/07/25 8:20, Tetsuo Handa wrote:
> On 2025/07/25 7:05, Tetsuo Handa wrote:
>>>> But I can't be convinced that above change is sufficient, for if I do
>>>>
>>>> +	            static u8 serial;
>>>> +               if (inode->i_ino < HFS_FIRSTUSER_CNID && ((1U << inode->i_ino) & bad_cnid_list))
>>>> +                       inode->i_ino = (serial++) % 16;
>>>
>>> I don't see the point in flags introduction. It makes logic very complicated.
>>
>> The point of this change is to excecise inode->i_ino for all values between 0 and 15.
>> Some of values between 0 and 15 must be valid as inode->i_ino , doesn't these? Then,
> 
> Background: I assume that the value of rec->dir.DirID comes from the hfs filesystem image in the
> reproducer (i.e. memfd file associated with /dev/loop0 ). But since I don't know the offset to modify
> the value if I want the reproducer to pass rec->dir.DirID == 1...15 instead of rec->dir.DirID == 0,
> I am modifying inode->i_ino here when rec->dir.DirID == 0.
> 

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?

Discussion on what values should be filtered by hfs_read_inode() is
out of scope for this syzbot report.

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

[Viacheslav Dubeyko]

On Fri, 2025-07-25 at 07:05 +0900, Tetsuo Handa wrote:
> On 2025/07/25 4:49, Viacheslav Dubeyko wrote:
> > On Thu, 2025-07-24 at 15:55 +0900, Tetsuo Handa wrote:
> > > Then, something like below change?
> > > 
> > > --- a/fs/hfs/inode.c
> > > +++ b/fs/hfs/inode.c
> > > @@ -318,6 +318,9 @@ static int hfs_read_inode(struct inode *inode, void *data)
> > >         struct hfs_iget_data *idata = data;
> > >         struct hfs_sb_info *hsb = HFS_SB(inode->i_sb);
> > >         hfs_cat_rec *rec;
> > > +       /* https://developer.apple.com/library/archive/technotes/tn/tn1150.html#CNID     */
> > 
> > We already have all declarations in hfs.h:
> > 
> > /* Some special File ID numbers */
> > #define HFS_POR_CNID		1	/* Parent Of the Root */
> > #define HFS_ROOT_CNID		2	/* ROOT directory */
> > #define HFS_EXT_CNID		3	/* EXTents B-tree */
> > #define HFS_CAT_CNID		4	/* CATalog B-tree */
> > #define HFS_BAD_CNID		5	/* BAD blocks file */
> > #define HFS_ALLOC_CNID		6	/* ALLOCation file (HFS+) */
> > #define HFS_START_CNID		7	/* STARTup file (HFS+) */
> > #define HFS_ATTR_CNID		8	/* ATTRibutes file (HFS+) */
> > #define HFS_EXCH_CNID		15	/* ExchangeFiles temp id */
> > #define HFS_FIRSTUSER_CNID	16
> 
> These declarations does not define 14, and some flags are never used despite
> being declared here.
> 
> > 
> > So, adding the link here doesn't make any sense.
> > 
> > > +       static const u16 bad_cnid_list = (1 << 0) | (1 << 6) | (1 << 7) | (1 << 8) |
> > > +               (1 << 9) | (1 << 10) | (1 << 11) | (1 << 12) | (1 << 13);
> 
> Some of values in this constant are not declared.
> 

It means that we need to declare the missing values. But hardcoded bare numbers
are really bad practice.

> > 
> > I don't see any sense to introduce flags here. First of all, please, don't use
> > hardcoded values but you should use declared constants from hfs.h (for example,
> > HFS_EXT_CNID instead of 3). Secondly, you can simply compare the i_ino with
> > constants, for example:
> 
> This will save a lot of computational power compared to switch().
> 

Even if you would like to use flags, then the logic must to be simple and
understandable. You still can use special inline function and do not create a
mess in hfs_read_inode(). Especially, you can declare the mask one time in
header, for example, but not to prepare the bad_cnid_list for every function
call. Currently, the code looks really messy.

> > 
> > bool is_inode_id_invalid(u64 ino) {
> >       switch (inode->i_ino) {
> >       case HFS_EXT_CNID:
> >       ...
> >           return true;
> > 
> >       }
> > 
> >       return false;
> > }
> > 
> > Thirdly, you can introduce an inline function that can do such check. And it
> > make sense to introduce constant for the case of zero value.
> > 
> > Why have you missed HFS_EXT_CNID, HFS_CAT_CNID? These values cannot used in
> > hfs_read_inode().
> 
> Is hfs_read_inode() never called for HFS_EXT_CNID and HFS_CAT_CNID ?
> 

The location of Catalog File and Extents File are defined in superblock. As a
result, Catalog File cannot contain a record with CNID HFS_EXT_CNID or
HFS_CAT_CNID. And if hfs_read_inode() receives these values, then it is some
corruption of Catalog File.

> > 
> > > 
> > >         HFS_I(inode)->flags = 0;
> > >         HFS_I(inode)->rsrc_inode = NULL;
> > > @@ -358,6 +361,8 @@ static int hfs_read_inode(struct inode *inode, void *data)
> > >                 inode->i_op = &hfs_file_inode_operations;
> > >                 inode->i_fop = &hfs_file_operations;
> > >                 inode->i_mapping->a_ops = &hfs_aops;
> > > +               if (inode->i_ino < HFS_FIRSTUSER_CNID && ((1U << inode->i_ino) & bad_cnid_list))
> > > +                       make_bad_inode(inode);
> > 
> > It looks pretty complicated. You can simply use one above-mentioned function
> > with the check:
> > 
> > if (is_inode_id_invalid(be32_to_cpu(rec->dir.DirID)))
> >      <goto to make bad inode>
> > 
> > We can simply check the the inode ID in the beginning of the whole action:
> > 
> > <Make the check here>
> > 		inode->i_ino = be32_to_cpu(rec->file.FlNum);
> > 		inode->i_mode = S_IRUGO | S_IXUGO;
> > 		if (!(rec->file.Flags & HFS_FIL_LOCK))
> > 			inode->i_mode |= S_IWUGO;
> > 		inode->i_mode &= ~hsb->s_file_umask;
> > 		inode->i_mode |= S_IFREG;
> > 		inode_set_mtime_to_ts(inode,
> > 				      inode_set_atime_to_ts(inode,
> > inode_set_ctime_to_ts(inode, hfs_m_to_utime(rec->file.MdDat))));
> > 		inode->i_op = &hfs_file_inode_operations;
> > 		inode->i_fop = &hfs_file_operations;
> > 		inode->i_mapping->a_ops = &hfs_aops;
> > 
> > It doesn't make any sense to construct inode if we will make in bad inode,
> > finally. Don't waste computational power. :)
> > 
> > >                 break;
> > >         case HFS_CDR_DIR:
> > >                 inode->i_ino = be32_to_cpu(rec->dir.DirID);
> > > @@ -368,6 +373,8 @@ static int hfs_read_inode(struct inode *inode, void *data)
> > >                                       inode_set_atime_to_ts(inode, inode_set_ctime_to_ts(inode, hfs_m_to_utime(rec->dir.MdDat))));
> > >                 inode->i_op = &hfs_dir_inode_operations;
> > >                 inode->i_fop = &hfs_dir_operations;
> > > +               if (inode->i_ino < HFS_FIRSTUSER_CNID && ((1U << inode->i_ino) & bad_cnid_list))
> > > +                       make_bad_inode(inode);
> > 
> > We already have make_bad_inode(inode) as default action. So, simply jump there.
> > 
> > >                 break;
> > >         default:
> > >                 make_bad_inode(inode);
> > > 
> > > 
> > > 
> > > But I can't be convinced that above change is sufficient, for if I do
> > > 
> > > +		static u8 serial;
> > > +               if (inode->i_ino < HFS_FIRSTUSER_CNID && ((1U << inode->i_ino) & bad_cnid_list))
> > > +                       inode->i_ino = (serial++) % 16;
> > 
> > I don't see the point in flags introduction. It makes logic very complicated.
> 
> The point of this change is to excecise inode->i_ino for all values between 0 and 15.
> Some of values between 0 and 15 must be valid as inode->i_ino , doesn't these? Then,
> 

If you have mask of valid or/and invalid, then you can simply check that this
mask contain  the flag. It will bed simple bit state checking. Currently, the
code looks weird, not clear, complicated, and inefficient.

> > 
> > > 
> > > instead of
> > > 
> > > +               if (inode->i_ino < HFS_FIRSTUSER_CNID && ((1U << inode->i_ino) & bad_cnid_list))
> > > +                       make_bad_inode(inode);
> > > 
> > > , the reproducer still hits BUG() for 0, 1, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14 and 15
> > > because hfs_write_inode() handles only 2, 3 and 4.
> > > 
> > 
> > How can we go into hfs_write_inode() if we created the bad inode for invalid
> > inode ID? How is it possible?
> 
> are all of 0, 1, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14 and 15 invalid value for hfs_read_inode() ?
> 
> If all of 0, 1, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14 and 15 are invalid value for hfs_read_inode(),
> and 3 and 4 are also invalid value for hfs_read_inode(), hfs_read_inode() would accept only 2.
> Something is crazily wrong.
> 
> Can we really filter some of values between 0 and 15 at hfs_read_inode() ?

0 value is invalid.

#define HFS_POR_CNID		1	/* Parent Of the Root */
#define HFS_ROOT_CNID		2	/* ROOT directory */

These values are legitimate values.

#define HFS_EXT_CNID		3	/* EXTents B-tree */
#define HFS_CAT_CNID		4	/* CATalog B-tree */

This metadata structures are defined in MDB. This is invalid values for
hfs_read_inode().

#define HFS_BAD_CNID		5	/* BAD blocks file */

This could be defined in Catalog File because MDB has nothing for this metadata
structure. However, it's ancient technology.

#define HFS_ALLOC_CNID		6	/* ALLOCation file (HFS+) */
#define HFS_START_CNID		7	/* STARTup file (HFS+) */
#define HFS_ATTR_CNID		8	/* ATTRibutes file (HFS+) */

These value are invalid for HFS.

9, 10, 11, 12, 13, 14 can be defined as constants and it is invalid values. Foe
example:

#define HFS_RESERVED_CNID_9    9
#define HFS_RESERVED_CNID_10   10
...
#define HFS_RESERVED_CNID_14   14

#define HFS_EXCH_CNID		15	/* ExchangeFiles temp id */

This could be defined in Catalog File (maybe not). I didn't find anything
related to this in HFS specification.

So, 1, 2, 5, 15, etc can be accepted by hfs_read_inode().
0, 3, 4, 6, 7, 8, 9, 10, 11, 12, 13, 14 is invalid values for hfs_read_inode().

Thanks,
Slava.

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

[Viacheslav Dubeyko]

On Fri, 2025-07-25 at 08:20 +0900, Tetsuo Handa wrote:
> On 2025/07/25 7:05, Tetsuo Handa wrote:
> > > > But I can't be convinced that above change is sufficient, for if I do
> > > > 
> > > > +	            static u8 serial;
> > > > +               if (inode->i_ino < HFS_FIRSTUSER_CNID && ((1U << inode->i_ino) & bad_cnid_list))
> > > > +                       inode->i_ino = (serial++) % 16;
> > > 
> > > I don't see the point in flags introduction. It makes logic very complicated.
> > 
> > The point of this change is to excecise inode->i_ino for all values between 0 and 15.
> > Some of values between 0 and 15 must be valid as inode->i_ino , doesn't these? Then,
> 
> Background: I assume that the value of rec->dir.DirID comes from the hfs filesystem image in the
> reproducer (i.e. memfd file associated with /dev/loop0 ). But since I don't know the offset to modify
> the value if I want the reproducer to pass rec->dir.DirID == 1...15 instead of rec->dir.DirID == 0,
> I am modifying inode->i_ino here when rec->dir.DirID == 0.
> 
> > 
> > > 
> > > > 
> > > > instead of
> > > > 
> > > > +               if (inode->i_ino < HFS_FIRSTUSER_CNID && ((1U << inode->i_ino) & bad_cnid_list))
> > > > +                       make_bad_inode(inode);
> > > > 
> > > > , the reproducer still hits BUG() for 0, 1, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14 and 15
> > > > because hfs_write_inode() handles only 2, 3 and 4.
> > > > 
> > > 
> > > How can we go into hfs_write_inode() if we created the bad inode for invalid
> > > inode ID? How is it possible?
> 
> Calling make_bad_inode() for some of values between 0...15 at hfs_read_inode() will prevent
> that inode from going into hfs_write_inode(). But regarding the values between 0...15 which
> were not calling make_bad_inode() at hfs_read_inode() will not prevent that inode from going
> into hfs_write_inode().
> 
> Since hfs_write_inode() calls BUG() for values 0...15 except 2...4, any values between 0...15
> except 2...4 which were not calling make_bad_inode() at hfs_read_inode() will hit BUG().
> 
> If we don't remove BUG(), the values which hfs_read_inode() does not need to call
> make_bad_inode() will be limited to only 2...4.
> 
> And since you say that hfs_read_inode() should call make_bad_inode() for 3...4, the only value
> hfs_read_inode() can accept (from the point of view of avoid hitting BUG() in hfs_write_inode())
> will be 2.
> 
> > 
> > are all of 0, 1, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14 and 15 invalid value for hfs_read_inode() ?
> > 
> > If all of 0, 1, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14 and 15 are invalid value for hfs_read_inode(),
> > and 3 and 4 are also invalid value for hfs_read_inode(), hfs_read_inode() would accept only 2.
> > Something is crazily wrong.
> > 
> > Can we really filter some of values between 0 and 15 at hfs_read_inode() ?
> > 
> 
> Can the attempt to filter some of values between 0 and 15 at hfs_read_inode() make sense,
> without the attempt to remove BUG() from hfs_write_inode() ?
> 
> I think that we need to remove BUG() from hfs_write_inode(), even if you try to filter
> at hfs_read_inode().

If we manage the inode IDs properly in hfs_read_inode(), then hfs_write_inode()
never will receive the invalid inode ID. I don't see the point to remove the
BUG() in hfs_write_inode().

Thanks,
Slava.

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

[Viacheslav Dubeyko]

On Fri, 2025-07-25 at 13:16 +0900, Tetsuo Handa wrote:
> On 2025/07/25 8:20, Tetsuo Handa wrote:
> > On 2025/07/25 7:05, Tetsuo Handa wrote:
> > > > > But I can't be convinced that above change is sufficient, for if I do
> > > > > 
> > > > > +	            static u8 serial;
> > > > > +               if (inode->i_ino < HFS_FIRSTUSER_CNID && ((1U << inode->i_ino) & bad_cnid_list))
> > > > > +                       inode->i_ino = (serial++) % 16;
> > > > 
> > > > I don't see the point in flags introduction. It makes logic very complicated.
> > > 
> > > The point of this change is to excecise inode->i_ino for all values between 0 and 15.
> > > Some of values between 0 and 15 must be valid as inode->i_ino , doesn't these? Then,
> > 
> > Background: I assume that the value of rec->dir.DirID comes from the hfs filesystem image in the
> > reproducer (i.e. memfd file associated with /dev/loop0 ). But since I don't know the offset to modify
> > the value if I want the reproducer to pass rec->dir.DirID == 1...15 instead of rec->dir.DirID == 0,
> > I am modifying inode->i_ino here when rec->dir.DirID == 0.
> > 
> 
> 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?
> 
> Discussion on what values should be filtered by hfs_read_inode() is
> out of scope for this syzbot report.

I already shared in previous emails which particular inode IDs are valid or not
for [0-16] group of values in the environment of hfs_read_inode().

Thanks,
Slava.

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

[Tetsuo Handa]

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

>>
>> Discussion on what values should be filtered by hfs_read_inode() is
>> out of scope for this syzbot report.
> 
> I already shared in previous emails which particular inode IDs are valid or not
> for [0-16] group of values in the environment of hfs_read_inode().
> 

Checking which particular inode IDs hfs_read_inode() should accept is fine.
But such check cannot fix the bug reported at
https://syzkaller.appspot.com/bug?extid=97e301b4b82ae803d21b .

Checking the inode ID retrieved by hfs_cat_find_brec(HFS_ROOT_CNID) is indeed
HFS_ROOT_CNID can fix the bug reported at
https://syzkaller.appspot.com/bug?extid=97e301b4b82ae803d21b .

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

[Tetsuo Handa]

On 2025/07/26 2:42, Viacheslav Dubeyko wrote:
>>>
>>> I don't see any sense to introduce flags here. First of all, please, don't use
>>> hardcoded values but you should use declared constants from hfs.h (for example,
>>> HFS_EXT_CNID instead of 3). Secondly, you can simply compare the i_ino with
>>> constants, for example:
>>
>> This will save a lot of computational power compared to switch().
>>
> 
> Even if you would like to use flags, then the logic must to be simple and
> understandable. You still can use special inline function and do not create a
> mess in hfs_read_inode(). Especially, you can declare the mask one time in
> header, for example, but not to prepare the bad_cnid_list for every function
> call. Currently, the code looks really messy.

No, since this is "static const u16", the compiler will prepare it at build time
than every function call. Also since it is a simple u16, the compiler would
generate simple code like

  test ax, an_imm16_constant_value_determined_at_build_time
  jnz somewhere

which is much faster than

  cmp eax, HFS_EXT_CNID
  je somewhere
  cmp eax, other_cnid_1
  je somewhere
  cmp eax, other_cnid_2
  je somewhere
  cmp eax, other_cnid_3
  je somewhere
  cmp eax, other_cnid_4
  je somewhere

based on switch() in is_inode_id_invalid() shown below.

We can replace "static const u16" with "#define" if you prefer.

> 
>>>
>>> bool is_inode_id_invalid(u64 ino) {
>>>       switch (inode->i_ino) {
>>>       case HFS_EXT_CNID:
>>>       ...
>>>           return true;
>>>
>>>       }
>>>
>>>       return false;
>>> }



> So, 1, 2, 5, 15, etc can be accepted by hfs_read_inode().
> 0, 3, 4, 6, 7, 8, 9, 10, 11, 12, 13, 14 is invalid values for hfs_read_inode().

OK. This list will be useful for hardening, but we can't use this list for fixing
the bug reported at https://syzkaller.appspot.com/bug?extid=97e301b4b82ae803d21b .

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

[Tetsuo Handa]

On 2025/07/26 2:45, Viacheslav Dubeyko wrote:
> If we manage the inode IDs properly in hfs_read_inode(), then hfs_write_inode()
> never will receive the invalid inode ID. I don't see the point to remove the
> BUG() in hfs_write_inode().

As long as we don't check that rec.dir.DirID is HFS_ROOT_CNID at hfs_fill_super(), 
hfs_write_inode() shall receive the invalid inode ID upon unmount operation.

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

[Tetsuo Handa]

On 2025/07/26 7:25, Tetsuo Handa wrote:
> On 2025/07/26 2:45, Viacheslav Dubeyko wrote:
>> If we manage the inode IDs properly in hfs_read_inode(), then hfs_write_inode()
>> never will receive the invalid inode ID. I don't see the point to remove the
>> BUG() in hfs_write_inode().
> 
> As long as we don't check that rec.dir.DirID is HFS_ROOT_CNID at hfs_fill_super(), 
> hfs_write_inode() shall receive the invalid inode ID upon unmount operation.
> 

Here is the steps to confirm.

$ wget -O hfs.c 'https://syzkaller.appspot.com/text?tag=ReproC&x=111450f0580000'
$ patch -lp1 << "EOF"
--- a/hfs.c
+++ b/hfs.c
@@ -34,6 +34,7 @@
 #endif

 static unsigned long long procid;
+static unsigned int dirid;

 static void sleep_ms(uint64_t ms)
 {
@@ -437,6 +438,7 @@
       goto error_close_loop;
     }
   }
+  pwrite(memfd, &dirid, sizeof(dirid), 4096 + 548);
   close(memfd);
   *loopfd_p = loopfd;
   return 0;
@@ -669,8 +671,10 @@
   syz_mount_image(/*fs=*/0, /*dir=*/0x200000002080, /*flags=*/0, /*opts=*/0,
                   /*chdir=*/0, /*size=*/0, /*img=*/0);
 }
-int main(void)
+int main(int argc, char *argv[])
 {
+  if (argc == 2)
+    dirid = (atoi(argv[1]) & 15) * 0x1000000;
   syscall(__NR_mmap, /*addr=*/0x1ffffffff000ul, /*len=*/0x1000ul, /*prot=*/0ul,
           /*flags=MAP_FIXED|MAP_ANONYMOUS|MAP_PRIVATE*/ 0x32ul,
           /*fd=*/(intptr_t)-1, /*offset=*/0ul);
EOF
$ gcc -Wall -O2 -o hfs hfs.c
# timeout 1 unshare -m ./hfs 1

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

[Viacheslav Dubeyko]

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.

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

[Tetsuo Handa]

On 2025/07/29 4:37, Viacheslav Dubeyko wrote:
> I don't see the point of making modifications here. The modification of
> hfs_read_inode() should be enough.

Sigh... Did you try
https://lkml.kernel.org/r/a8f8da77-f099-499b-98e0-39ed159b6a2d@I-love.SAKURA.ne.jp ?

I'm repeating that the modification of hfs_read_inode() does not fix
the bug syzbot is reporting.

[PATCH v4] hfs: update sanity check of the root record

[Tetsuo Handa]

syzbot is reporting that BUG() in hfs_write_inode() fires upon unmount
operation when the inode number of the record retrieved as a result of
hfs_cat_find_brec(HFS_ROOT_CNID) is not HFS_ROOT_CNID, for
commit b905bafdea21 ("hfs: Sanity check the root record") checked
the record size and the record type but did not check the inode number.

Reported-by: syzbot+97e301b4b82ae803d21b@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=97e301b4b82ae803d21b
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 fs/hfs/super.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/hfs/super.c b/fs/hfs/super.c
index fe09c2093a93..d231989b4e23 100644
--- 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)
-- 
2.50.1

Re: [PATCH v4] hfs: update sanity check of the root record

[Viacheslav Dubeyko]

On Wed, 2025-07-30 at 08:21 +0900, Tetsuo Handa wrote:
> syzbot is reporting that BUG() in hfs_write_inode() fires upon unmount
> operation when the inode number of the record retrieved as a result of
> hfs_cat_find_brec(HFS_ROOT_CNID) is not HFS_ROOT_CNID, for
> commit b905bafdea21 ("hfs: Sanity check the root record") checked
> the record size and the record type but did not check the inode number.
> 
> Reported-by: syzbot+97e301b4b82ae803d21b@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=97e301b4b82ae803d21b  
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> ---
>  fs/hfs/super.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/hfs/super.c b/fs/hfs/super.c
> index fe09c2093a93..d231989b4e23 100644
> --- 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)

Why do not localize the all checks in hfs_read_inode()?

We will do such logic then [1], even if rec.dir.DirID !=
cpu_to_be32(HFS_ROOT_CNID):

root_inode = hfs_iget(sb, &fd.search_key->cat, &rec);
hfs_find_exit(&fd);
if (!root_inode)
	goto bail_no_root;

The hfs_iget() calls iget5_locked() [2]:

inode = iget5_locked(sb, cnid, hfs_test_inode, hfs_read_inode, &data);

And hfs_read_inode() will be called, finally. If inode ID is wrong, then
make_bad_inode(inode) can be called [3].

If we considering case HFS_CDR_DIR in hfs_read_inode(), then we know that it
could be HFS_POR_CNID, HFS_ROOT_CNID, or >= HFS_FIRSTUSER_CNID. Do you mean that
HFS_POR_CNID could be a problem in hfs_write_inode()?

Thanks,
Slava.

[1] https://elixir.bootlin.com/linux/v6.16-rc6/source/fs/hfs/super.c#L363
[2] https://elixir.bootlin.com/linux/v6.16-rc6/source/fs/hfs/inode.c#L403
[3] 
https://elixir.bootlin.com/linux/v6.16-rc6/source/fs/hfs/inode.c#L373

Re: [PATCH v4] hfs: update sanity check of the root record

[Tetsuo Handa]

On 2025/07/31 4:24, Viacheslav Dubeyko wrote:
> If we considering case HFS_CDR_DIR in hfs_read_inode(), then we know that it
> could be HFS_POR_CNID, HFS_ROOT_CNID, or >= HFS_FIRSTUSER_CNID. Do you mean that
> HFS_POR_CNID could be a problem in hfs_write_inode()?

Yes. Passing one of 1, 5 or 15 instead of 2 from hfs_fill_super() triggers BUG()
in hfs_write_inode(). We *MUST* validate at hfs_fill_super(), or hfs_read_inode()
shall have to also reject 1, 5 and 15 (and as a result only accept 2).

RE: [PATCH v4] hfs: update sanity check of the root record

[Viacheslav Dubeyko]

On Thu, 2025-07-31 at 07:02 +0900, Tetsuo Handa wrote:
> On 2025/07/31 4:24, Viacheslav Dubeyko wrote:
> > If we considering case HFS_CDR_DIR in hfs_read_inode(), then we know that it
> > could be HFS_POR_CNID, HFS_ROOT_CNID, or >= HFS_FIRSTUSER_CNID. Do you mean that
> > HFS_POR_CNID could be a problem in hfs_write_inode()?
> 
> Yes. Passing one of 1, 5 or 15 instead of 2 from hfs_fill_super() triggers BUG()
> in hfs_write_inode(). We *MUST* validate at hfs_fill_super(), or hfs_read_inode()
> shall have to also reject 1, 5 and 15 (and as a result only accept 2).

The fix should be in hfs_read_inode(). Currently, suggested solution hides the
issue but not fix the problem. Because b-tree nodes could contain multiple
corrupted records. Now, this patch checks only record for root folder. Let's
imagine that root folder record will be OK but another record(s) will be
corrupted in such way. Finally, we will have successful mount but operation with
corrupted record(s) will trigger this issue. So, I cannot consider this patch as
a complete fix of the problem.

Thanks,
Slava.

Re: [PATCH v4] hfs: update sanity check of the root record

[Tetsuo Handa]

On 2025/08/01 3:03, Viacheslav Dubeyko wrote:
> On Thu, 2025-07-31 at 07:02 +0900, Tetsuo Handa wrote:
>> On 2025/07/31 4:24, Viacheslav Dubeyko wrote:
>>> If we considering case HFS_CDR_DIR in hfs_read_inode(), then we know that it
>>> could be HFS_POR_CNID, HFS_ROOT_CNID, or >= HFS_FIRSTUSER_CNID. Do you mean that
>>> HFS_POR_CNID could be a problem in hfs_write_inode()?
>>
>> Yes. Passing one of 1, 5 or 15 instead of 2 from hfs_fill_super() triggers BUG()
>> in hfs_write_inode(). We *MUST* validate at hfs_fill_super(), or hfs_read_inode()
>> shall have to also reject 1, 5 and 15 (and as a result only accept 2).
> 
> The fix should be in hfs_read_inode(). Currently, suggested solution hides the
> issue but not fix the problem.

Not fixing this problem might be hiding other issues, by hitting BUG() before
other issues shows up.

> Because b-tree nodes could contain multiple
> corrupted records. Now, this patch checks only record for root folder. Let's
> imagine that root folder record will be OK but another record(s) will be
> corrupted in such way.

Can the inode number of the record retrieved as a result of
hfs_cat_find_brec(HFS_ROOT_CNID) be something other than HFS_ROOT_CNID ?

If the inode number of the record retrieved as a result of
hfs_cat_find_brec(HFS_ROOT_CNID) must be HFS_ROOT_CNID, this patch itself will be
a complete fix for this problem.

> Finally, we will have successful mount but operation with
> corrupted record(s) will trigger this issue. So, I cannot consider this patch as
> a complete fix of the problem.

Did you try what you think as a fix of this problem (I guess something like
shown below will be needed for avoid hitting BUG()) using
https://lkml.kernel.org/r/a8f8da77-f099-499b-98e0-39ed159b6a2d@I-love.SAKURA.ne.jp ?

diff --git a/fs/hfs/inode.c b/fs/hfs/inode.c
index a81ce7a740b9..d60395111ed5 100644
--- a/fs/hfs/inode.c
+++ b/fs/hfs/inode.c
@@ -81,7 +81,8 @@ static bool hfs_release_folio(struct folio *folio, gfp_t mask)
 		tree = HFS_SB(sb)->cat_tree;
 		break;
 	default:
-		BUG();
+		pr_err("detected unknown inode %lu, running fsck.hfs is recommended.\n",
+		       inode->i_ino);
 		return false;
 	}
 
@@ -305,11 +306,31 @@ static int hfs_test_inode(struct inode *inode, void *data)
 	case HFS_CDR_FIL:
 		return inode->i_ino == be32_to_cpu(rec->file.FlNum);
 	default:
-		BUG();
+		pr_err("detected unknown type %u, running fsck.hfs is recommended.\n", rec->type);
 		return 1;
 	}
 }
 
+static bool is_bad_id(unsigned long ino)
+{
+	switch (ino) {
+	case 0:
+	case 3:
+	case 4:
+	case 6:
+	case 7:
+	case 8:
+	case 9:
+	case 10:
+	case 11:
+	case 12:
+	case 13:
+	case 14:
+		return true;
+	}
+	return false;
+}
+
 /*
  * hfs_read_inode
  */
@@ -348,6 +369,10 @@ static int hfs_read_inode(struct inode *inode, void *data)
 		}
 
 		inode->i_ino = be32_to_cpu(rec->file.FlNum);
+		if (is_bad_id(inode->i_ino)) {
+			make_bad_inode(inode);
+			break;
+		}
 		inode->i_mode = S_IRUGO | S_IXUGO;
 		if (!(rec->file.Flags & HFS_FIL_LOCK))
 			inode->i_mode |= S_IWUGO;
@@ -358,9 +383,15 @@ static int hfs_read_inode(struct inode *inode, void *data)
 		inode->i_op = &hfs_file_inode_operations;
 		inode->i_fop = &hfs_file_operations;
 		inode->i_mapping->a_ops = &hfs_aops;
+		if (inode->i_ino < 16)
+			pr_info("HFS_CDR_FIL i_ino=%ld\n", inode->i_ino);
 		break;
 	case HFS_CDR_DIR:
 		inode->i_ino = be32_to_cpu(rec->dir.DirID);
+		if (is_bad_id(inode->i_ino)) {
+			make_bad_inode(inode);
+			break;
+		}
 		inode->i_size = be16_to_cpu(rec->dir.Val) + 2;
 		HFS_I(inode)->fs_blocks = 0;
 		inode->i_mode = S_IFDIR | (S_IRWXUGO & ~hsb->s_dir_umask);
@@ -368,6 +399,8 @@ static int hfs_read_inode(struct inode *inode, void *data)
 				      inode_set_atime_to_ts(inode, inode_set_ctime_to_ts(inode, hfs_m_to_utime(rec->dir.MdDat))));
 		inode->i_op = &hfs_dir_inode_operations;
 		inode->i_fop = &hfs_dir_operations;
+		if (inode->i_ino < 16)
+			pr_info("HFS_CDR_DIR i_ino=%ld\n", inode->i_ino);
 		break;
 	default:
 		make_bad_inode(inode);
@@ -441,7 +474,8 @@ int hfs_write_inode(struct inode *inode, struct writeback_control *wbc)
 			hfs_btree_write(HFS_SB(inode->i_sb)->cat_tree);
 			return 0;
 		default:
-			BUG();
+			pr_err("detected unknown inode %lu, running fsck.hfs is recommended.\n",
+			       inode->i_ino);
 			return -EIO;
 		}
 	}


# for i in $(seq 0 15); do timeout 1 unshare -m ./hfs $i; done
# dmesg | grep fsck
[   52.563547] [    T479] hfs: detected unknown inode 1, running fsck.hfs is recommended.
[   56.606238] [    T255] hfs: detected unknown inode 5, running fsck.hfs is recommended.
[   66.694795] [    T500] hfs: detected unknown inode 15, running fsck.hfs is recommended.

RE: [PATCH v4] hfs: update sanity check of the root record

[Viacheslav Dubeyko]

On Fri, 2025-08-01 at 06:12 +0900, Tetsuo Handa wrote:
> On 2025/08/01 3:03, Viacheslav Dubeyko wrote:
> > On Thu, 2025-07-31 at 07:02 +0900, Tetsuo Handa wrote:
> > > On 2025/07/31 4:24, Viacheslav Dubeyko wrote:
> > > > If we considering case HFS_CDR_DIR in hfs_read_inode(), then we know that it
> > > > could be HFS_POR_CNID, HFS_ROOT_CNID, or >= HFS_FIRSTUSER_CNID. Do you mean that
> > > > HFS_POR_CNID could be a problem in hfs_write_inode()?
> > > 
> > > Yes. Passing one of 1, 5 or 15 instead of 2 from hfs_fill_super() triggers BUG()
> > > in hfs_write_inode(). We *MUST* validate at hfs_fill_super(), or hfs_read_inode()
> > > shall have to also reject 1, 5 and 15 (and as a result only accept 2).
> > 
> > The fix should be in hfs_read_inode(). Currently, suggested solution hides the
> > issue but not fix the problem.
> 
> Not fixing this problem might be hiding other issues, by hitting BUG() before
> other issues shows up.
> 

I am not going to start a philosophical discussion. We simply need to fix the
bug. The suggested patch doesn't fix the issue.

> > Because b-tree nodes could contain multiple
> > corrupted records. Now, this patch checks only record for root folder. Let's
> > imagine that root folder record will be OK but another record(s) will be
> > corrupted in such way.
> 
> Can the inode number of the record retrieved as a result of
> hfs_cat_find_brec(HFS_ROOT_CNID) be something other than HFS_ROOT_CNID ?
> 
> If the inode number of the record retrieved as a result of
> hfs_cat_find_brec(HFS_ROOT_CNID) must be HFS_ROOT_CNID, this patch itself will be
> a complete fix for this problem.
> 

You are working with corrupted volume. In this case, you can extract any state
of the Catalog File's record.

> > Finally, we will have successful mount but operation with
> > corrupted record(s) will trigger this issue. So, I cannot consider this patch as
> > a complete fix of the problem.
> 
> Did you try what you think as a fix of this problem (I guess something like
> shown below will be needed for avoid hitting BUG()) using
> https://lkml.kernel.org/r/a8f8da77-f099-499b-98e0-39ed159b6a2d@I-love.SAKURA.ne.jp   ?
> 

If you believe that you have another version of the patch, then simply send it
and I will review it. Sorry, I haven't enough time to discuss every movement of
your thoughts.

> diff --git a/fs/hfs/inode.c b/fs/hfs/inode.c
> index a81ce7a740b9..d60395111ed5 100644
> --- a/fs/hfs/inode.c
> +++ b/fs/hfs/inode.c
> @@ -81,7 +81,8 @@ static bool hfs_release_folio(struct folio *folio, gfp_t mask)
>  		tree = HFS_SB(sb)->cat_tree;
>  		break;
>  	default:
> -		BUG();
> +		pr_err("detected unknown inode %lu, running fsck.hfs is recommended.\n",
> +		       inode->i_ino);
>  		return false;
>  	}
>  
> @@ -305,11 +306,31 @@ static int hfs_test_inode(struct inode *inode, void *data)
>  	case HFS_CDR_FIL:
>  		return inode->i_ino == be32_to_cpu(rec->file.FlNum);
>  	default:
> -		BUG();
> +		pr_err("detected unknown type %u, running fsck.hfs is recommended.\n", rec->type);
>  		return 1;
>  	}
>  }
>  
> +static bool is_bad_id(unsigned long ino)
> +{
> +	switch (ino) {
> +	case 0:
> +	case 3:
> +	case 4:
> +	case 6:
> +	case 7:
> +	case 8:
> +	case 9:
> +	case 10:
> +	case 11:
> +	case 12:
> +	case 13:
> +	case 14:
> +		return true;
> +	}
> +	return false;
> +}

Please, don't use hardcoded value. I already shared the point that we must use
the declared constants.

This function is incorrect and it cannot work for folders and files at the same
time.

Thanks,
Slava.

> +
>  /*
>   * hfs_read_inode
>   */
> @@ -348,6 +369,10 @@ static int hfs_read_inode(struct inode *inode, void *data)
>  		}
>  
>  		inode->i_ino = be32_to_cpu(rec->file.FlNum);
> +		if (is_bad_id(inode->i_ino)) {
> +			make_bad_inode(inode);
> +			break;
> +		}
>  		inode->i_mode = S_IRUGO | S_IXUGO;
>  		if (!(rec->file.Flags & HFS_FIL_LOCK))
>  			inode->i_mode |= S_IWUGO;
> @@ -358,9 +383,15 @@ static int hfs_read_inode(struct inode *inode, void *data)
>  		inode->i_op = &hfs_file_inode_operations;
>  		inode->i_fop = &hfs_file_operations;
>  		inode->i_mapping->a_ops = &hfs_aops;
> +		if (inode->i_ino < 16)
> +			pr_info("HFS_CDR_FIL i_ino=%ld\n", inode->i_ino);
>  		break;
>  	case HFS_CDR_DIR:
>  		inode->i_ino = be32_to_cpu(rec->dir.DirID);
> +		if (is_bad_id(inode->i_ino)) {
> +			make_bad_inode(inode);
> +			break;
> +		}
>  		inode->i_size = be16_to_cpu(rec->dir.Val) + 2;
>  		HFS_I(inode)->fs_blocks = 0;
>  		inode->i_mode = S_IFDIR | (S_IRWXUGO & ~hsb->s_dir_umask);
> @@ -368,6 +399,8 @@ static int hfs_read_inode(struct inode *inode, void *data)
>  				      inode_set_atime_to_ts(inode, inode_set_ctime_to_ts(inode, hfs_m_to_utime(rec->dir.MdDat))));
>  		inode->i_op = &hfs_dir_inode_operations;
>  		inode->i_fop = &hfs_dir_operations;
> +		if (inode->i_ino < 16)
> +			pr_info("HFS_CDR_DIR i_ino=%ld\n", inode->i_ino);
>  		break;
>  	default:
>  		make_bad_inode(inode);
> @@ -441,7 +474,8 @@ int hfs_write_inode(struct inode *inode, struct writeback_control *wbc)
>  			hfs_btree_write(HFS_SB(inode->i_sb)->cat_tree);
>  			return 0;
>  		default:
> -			BUG();
> +			pr_err("detected unknown inode %lu, running fsck.hfs is recommended.\n",
> +			       inode->i_ino);
>  			return -EIO;
>  		}
>  	}
> 
> 
> # for i in $(seq 0 15); do timeout 1 unshare -m ./hfs $i; done
> # dmesg | grep fsck
> [   52.563547] [    T479] hfs: detected unknown inode 1, running fsck.hfs is recommended.
> [   56.606238] [    T255] hfs: detected unknown inode 5, running fsck.hfs is recommended.
> [   66.694795] [    T500] hfs: detected unknown inode 15, running fsck.hfs is recommended.

Re: [PATCH v4] hfs: update sanity check of the root record

[Tetsuo Handa]

On 2025/08/02 3:26, Viacheslav Dubeyko wrote:
> On Fri, 2025-08-01 at 06:12 +0900, Tetsuo Handa wrote:
>> On 2025/08/01 3:03, Viacheslav Dubeyko wrote:
>>> On Thu, 2025-07-31 at 07:02 +0900, Tetsuo Handa wrote:
>>>> On 2025/07/31 4:24, Viacheslav Dubeyko wrote:
>>>>> If we considering case HFS_CDR_DIR in hfs_read_inode(), then we know that it
>>>>> could be HFS_POR_CNID, HFS_ROOT_CNID, or >= HFS_FIRSTUSER_CNID. Do you mean that
>>>>> HFS_POR_CNID could be a problem in hfs_write_inode()?
>>>>
>>>> Yes. Passing one of 1, 5 or 15 instead of 2 from hfs_fill_super() triggers BUG()
>>>> in hfs_write_inode(). We *MUST* validate at hfs_fill_super(), or hfs_read_inode()
>>>> shall have to also reject 1, 5 and 15 (and as a result only accept 2).
>>>
>>> The fix should be in hfs_read_inode(). Currently, suggested solution hides the
>>> issue but not fix the problem.
>>
>> Not fixing this problem might be hiding other issues, by hitting BUG() before
>> other issues shows up.
>>
> 
> I am not going to start a philosophical discussion. We simply need to fix the
> bug. The suggested patch doesn't fix the issue.

What is your issue?

My issue (what syzbot is reporting) is that the kernel crashes if the inode number
of the record retrieved as a result of hfs_cat_find_brec(HFS_ROOT_CNID) is not
HFS_ROOT_CNID. My suggested patch does fix my issue.

> Please, don't use hardcoded value. I already shared the point that we must use
> the declared constants.
> 
> This function is incorrect and it cannot work for folders and files at the same
> time.

I already shared that I don't plan to try writing such function
( http://lkml.kernel.org/r/38d8f48e-47c3-4d67-9caa-498f3b47004f@I-love.SAKURA.ne.jp ).

Please show us your patch that solves your issue.

RE: [PATCH v4] hfs: update sanity check of the root record

[Viacheslav Dubeyko]

On Sat, 2025-08-02 at 06:52 +0900, Tetsuo Handa wrote:
> On 2025/08/02 3:26, Viacheslav Dubeyko wrote:
> > On Fri, 2025-08-01 at 06:12 +0900, Tetsuo Handa wrote:
> > > On 2025/08/01 3:03, Viacheslav Dubeyko wrote:
> > > > On Thu, 2025-07-31 at 07:02 +0900, Tetsuo Handa wrote:
> > > > > On 2025/07/31 4:24, Viacheslav Dubeyko wrote:
> > > > > > If we considering case HFS_CDR_DIR in hfs_read_inode(), then we know that it
> > > > > > could be HFS_POR_CNID, HFS_ROOT_CNID, or >= HFS_FIRSTUSER_CNID. Do you mean that
> > > > > > HFS_POR_CNID could be a problem in hfs_write_inode()?
> > > > > 
> > > > > Yes. Passing one of 1, 5 or 15 instead of 2 from hfs_fill_super() triggers BUG()
> > > > > in hfs_write_inode(). We *MUST* validate at hfs_fill_super(), or hfs_read_inode()
> > > > > shall have to also reject 1, 5 and 15 (and as a result only accept 2).
> > > > 
> > > > The fix should be in hfs_read_inode(). Currently, suggested solution hides the
> > > > issue but not fix the problem.
> > > 
> > > Not fixing this problem might be hiding other issues, by hitting BUG() before
> > > other issues shows up.
> > > 
> > 
> > I am not going to start a philosophical discussion. We simply need to fix the
> > bug. The suggested patch doesn't fix the issue.
> 
> What is your issue?
> 
> My issue (what syzbot is reporting) is that the kernel crashes if the inode number
> of the record retrieved as a result of hfs_cat_find_brec(HFS_ROOT_CNID) is not
> HFS_ROOT_CNID. My suggested patch does fix my issue.
> 
> > Please, don't use hardcoded value. I already shared the point that we must use
> > the declared constants.
> > 
> > This function is incorrect and it cannot work for folders and files at the same
> > time.
> 
> I already shared that I don't plan to try writing such function
> ( http://lkml.kernel.org/r/38d8f48e-47c3-4d67-9caa-498f3b47004f@I-love.SAKURA.ne.jp   ).
> 
> Please show us your patch that solves your issue.

OK. It will be faster to write my own patch. It works for me.

Thanks,
Slava.

Re: [PATCH v4] hfs: update sanity check of the root record

[Tetsuo Handa]

On 2025/08/05 7:00, Viacheslav Dubeyko wrote:
>> Please show us your patch that solves your issue.
> 
> OK. It will be faster to write my own patch. It works for me.

I haven't heard from you about your own patch.

I guess that your patch will include

diff --git a/fs/hfs/inode.c b/fs/hfs/inode.c
index bf4cb7e78396..8d033ffeb8af 100644
--- a/fs/hfs/inode.c
+++ b/fs/hfs/inode.c
@@ -361,6 +361,10 @@ static int hfs_read_inode(struct inode *inode, void *data)
 		break;
 	case HFS_CDR_DIR:
 		inode->i_ino = be32_to_cpu(rec->dir.DirID);
+		if (inode->i_ino < HFS_FIRSTUSER_CNID && inode->i_ino != HFS_ROOT_CNID) {
+			make_bad_inode(inode);
+			break;
+		}
 		inode->i_size = be16_to_cpu(rec->dir.Val) + 2;
 		HFS_I(inode)->fs_blocks = 0;
 		inode->i_mode = S_IFDIR | (S_IRWXUGO & ~hsb->s_dir_umask);

change, which results in the following.

----------
The root inode's i_ino is 0 or 1 = fail with EINVAL
The root inode's i_ino is 2 = success
The root inode's i_ino is 3 or 4 = fail with ENOTDIR
The root inode's i_ino is 5 to 15 = fail with EINVAL
The root inode's i_ino is 16 and beyond = success
----------

But my patch has extra validation on the root inode's i_ino,
which results in the following.

----------
The root inode's i_ino is 2 = success
The root inode's i_ino is all (i.e. including 16 and beyond) but 2 = fail with EIO
----------

Therefore, while you can propose your patch,
I consider that there is no reason to defer my patch.