All of lore.kernel.org
 help / color / mirror / Atom feed
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?


  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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.