Re: [PATCH v2] fs/hfs: fix s_fs_info leak on setup_bdev_super() failure

[Mehdi Ben Hadj Khelifa <mehdi.benhadjkhelifa@gmail.com>]

On 11/22/25 12:01 AM, Viacheslav Dubeyko wrote:
> On Sat, 2025-11-22 at 00:36 +0100, Mehdi Ben Hadj Khelifa wrote:
>> On 11/21/25 11:28 PM, Viacheslav Dubeyko wrote:
>>> On Sat, 2025-11-22 at 00:16 +0100, Mehdi Ben Hadj Khelifa wrote:
>>>> On 11/21/25 11:04 PM, Viacheslav Dubeyko wrote:
>>>>> On Fri, 2025-11-21 at 23:48 +0100, Mehdi Ben Hadj Khelifa wrote:
>>>>>> On 11/21/25 10:15 PM, Viacheslav Dubeyko wrote:
>>>>>>> On Fri, 2025-11-21 at 20:44 +0100, Mehdi Ben Hadj Khelifa wrote:
>>>>>>>> On 11/19/25 8:58 PM, Viacheslav Dubeyko wrote:
>>>>>>>>> On Wed, 2025-11-19 at 08:38 +0100, Mehdi Ben Hadj Khelifa wrote:
>>>>>>>>>>
> 
> <skipped>
> 
>>>>>>>
>>>>>> IIUC, hfs_mdb_put() isn't called in the case of hfs_kill_super() in
>>>>>> christian's patch because fill_super() (for the each specific
>>>>>> filesystem) is responsible for cleaning up the superblock in case of
>>>>>> failure and you can reference christian's patch[1] which he explained
>>>>>> the reasoning for here[2].And in the error path the we are trying to
>>>>>> fix, fill_super() isn't even called yet. So such pointers shouldn't be
>>>>>> pointing to anything allocated yet hence only freeing the pointer to the
>>>>>> sb_info here is sufficient I think.
>>>
>>> I was confused that your code with hfs_mdb_put() is still in this email. So,
>>> yes, hfs_fill_super()/hfsplus_fill_super() try to free the memory in the case of
>>> failure. It means that if something wasn't been freed, then it will be issue in
>>> these methods. Then, I don't see what should else need to be added here. Some
>>> file systems do sb->s_fs_info = NULL. But absence of this statement is not
>>> critical, from my point of view.
>>>
>> Thanks for the input. I will be sending the same mentionned patch after
>> doing testing for it and also after finishing my testing for the hfs
>> patch too.
>>>
> 
> I am guessing... Should we consider to introduce some xfstest, self-test, or
> unit-test to detect this issue in all Linux's file systems family?
> 
Yes, It isn't that hard either IIUC you just need to fail the 
bdev_file_open_by_dev() function somehow to trigger this error path..
> Thanks,
> Slava.

So I wanted to update you on my testing for the hfs patch and the 
hfsplus patch. For the testing I used both my desktop pc and my laptop 
pc running the same configuraitons and the same linux distribution to 
have more accurate testing. There are three variants that I used for 
testing : A stable kernel, 6.18-rc7 kernel with no patch, 6.18-rc7 
kernel with hfs or hfsplus patch.

Firstly, I couldn't run the hfs tests due to mkfs.hfs being unavailable 
in my search for it. they all point to mkfs.hfsplus and you pointed out 
that mkfs.hfsplus can create hfs filesystems with the -h flag but in my 
case it doesn't. I pointed out last time that I found a tool to create 
HFS filesystems which it does (it's called hformat) but the xfstests 
require the availability of mkfs.hfs and fsck.hfs for them to run. More 
help on this is needed for me to run hfs tests. I also tested ext4 as 
you have suggested as a base to compare to. Here is my summary of testing:

For Stable kernel 6.17.8:

On desktop:
ext4 tests ran successfully.
hfsplus tests crash the pc around generic 631 test.

On Laptop:
ext4 and hfsplus tests ran successfully.

For 6.18-rc7 kernel:

On desktop:
ext4 tests ran successfully same results as the stable kernel.
hfsplus crashes on testing startup.For launching any test.

On Laptop:
ext4 tests ran successfully same results as the stable kernel.
hfsplus crashes on testing startup.For launcing any test.


For the patched 6.18-rc7 kernel.

Same results for both desktop and laptop pcs as in the 6.18-rc7 kernel.


Should be noted that I have tried many different setups regarding the 
devices and their creation for the 6.18-rc7 kernel and none of them 
worked.Still I can't deduce what is causing the issue.If they work for 
you, my only assumption is that some dependency of xfstests is not met 
on my part even though I made sure that I do cover them all especially 
with repeatedly failed testing...

What could be the issue here on my end if you have any idea?

Also should I send you the hfsplus patch in one of my earlier replies[1] 
for you to test too and maybe add it to hfsplus?

Best Regards,
Mehdi Ben Hadj Khelifa

[1]:https://lore.kernel.org/all/3ad2e91e-2c7f-488b-a119-51d62a6e95b8@gmail.com/