[PATCH] Replace BUG_ON with error handling in hfs_new_inode()

[PATCH] Replace BUG_ON with error handling in hfs_new_inode()

[Jori Koolstra]

In a06ec283e125 next_id, folder_count, and file_count in the super block
info were expanded to 64 bits, and BUG_ONs were added to detect
overflow. This triggered an error reported by syzbot: if the MDB is
corrupted, the BUG_ON is triggered. This patch replaces this mechanism
with proper error handling and resolves the syzbot reported bug.

hfs_new_inode() is the only place were the 32-bit limits need to be
verified, since only in that function can these values be increased.
Therefore, the checks in hfs_mdb_commit() and hfs_delete_inode() are
removed.

Singed-off-by: Jori Koolstra <jkoolstra@xs4all.nl>
Reported-by: syzbot+17cc9bb6d8d69b4139f0@syzkaller.appspotmail.com
Closes: https://syzbot.org/bug?extid=17cc9bb6d8d69b4139f0
Signed-off-by: Jori Koolstra <jkoolstra@xs4all.nl>
---
 fs/hfs/dir.c   |  8 ++++----
 fs/hfs/inode.c | 30 ++++++++++++++++++++++++------
 fs/hfs/mdb.c   |  3 ---
 3 files changed, 28 insertions(+), 13 deletions(-)

