linux-kernel - Re: [PATCH] adfs: fix memory leak in sb->s_fs

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: <CAHxJ8O_3CG9vmQcbF7qkG-CXB0423ark2bfYrKvt-HfLYD68BQ@mail.gmail.com>
Date: Sun, 14 Dec 2025 05:58:19 +0300
From: Ahmet Eray Karadag <eraykrdg1@...il.com>
To: Al Viro <viro@...iv.linux.org.uk>
Cc: akpm@...ux-foundation.org, linux-kernel@...r.kernel.org, 
	linux-fsdevel@...r.kernel.org, skhan@...uxfoundation.org, 
	david.hunter.linux@...il.com, 
	syzbot+1c70732df5fd4f0e4fbb@...kaller.appspotmail.com
Subject: Re: [PATCH] adfs: fix memory leak in sb->s_fs_info

Hi Al,

I apologize for overlooking the double-free scenario in the success path.
I focused too narrowly on the failure case provided by the reproducer and
failed to verify the fix against the normal lifecycle.

As a newcomer to kernel development, I truly appreciate you taking the time
to guide me through this analysis. It is a valuable lesson for me on looking
at the broader lifecycle rather than just the immediate bug.

I will implement the changes you suggested in v2.

Thank you for your patience and the detailed explanation.

Best regards,
Ahmet Eray

On Sun, Dec 14, 2025 at 5:27 AM Al Viro <viro@...iv.linux.org.uk> wrote:
>
> On Sun, Dec 14, 2025 at 02:02:12AM +0000, Al Viro wrote:
>
> > IOW, there's our double-free.  For extra fun, it's not just kfree() + kfree(),
> > it's kfree_rcu() + kfree().
>
> [sorry, accidentally sent halfway through writing a reply; continued below]
>
> So after successful mount, it gets freed (RCU-delayed) from ->kill_sb() called
> at fs shutdown.
>
> On adfs_fill_super() failure (hit #2) it is freed on failure exit - with non-delayed
> kfree().
>
> In case we never got to superblock allocation, the thing gets freed by adfs_free_fc()
> (also non-delayed).
>
> The gap is between a successful call of sget_fc() and call of adfs_fill_super()
> (in get_tree_bdev(), which is where adfs_fill_super() is passed as a callback).
> If setup_bdev_super() fails, we will
>         * transfer it from fs_context to super_block, so the fs_context destruction
> won't have anything to free
>         * won't free it in never-called adfs_fill_super()
>         * won't free it in ->kill_sb(), since ->s_root remains NULL and ->put_super()
> is never called.
>
> A leak is real, IOW.
>
> Getting ->kill_sb() to do freeing unconditionally would cover the gap.  However,
> to do that, we need to _move_ freeing (RCU-delayed) from adfs_put_super() to
> adfs_kill_sb(), not just add kfree() in the latter.
>
> What's more, that allows to simplify adfs_fill_super() failure exit: we can leave
> freeing asb (and clearing ->s_fs_info, of course) to ->kill_sb() - the latter is
> called on any superblock destruction, including that after failing fill_super()
> callback.  Almost the first thing done by deactivate_locked_super() called in
> that case is
>                 fs->kill_sb(s);
>
> So if we go with "have it freed in ->kill_sb()" approach, the solution would be
>
> 1) adfs_kill_sb() calling kfree_rcu(asb, rcu) instead of kfree(asb)
> 2) call of kfree_rcu() removed from adfs_put_super()
> 3) all goto error; in adfs_fill_super() becoming return ret; (and error:
> getting removed, that is)