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/27/25 6:45 PM, David Hunter wrote:
> On 11/25/25 17:14, Mehdi Ben Hadj Khelifa wrote:
>> 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/
>>
>>
>>
>>
>>
>>
> 
> Hey everyone,
> 
> I am helping Shuah with the Linux Kernel Mentorship Program. I wanted to
> report that many mentees have also had problems with xfstests recently.
> 
> I am tracking what happens here, so I can help future developers, but I
> just wanted to let everyone know that others are also having issues with
> xfstests.
> 
> Also, Mehdi, by any chance have you used any of the configuration
> targets like "localmodconfig". I am wondering if something in your
> configuration file is missing.
> 
No I have not,Thanks for the suggestion.I have been messing with a lot 
of syzbot configurations but IIRC I always have made sure to make 
mrproper and copy my in use .config file before building for my pc.I 
still want to reproduce the same crash as I did before on my part now to 
see what caused the problem anyway but my assumption for the time being 
is that localmodconfig would fix it.

I will keep you updated on the matter too David.Thanks

> Thanks,
> David Hunter
Best Regards,
Mehdi Ben Hadj khelifa