Re: [PATCH] fs/super: fix memory leak of s_fs_info on setup_bdev_super failure

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

On 11/18/25 5:35 PM, Al Viro wrote:
> On Tue, Nov 18, 2025 at 05:21:59PM +0100, Mehdi Ben Hadj Khelifa wrote:
> 
>>> Almost certainly bogus; quite a few fill_super() callbacks seriously count
>>> upon "->kill_sb() will take care care of cleanup if we return an error".
>>
>> So should I then free the allocated s_fs_info in the kill_block_super
>> instead and check for the null pointer in put_fs_context to not execute
>> kfree in subsequent call to hfs_free_fc()?
> 
> Huh?  How the hell would kill_block_super() know what to do with ->s_fs_info
> for that particular fs type?  kill_block_super() is a convenience helper,
> no more than that...
> 
Yes, I missed that. Since i only looked at the hfs_free_fc(), I forgot 
that in kill_block_super() it handles all fs types not only hfs which 
only frees s_fs_info.

>> Because the error generated in setup_bdev_super() when returned to
>> do_new_mount() (after a lot of error propagation) it doesn't get handled:	
>> 	if (!err)
>> 		err = do_new_mount_fc(fc, path, mnt_flags);
>> 	put_fs_context(fc);
>> 	return err;
> 
> Would be hard to handle something that is already gone, wouldn't it?
> deactivate_locked_super() after the fill_super() failure is where
> the superblock is destroyed - nothing past that point could possibly
> be of any use.
> 
> I would still like the details on the problem you are seeing.

The Problem isn't produced by fill_super failure, instead it's produced 
by setup_bdev_super failure just a line before it. here is a snip from 
fs/super:

		error = setup_bdev_super(s, fc->sb_flags, fc);
		if (!error)
			error = fill_super(s, fc);
		if (error) {
			deactivate_locked_super(s);
			return error;
		}
and in the above code, fc->s_fs_info has already been transferred to sb 
as you have mentionned in the sget_fc() function before the above snip.
But subsequent calls after setup_bdev_super fail to free s_fs_info IIUC.


> 
> Normal operation (for filesystems that preallocate ->s_fs_info and hang
> it off fc) goes like this:
> 
> 	* fc->s_fs_info is allocated in ->init_fs_context()
> 	* it is modified (possibly) in ->parse_param()
> 	* eventually ->get_tree() is called and at some point it
> asks for superblock by calling sget_fc().  It may fail (in which
> case fc->s_fs_info stays where it is), if may return a preexisting
> superblock (ditto) *OR* it may create and return a new superblock.
> In that case fc->s_fs_info is no more - it's been moved over to
> sb->s_fs_info.  NULL is left behind.  From that point on the
> responsibility for that sucker is with the filesystem; nothing in
> VFS has any idea where to find it.
> 
In this case, it doesn create a new superblock which transferes the 
ownership of the pointer. But as i said the problem is that in the error 
path of setup_bdev_super(), there is no freeing of such memory and since 
the pointer has already been transfered and it's the responsibility is 
with the filesystem, put_fs_context() calling hfs_free_fc() doesn't free 
the allocated memory too.
> Again, there is no such thing as transferring it back to fc - once
> fill_super() has been called, there might be any number of additional
> things that need to be undone.
> 
As I said above, fill_super isn't even called in this case.
> For HFS I would expect that hfs_fill_super() would call hfs_mdb_put(sb)
> on all failures and have it called from subsequent ->put_super() if
> we succeed and later unmount the filesystem.  That seems to be where
> ->s_fs_info is taken out of superblock and freed.
> 
> What do you observe getting leaked and in which case does that happen?
> 
Exactly in bdev_file_open_by_dev() in the setup_bdev_super call 
mentionned above is what triggers the error path that doesn't free the 
hfs_sb_info since hfs_free_fc calls kfree on a NULL pointer..