linux-kernel - Re: [PATCH] hfs: Replace BUG_ON with error handling in hfs_new

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <1287044022.2349785.1763841059344@kpc.webmail.kpnmail.nl>
Date: Sat, 22 Nov 2025 20:50:59 +0100 (CET)
From: Jori Koolstra <jkoolstra@...all.nl>
To: Viacheslav Dubeyko <slava@...eyko.com>,
	John Paul Adrian Glaubitz <glaubitz@...sik.fu-berlin.de>,
	Yangtao Li <frank.li@...o.com>,
	"skhan@...uxfoundation.org" <skhan@...uxfoundation.org>,
	"david.shane.hunter@...il.com" <david.shane.hunter@...il.com>
Cc: linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
	syzbot+17cc9bb6d8d69b4139f0@...kaller.appspotmail.com
Subject: Re: [PATCH] hfs: Replace BUG_ON with error handling in
 hfs_new_inode()


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