From: Al Viro <viro@zeniv.linux.org.uk>
To: Mehdi Ben Hadj Khelifa <mehdi.benhadjkhelifa@gmail.com>
Cc: brauner@kernel.org, jack@suse.cz,
syzbot+ad45f827c88778ff7df6@syzkaller.appspotmail.com,
frank.li@vivo.com, glaubitz@physik.fu-berlin.de,
linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
slava@dubeyko.com, syzkaller-bugs@googlegroups.com,
skhan@linuxfoundation.org, david.hunter.linux@gmail.com,
khalid@kernel.org,
linux-kernel-mentees@lists.linuxfoundation.org
Subject: Re: [PATCH] fs/super: fix memory leak of s_fs_info on setup_bdev_super failure
Date: Tue, 18 Nov 2025 16:35:09 +0000 [thread overview]
Message-ID: <20251118163509.GE2441659@ZenIV> (raw)
In-Reply-To: <6c482108-78b8-4e09-814a-67820a5c021e@gmail.com>
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?
next prev parent reply other threads:[~2025-11-18 16:35 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-11-14 16:52 [PATCH] fs/super: fix memory leak of s_fs_info on setup_bdev_super failure Mehdi Ben Hadj Khelifa
2025-11-18 14:59 ` Al Viro
2025-11-18 16:21 ` Mehdi Ben Hadj Khelifa
2025-11-18 16:35 ` Al Viro [this message]
2025-11-18 16:55 ` Al Viro
2025-11-18 18:05 ` Mehdi Ben Hadj Khelifa
2025-11-18 17:58 ` Mehdi Ben Hadj Khelifa
2025-11-26 14:01 ` kernel test robot
-- strict thread matches above, loose matches on Subject: below --
2025-11-13 4:27 [syzbot] [hfs?] memory leak in hfs_init_fs_context syzbot
2025-11-14 5:12 ` [PATCH] fs/super: fix memory leak of s_fs_info on setup_bdev_super failure Mehdi Ben Hadj Khelifa
2025-11-14 11:55 ` Christian Brauner
2025-11-14 16:05 ` Mehdi Ben Hadj Khelifa
2025-11-14 17:15 ` Mehdi Ben Hadj Khelifa
2025-11-19 13:43 ` Christian Brauner
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20251118163509.GE2441659@ZenIV \
--to=viro@zeniv.linux.org.uk \
--cc=brauner@kernel.org \
--cc=david.hunter.linux@gmail.com \
--cc=frank.li@vivo.com \
--cc=glaubitz@physik.fu-berlin.de \
--cc=jack@suse.cz \
--cc=khalid@kernel.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel-mentees@lists.linuxfoundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mehdi.benhadjkhelifa@gmail.com \
--cc=skhan@linuxfoundation.org \
--cc=slava@dubeyko.com \
--cc=syzbot+ad45f827c88778ff7df6@syzkaller.appspotmail.com \
--cc=syzkaller-bugs@googlegroups.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).