diff --git a/fs/hfs/dir.c b/fs/hfs/dir.c
index 86a6b317b474..ee1760305380 100644
--- a/fs/hfs/dir.c
+++ b/fs/hfs/dir.c
@@ -196,8 +196,8 @@ static int hfs_create(struct mnt_idmap *idmap, struct inode *dir,
 	int res;
 
 	inode = hfs_new_inode(dir, &dentry->d_name, mode);
-	if (!inode)
-		return -ENOMEM;
+	if (IS_ERR(inode))
+		return PTR_ERR(inode);
 
 	res = hfs_cat_create(inode->i_ino, dir, &dentry->d_name, inode);
 	if (res) {
@@ -226,8 +226,8 @@ static struct dentry *hfs_mkdir(struct mnt_idmap *idmap, struct inode *dir,
 	int res;
 
 	inode = hfs_new_inode(dir, &dentry->d_name, S_IFDIR | mode);
-	if (!inode)
-		return ERR_PTR(-ENOMEM);
+	if (IS_ERR(inode))
+		return ERR_CAST(inode);
 
 	res = hfs_cat_create(inode->i_ino, dir, &dentry->d_name, inode);
 	if (res) {
diff --git a/fs/hfs/inode.c b/fs/hfs/inode.c
index 9cd449913dc8..beec6fe7e801 100644
--- a/fs/hfs/inode.c
+++ b/fs/hfs/inode.c
@@ -186,16 +186,23 @@ struct inode *hfs_new_inode(struct inode *dir, const struct qstr *name, umode_t
 	s64 next_id;
 	s64 file_count;
 	s64 folder_count;
+	int err = -ENOMEM;
 
 	if (!inode)
-		return NULL;
+		goto out_err;
+
+	err = -ENOSPC;
 
 	mutex_init(&HFS_I(inode)->extents_lock);
 	INIT_LIST_HEAD(&HFS_I(inode)->open_dir_list);
 	spin_lock_init(&HFS_I(inode)->open_dir_lock);
 	hfs_cat_build_key(sb, (btree_key *)&HFS_I(inode)->cat_key, dir->i_ino, name);
 	next_id = atomic64_inc_return(&HFS_SB(sb)->next_id);
-	BUG_ON(next_id > U32_MAX);
+	if (next_id > U32_MAX) {
+		pr_err("hfs: next file ID exceeds 32-bit limit — possible "
+		       "superblock corruption");
+		goto out_discard;
+	}
 	inode->i_ino = (u32)next_id;
 	inode->i_mode = mode;
 	inode->i_uid = current_fsuid();
@@ -209,7 +216,11 @@ struct inode *hfs_new_inode(struct inode *dir, const struct qstr *name, umode_t
 	if (S_ISDIR(mode)) {
 		inode->i_size = 2;
 		folder_count = atomic64_inc_return(&HFS_SB(sb)->folder_count);
-		BUG_ON(folder_count > U32_MAX);
+		if (folder_count > U32_MAX) {
+			pr_err("hfs: folder count exceeds 32-bit limit — possible "
+			       "superblock corruption");
+			goto out_discard;
+		}
 		if (dir->i_ino == HFS_ROOT_CNID)
 			HFS_SB(sb)->root_dirs++;
 		inode->i_op = &hfs_dir_inode_operations;
@@ -219,7 +230,11 @@ struct inode *hfs_new_inode(struct inode *dir, const struct qstr *name, umode_t
 	} else if (S_ISREG(mode)) {
 		HFS_I(inode)->clump_blocks = HFS_SB(sb)->clumpablks;
 		file_count = atomic64_inc_return(&HFS_SB(sb)->file_count);
-		BUG_ON(file_count > U32_MAX);
+		if (file_count > U32_MAX) {
+			pr_err("hfs: file count exceeds 32-bit limit — possible "
+			       "superblock corruption");
+			goto out_discard;
+		}
 		if (dir->i_ino == HFS_ROOT_CNID)
 			HFS_SB(sb)->root_files++;
 		inode->i_op = &hfs_file_inode_operations;
@@ -243,6 +258,11 @@ struct inode *hfs_new_inode(struct inode *dir, const struct qstr *name, umode_t
 	hfs_mark_mdb_dirty(sb);
 
 	return inode;
+
+	out_discard:
+		iput(inode);	
+	out_err:
+		return ERR_PTR(err); 
 }
 
 void hfs_delete_inode(struct inode *inode)
@@ -251,7 +271,6 @@ void hfs_delete_inode(struct inode *inode)
 
 	hfs_dbg("ino %lu\n", inode->i_ino);
 	if (S_ISDIR(inode->i_mode)) {
-		BUG_ON(atomic64_read(&HFS_SB(sb)->folder_count) > U32_MAX);
 		atomic64_dec(&HFS_SB(sb)->folder_count);
 		if (HFS_I(inode)->cat_key.ParID == cpu_to_be32(HFS_ROOT_CNID))
 			HFS_SB(sb)->root_dirs--;
@@ -260,7 +279,6 @@ void hfs_delete_inode(struct inode *inode)
 		return;
 	}
 
-	BUG_ON(atomic64_read(&HFS_SB(sb)->file_count) > U32_MAX);
 	atomic64_dec(&HFS_SB(sb)->file_count);
 	if (HFS_I(inode)->cat_key.ParID == cpu_to_be32(HFS_ROOT_CNID))
 		HFS_SB(sb)->root_files--;
diff --git a/fs/hfs/mdb.c b/fs/hfs/mdb.c
index 53f3fae60217..1c3fb631cc8e 100644
--- a/fs/hfs/mdb.c
+++ b/fs/hfs/mdb.c
@@ -273,15 +273,12 @@ void hfs_mdb_commit(struct super_block *sb)
 		/* These parameters may have been modified, so write them back */
 		mdb->drLsMod = hfs_mtime();
 		mdb->drFreeBks = cpu_to_be16(HFS_SB(sb)->free_ablocks);
-		BUG_ON(atomic64_read(&HFS_SB(sb)->next_id) > U32_MAX);
 		mdb->drNxtCNID =
 			cpu_to_be32((u32)atomic64_read(&HFS_SB(sb)->next_id));
 		mdb->drNmFls = cpu_to_be16(HFS_SB(sb)->root_files);
 		mdb->drNmRtDirs = cpu_to_be16(HFS_SB(sb)->root_dirs);
-		BUG_ON(atomic64_read(&HFS_SB(sb)->file_count) > U32_MAX);
 		mdb->drFilCnt =
 			cpu_to_be32((u32)atomic64_read(&HFS_SB(sb)->file_count));
-		BUG_ON(atomic64_read(&HFS_SB(sb)->folder_count) > U32_MAX);
 		mdb->drDirCnt =
 			cpu_to_be32((u32)atomic64_read(&HFS_SB(sb)->folder_count));
 
-- 
2.51.1.dirty

Re: [PATCH] Replace BUG_ON with error handling in hfs_new_inode()

[Viacheslav Dubeyko]

On Mon, 2025-11-03 at 14:10 +0100, Jori Koolstra wrote:
> In a06ec283e125 next_id, folder_count, and file_count in the super
> block
> info were expanded to 64 bits, and BUG_ONs were added to detect
> overflow. This triggered an error reported by syzbot: if the MDB is
> corrupted, the BUG_ON is triggered. This patch replaces this
> mechanism
> with proper error handling and resolves the syzbot reported bug.
> 
> hfs_new_inode() is the only place were the 32-bit limits need to be
> verified, since only in that function can these values be increased.
> Therefore, the checks in hfs_mdb_commit() and hfs_delete_inode() are
> removed.
> 

I am terribly sorry, I've missed the patch. But, please, please,
please, add prefix 'hfs:' to the topic. This is the reason why I've
missed the patch. I expected to see something like this:

hfs: Replace BUG_ON with error handling in hfs_new_inode()

I need to process dozens emails every day. So, if I don't see proper
keyword in the topic, then I skip the emails.


> Singed-off-by: Jori Koolstra <jkoolstra@xs4all.nl>
> Reported-by: syzbot+17cc9bb6d8d69b4139f0@syzkaller.appspotmail.com
> Closes: https://syzbot.org/bug?extid=17cc9bb6d8d69b4139f0
> Signed-off-by: Jori Koolstra <jkoolstra@xs4all.nl>
> ---
>  fs/hfs/dir.c   |  8 ++++----
>  fs/hfs/inode.c | 30 ++++++++++++++++++++++++------
>  fs/hfs/mdb.c   |  3 ---
>  3 files changed, 28 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/hfs/dir.c b/fs/hfs/dir.c
> index 86a6b317b474..ee1760305380 100644
> --- a/fs/hfs/dir.c
> +++ b/fs/hfs/dir.c
> @@ -196,8 +196,8 @@ static int hfs_create(struct mnt_idmap *idmap,
> struct inode *dir,
>  	int res;
>  
>  	inode = hfs_new_inode(dir, &dentry->d_name, mode);
> -	if (!inode)
> -		return -ENOMEM;

I don't think that this removal is correct. The hfs_new_inode() can
return NULL [1].

> +	if (IS_ERR(inode))
> +		return PTR_ERR(inode);
>  
>  	res = hfs_cat_create(inode->i_ino, dir, &dentry->d_name,
> inode);
>  	if (res) {
> @@ -226,8 +226,8 @@ static struct dentry *hfs_mkdir(struct mnt_idmap
> *idmap, struct inode *dir,
>  	int res;
>  
>  	inode = hfs_new_inode(dir, &dentry->d_name, S_IFDIR | mode);
> -	if (!inode)
> -		return ERR_PTR(-ENOMEM);

Ditto. Please, take a look here [1].

> +	if (IS_ERR(inode))
> +		return ERR_CAST(inode);
>  
>  	res = hfs_cat_create(inode->i_ino, dir, &dentry->d_name,
> inode);
>  	if (res) {
> diff --git a/fs/hfs/inode.c b/fs/hfs/inode.c
> index 9cd449913dc8..beec6fe7e801 100644
> --- a/fs/hfs/inode.c
> +++ b/fs/hfs/inode.c
> @@ -186,16 +186,23 @@ struct inode *hfs_new_inode(struct inode *dir,
> const struct qstr *name, umode_t
>  	s64 next_id;
>  	s64 file_count;
>  	s64 folder_count;
> +	int err = -ENOMEM;
>  
>  	if (!inode)
> -		return NULL;
> +		goto out_err;

OK. I see. You have modified the hfs_new_inode() with the goal to
return error code instead of NULL.

Frankly speaking, I am not sure that inode is NULL, then it means
always that we are out of memory (-ENOMEM).

> +
> +	err = -ENOSPC;

Why do we use -ENOSPC here? If next_id > U32_MAX, then it doesn't mean
that volume is full. Probably, we have corrupted volume, then code
error should be completely different (maybe, -EIO).

>  
>  	mutex_init(&HFS_I(inode)->extents_lock);
>  	INIT_LIST_HEAD(&HFS_I(inode)->open_dir_list);
>  	spin_lock_init(&HFS_I(inode)->open_dir_lock);
>  	hfs_cat_build_key(sb, (btree_key *)&HFS_I(inode)->cat_key,
> dir->i_ino, name);
>  	next_id = atomic64_inc_return(&HFS_SB(sb)->next_id);
> -	BUG_ON(next_id > U32_MAX);
> +	if (next_id > U32_MAX) {
> +		pr_err("hfs: next file ID exceeds 32-bit limit —
> possible "
> +		       "superblock corruption");

The 'hfs:' prefix is not necessary here. It could be not only file but
folder ID too. So, maybe, it makes sense to mention "next CNID". The
whole comment needs to be on one line. Also, I believe it makes sense
to recommend run FSCK tool here.

> +		goto out_discard;
> +	}
>  	inode->i_ino = (u32)next_id;
>  	inode->i_mode = mode;
>  	inode->i_uid = current_fsuid();
> @@ -209,7 +216,11 @@ struct inode *hfs_new_inode(struct inode *dir,
> const struct qstr *name, umode_t
>  	if (S_ISDIR(mode)) {
>  		inode->i_size = 2;
>  		folder_count = atomic64_inc_return(&HFS_SB(sb)-
> >folder_count);
> -		BUG_ON(folder_count > U32_MAX);
> +		if (folder_count > U32_MAX) {
> +			pr_err("hfs: folder count exceeds 32-bit
> limit — possible "
> +			       "superblock corruption");

Ditto.

> +			goto out_discard;
> +		}
>  		if (dir->i_ino == HFS_ROOT_CNID)
>  			HFS_SB(sb)->root_dirs++;
>  		inode->i_op = &hfs_dir_inode_operations;
> @@ -219,7 +230,11 @@ struct inode *hfs_new_inode(struct inode *dir,
> const struct qstr *name, umode_t
>  	} else if (S_ISREG(mode)) {
>  		HFS_I(inode)->clump_blocks = HFS_SB(sb)->clumpablks;
>  		file_count = atomic64_inc_return(&HFS_SB(sb)-
> >file_count);
> -		BUG_ON(file_count > U32_MAX);
> +		if (file_count > U32_MAX) {
> +			pr_err("hfs: file count exceeds 32-bit limit
> — possible "
> +			       "superblock corruption");

Ditto.

> +			goto out_discard;
> +		}
>  		if (dir->i_ino == HFS_ROOT_CNID)
>  			HFS_SB(sb)->root_files++;
>  		inode->i_op = &hfs_file_inode_operations;
> @@ -243,6 +258,11 @@ struct inode *hfs_new_inode(struct inode *dir,
> const struct qstr *name, umode_t
>  	hfs_mark_mdb_dirty(sb);
>  
>  	return inode;
> +
> +	out_discard:
> +		iput(inode);	
> +	out_err:
> +		return ERR_PTR(err); 
>  }
>  
>  void hfs_delete_inode(struct inode *inode)
> @@ -251,7 +271,6 @@ void hfs_delete_inode(struct inode *inode)
>  
>  	hfs_dbg("ino %lu\n", inode->i_ino);
>  	if (S_ISDIR(inode->i_mode)) {
> -		BUG_ON(atomic64_read(&HFS_SB(sb)->folder_count) >
> U32_MAX);

I don't agree with complete removal of this check. Because, we could
have bugs in file system logic that can increase folder_count
wrongfully above U32_MAX limit.

So, I prefer to have this check in some way. Error code sounds good.

> 
>  		atomic64_dec(&HFS_SB(sb)->folder_count);
>  		if (HFS_I(inode)->cat_key.ParID ==
> cpu_to_be32(HFS_ROOT_CNID))
>  			HFS_SB(sb)->root_dirs--;
> @@ -260,7 +279,6 @@ void hfs_delete_inode(struct inode *inode)
>  		return;
>  	}
>  
> -	BUG_ON(atomic64_read(&HFS_SB(sb)->file_count) > U32_MAX);

Ditto. I don't agree with complete removal of this check.

>  	atomic64_dec(&HFS_SB(sb)->file_count);
>  	if (HFS_I(inode)->cat_key.ParID ==
> cpu_to_be32(HFS_ROOT_CNID))
>  		HFS_SB(sb)->root_files--;
> diff --git a/fs/hfs/mdb.c b/fs/hfs/mdb.c
> index 53f3fae60217..1c3fb631cc8e 100644
> --- a/fs/hfs/mdb.c
> +++ b/fs/hfs/mdb.c
> @@ -273,15 +273,12 @@ void hfs_mdb_commit(struct super_block *sb)
>  		/* These parameters may have been modified, so write
> them back */
>  		mdb->drLsMod = hfs_mtime();
>  		mdb->drFreeBks = cpu_to_be16(HFS_SB(sb)-
> >free_ablocks);
> -		BUG_ON(atomic64_read(&HFS_SB(sb)->next_id) >
> U32_MAX);

Ditto. I don't agree with complete removal of this check.

>  		mdb->drNxtCNID =
>  			cpu_to_be32((u32)atomic64_read(&HFS_SB(sb)-
> >next_id));
>  		mdb->drNmFls = cpu_to_be16(HFS_SB(sb)->root_files);
>  		mdb->drNmRtDirs = cpu_to_be16(HFS_SB(sb)-
> >root_dirs);
> -		BUG_ON(atomic64_read(&HFS_SB(sb)->file_count) >
> U32_MAX);

Ditto. I don't agree with complete removal of this check.

>  		mdb->drFilCnt =
>  			cpu_to_be32((u32)atomic64_read(&HFS_SB(sb)-
> >file_count));
> -		BUG_ON(atomic64_read(&HFS_SB(sb)->folder_count) >
> U32_MAX);

Ditto. I don't agree with complete removal of this check.

>  		mdb->drDirCnt =
>  			cpu_to_be32((u32)atomic64_read(&HFS_SB(sb)-
> >folder_count));
>  

Thanks,
Slava.

[1]
https://elixir.bootlin.com/linux/v6.18-rc6/source/fs/hfs/inode.c#L190

Re: [PATCH] hfs: Replace BUG_ON with error handling in hfs_new_inode()

[Jori Koolstra]

> 
> I am terribly sorry, I've missed the patch. But, please, please,
> please, add prefix 'hfs:' to the topic. This is the reason why I've
> missed the patch. I expected to see something like this:
> 
> hfs: Replace BUG_ON with error handling in hfs_new_inode()
> 
> I need to process dozens emails every day. So, if I don't see proper
> keyword in the topic, then I skip the emails.

My bad, I didn't know this convention. Is this LKML-wide? Because I have
also been waiting for a few weeks on feedback on jfs patches. Normally,
it would not matter, but I am in the Linux Foundation Kernel Mentorship
Program and we need to get several patches in before the deadline to
succeed :)

> OK. I see. You have modified the hfs_new_inode() with the goal to
> return error code instead of NULL.
> 
> Frankly speaking, I am not sure that inode is NULL, then it means
> always that we are out of memory (-ENOMEM).
> 

I think this is correct. See for instance fs/ext4/ialloc.c at __ext4_new_inode.

> 
> Why do we use -ENOSPC here? If next_id > U32_MAX, then it doesn't mean
> that volume is full. Probably, we have corrupted volume, then code
> error should be completely different (maybe, -EIO).
>

ext4 uses EFSCORRUPTED which is defined as EUCLEAN, I can change that.

I wasn't exactly sure of the limits in place in hfs. But looking more closely,
there can only be 65,535 allocation blocks, and I think you need at least one
per inode. But then why are the CNID, max files, max directories 32-bit values
in the MBD? What limits indicate corruption?


> The 'hfs:' prefix is not necessary here. It could be not only file but
> folder ID too. So, maybe, it makes sense to mention "next CNID". The
> whole comment needs to be on one line. Also, I believe it makes sense
> to recommend run FSCK tool here.
> 

Will fix this.

> >  
> >  void hfs_delete_inode(struct inode *inode)
> > @@ -251,7 +271,6 @@ void hfs_delete_inode(struct inode *inode)
> >  
> >  	hfs_dbg("ino %lu\n", inode->i_ino);
> >  	if (S_ISDIR(inode->i_mode)) {
> > -		BUG_ON(atomic64_read(&HFS_SB(sb)->folder_count) >
> > U32_MAX);
> 
> I don't agree with complete removal of this check. Because, we could
> have bugs in file system logic that can increase folder_count
> wrongfully above U32_MAX limit.
> 
> So, I prefer to have this check in some way. Error code sounds good.
> 

Will add these back with error handling instead of BUG_ON.

> 
> Thanks,
> Slava.
> 

Thank-you for the extended response!

Jori.

RE: [PATCH] hfs: Replace BUG_ON with error handling in hfs_new_inode()

[Viacheslav Dubeyko]

On Sat, 2025-11-22 at 20:50 +0100, Jori Koolstra wrote:
> > 
> > I am terribly sorry, I've missed the patch. But, please, please,
> > please, add prefix 'hfs:' to the topic. This is the reason why I've
> > missed the patch. I expected to see something like this:
> > 
> > hfs: Replace BUG_ON with error handling in hfs_new_inode()
> > 
> > I need to process dozens emails every day. So, if I don't see proper
> > keyword in the topic, then I skip the emails.
> 
> My bad, I didn't know this convention. Is this LKML-wide? Because I have
> also been waiting for a few weeks on feedback on jfs patches. Normally,
> it would not matter, but I am in the Linux Foundation Kernel Mentorship
> Program and we need to get several patches in before the deadline to
> succeed :)
> 

You can take a look here [1]. Usually, yes, everybody expects kernel subsystem's
name as the prefix:

Subject: [PATCH 001/123] subsystem: summary phrase

> > OK. I see. You have modified the hfs_new_inode() with the goal to
> > return error code instead of NULL.
> > 
> > Frankly speaking, I am not sure that inode is NULL, then it means
> > always that we are out of memory (-ENOMEM).
> > 
> 
> I think this is correct. See for instance fs/ext4/ialloc.c at __ext4_new_inode.
> 

Probably, you are right. I simply took a look into alloc_inode():

struct inode *alloc_inode(struct super_block *sb)
{
	const struct super_operations *ops = sb->s_op;
	struct inode *inode;

	if (ops->alloc_inode)
		inode = ops->alloc_inode(sb);
	else
		inode = alloc_inode_sb(sb, inode_cachep, GFP_KERNEL);

	if (!inode)
		return NULL;

	if (unlikely(inode_init_always(sb, inode))) {
		if (ops->destroy_inode) {
			ops->destroy_inode(inode);
			if (!ops->free_inode)
				return NULL;
		}
		inode->free_inode = ops->free_inode;
		i_callback(&inode->i_rcu);
		return NULL;
	}

	return inode;
}

Second part of the method could return NULL even if inode has been allocated.
But we can consider -ENOMEM in hfs_new_inode(). I am OK with that.

Thanks,
Slava.

> > 
> > Why do we use -ENOSPC here? If next_id > U32_MAX, then it doesn't mean
> > that volume is full. Probably, we have corrupted volume, then code
> > error should be completely different (maybe, -EIO).
> > 
> 
> ext4 uses EFSCORRUPTED which is defined as EUCLEAN, I can change that.
> 
> I wasn't exactly sure of the limits in place in hfs. But looking more closely,
> there can only be 65,535 allocation blocks, and I think you need at least one
> per inode. But then why are the CNID, max files, max directories 32-bit values
> in the MBD? What limits indicate corruption?
> 
> 
> > The 'hfs:' prefix is not necessary here. It could be not only file but
> > folder ID too. So, maybe, it makes sense to mention "next CNID". The
> > whole comment needs to be on one line. Also, I believe it makes sense
> > to recommend run FSCK tool here.
> > 
> 
> Will fix this.
> 
> > >  
> > >  void hfs_delete_inode(struct inode *inode)
> > > @@ -251,7 +271,6 @@ void hfs_delete_inode(struct inode *inode)
> > >  
> > >  	hfs_dbg("ino %lu\n", inode->i_ino);
> > >  	if (S_ISDIR(inode->i_mode)) {
> > > -		BUG_ON(atomic64_read(&HFS_SB(sb)->folder_count) >
> > > U32_MAX);
> > 
> > I don't agree with complete removal of this check. Because, we could
> > have bugs in file system logic that can increase folder_count
> > wrongfully above U32_MAX limit.
> > 
> > So, I prefer to have this check in some way. Error code sounds good.
> > 
> 
> Will add these back with error handling instead of BUG_ON.
> 
> 
> 

[1]
https://docs.kernel.org/process/submitting-patches.html#the-canonical-patch-format