Re: [PATCH] adfs: fix memory leak in sb->s_fs_info

[Ahmet Eray Karadag]

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@xxxxxxxxxxxxxxxxxx> 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)