linux-kernel - Re: [PATCH] fs/super: fix memory leak of s_fs_info on setup_bdev

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20251118163509.GE2441659@ZenIV>
Date: Tue, 18 Nov 2025 16:35:09 +0000
From: Al Viro <viro@...iv.linux.org.uk>
To: Mehdi Ben Hadj Khelifa <mehdi.benhadjkhelifa@...il.com>
Cc: brauner@...nel.org, jack@...e.cz,
	syzbot+ad45f827c88778ff7df6@...kaller.appspotmail.com,
	frank.li@...o.com, glaubitz@...sik.fu-berlin.de,
	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
	slava@...eyko.com, syzkaller-bugs@...glegroups.com,
	skhan@...uxfoundation.org, david.hunter.linux@...il.com,
	khalid@...nel.org, linux-kernel-mentees@...ts.linuxfoundation.org
Subject: Re: [PATCH] fs/super: fix memory leak of s_fs_info on
 setup_bdev_super failure

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

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

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.

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.

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?