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

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

[Ahmet Eray Karadag]

Syzbot reported a memory leak in adfs during the mount process. The issue
arises because the ownership of the allocated (struct adfs_sb_info) is
transferred from the filesystem context to the superblock via sget_fc().
This function sets fc->s_fs_info to NULL after the transfer.

The ADFS filesystem previously used the default kill_block_super for
superblock destruction. This helper performs generic cleanup but does not
free the private sb->s_fs_info data. Since fc->s_fs_info is set to
NULL during the transfer, the standard context cleanup (adfs_free_fc)
also skips freeing this memory. As a result, if the superblock is
destroyed, the allocated struct adfs_sb_info is leaked.

Fix this by implementing a custom .kill_sb callback (adfs_kill_sb)
that explicitly frees sb->s_fs_info before invoking the generic
kill_block_super.

Reported-by: syzbot+1c70732df5fd4f0e4fbb@syzkaller.appspotmail.com
Fixes: https://syzkaller.appspot.com/bug?extid=1c70732df5fd4f0e4fbb
Signed-off-by: Ahmet Eray Karadag <eraykrdg1@gmail.com>
---
 fs/adfs/super.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/fs/adfs/super.c b/fs/adfs/super.c
index fdccdbbfc213..afcd9f6ef350 100644
--- a/fs/adfs/super.c
+++ b/fs/adfs/super.c
@@ -462,10 +462,19 @@ static int adfs_init_fs_context(struct fs_context *fc)
 	return 0;
 }
 
+static void adfs_kill_sb(struct super_block *sb)
+{
+	struct adfs_sb_info *asb = ADFS_SB(sb);
+
+	kill_block_super(sb);
+
+	kfree(asb);
+}
+
 static struct file_system_type adfs_fs_type = {
 	.owner		= THIS_MODULE,
 	.name		= "adfs",
-	.kill_sb	= kill_block_super,
+	.kill_sb	= adfs_kill_sb,
 	.fs_flags	= FS_REQUIRES_DEV,
 	.init_fs_context = adfs_init_fs_context,
 	.parameters	= adfs_param_spec,
-- 
2.43.0

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

[Al Viro]

On Sun, Dec 14, 2025 at 02:36:22AM +0300, Ahmet Eray Karadag wrote:
> Syzbot reported a memory leak in adfs during the mount process. The issue
> arises because the ownership of the allocated (struct adfs_sb_info) is
> transferred from the filesystem context to the superblock via sget_fc().
> This function sets fc->s_fs_info to NULL after the transfer.
> 
> The ADFS filesystem previously used the default kill_block_super for
> superblock destruction. This helper performs generic cleanup but does not
> free the private sb->s_fs_info data. Since fc->s_fs_info is set to
> NULL during the transfer, the standard context cleanup (adfs_free_fc)
> also skips freeing this memory. As a result, if the superblock is
> destroyed, the allocated struct adfs_sb_info is leaked.
> 
> Fix this by implementing a custom .kill_sb callback (adfs_kill_sb)
> that explicitly frees sb->s_fs_info before invoking the generic
> kill_block_super.

I hate dealing with humans in the way one would deal with a chatbot, but...


Question: if that thing is leaking all the time, why hadn't that been caught
earlier?

Question: does it really leak all the time?  How would one check that?

Question: if it does not leak in each and every case, presumably the damn thing
does get freed at some point; where would that be?

Question: would we, by any chance, run into a double-free with that "fix"?


Please, do yourself a favour and find answers to the questions above.
They are fairly trivial and it is the kind of exercise one has to do every
time when dealing with something of that sort.

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

[Al Viro]

On Sun, Dec 14, 2025 at 01:32:49AM +0000, Al Viro wrote:

> Question: if that thing is leaking all the time, why hadn't that been caught
> earlier?
> 
> Question: does it really leak all the time?  How would one check that?
> 
> Question: if it does not leak in each and every case, presumably the damn thing
> does get freed at some point; where would that be?
> 
> Question: would we, by any chance, run into a double-free with that "fix"?
> 
> 
> Please, do yourself a favour and find answers to the questions above.
> They are fairly trivial and it is the kind of exercise one has to do every
> time when dealing with something of that sort.

<spoiler alert>

\f

A trivial experiment would be to mount a valid image, unmount it and see
if anything has leaked.  Finding a valid image is not hard - the second
hit when googling for "ADFS image acorn" is a github-hosted project
and right in there there's a directory called "ADFS Test Images", with
expected contents.  Whether it's legitimate or not, mounting it in a kernel
that runs under unpriveleged qemu ought to be safe enough.

And no, it doesn't leak on mount + umount

Looking for places where it could be freed in normal operation is also
not terribly hard - looking for kfree() in fs/adfs/super.c catches three
hits:

1) in adfs_put_super() we see
	struct adfs_sb_info *asb = ADFS_SB(sb);
	adfs_free_map(sb);
	kfree_rcu(asb, rcu);

2) in the end of adfs_fill_super() there's
error:
        sb->s_fs_info = NULL;
        kfree(asb);
        return ret;

3) in adfs_free_fc() we have
        struct adfs_context *asb = fc->s_fs_info;
        kfree(asb);

#2 and #3 are obviously irrelevant for the case of normal mount + umount -
adfs_fill_super() has already run at mount time (and did not hit error:)
and so did adfs_free_fc().

So we have #1 to look into.  adfs_put_super() is never called directly and
it's only reached as a member of struct super_operations adfs_sops -
something called 'put_super'.  Where would that method be called?

grep and you shall find it:
void generic_shutdown_super(struct super_block *sb)
{
        const struct super_operations *sop = sb->s_op;
 
        if (sb->s_root) {
		...
                if (sop->put_super)
                        sop->put_super(sb);

So it is called by generic_shutdown_super() in case if ->s_root had not
been NULL.  Looking for callers of generic_shutdown_super() immediately
catches
void kill_block_super(struct super_block *sb)
{
        struct block_device *bdev = sb->s_bdev;
 
        generic_shutdown_super(sb);
        if (bdev) {
                sync_blockdev(bdev);
                bdev_fput(sb->s_bdev_file);
        }
}
so it is called by adfs ->kill_sb() - both the current mainline and with
that patch.

IOW, there's our double-free.  For extra fun, it's not just kfree() + kfree(),
it's kfree_rcu() + kfree().

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

[Al Viro]

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)

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@zeniv.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)

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

[Ahmet Eray Karadag]

Syzbot reported a memory leak in adfs during the mount process. The issue
arises because the ownership of the allocated (struct adfs_sb_info) is
transferred from the filesystem context to the superblock via sget_fc().
This function sets fc->s_fs_info to NULL after the transfer.

The ADFS filesystem previously used the default kill_block_super for
superblock destruction. This helper performs generic cleanup but does not
free the private sb->s_fs_info data. Since fc->s_fs_info is set to
NULL during the transfer, the standard context cleanup (adfs_free_fc)
also skips freeing this memory. As a result, if the superblock is
destroyed, the allocated struct adfs_sb_info is leaked.

Fix this by implementing a custom .kill_sb callback (adfs_kill_sb)
that explicitly frees sb->s_fs_info before invoking the generic
kill_block_super.

Reported-by: syzbot+1c70732df5fd4f0e4fbb@syzkaller.appspotmail.com
Fixes: https://syzkaller.appspot.com/bug?extid=1c70732df5fd4f0e4fbb
Signed-off-by: Ahmet Eray Karadag <eraykrdg1@gmail.com>
---
v2:
 - Remove adfs_put_super
 - Remove error label in adfs_fill_super
 - Use kfree_rcu instead kfree
 - Free map in adfs_kill_sb
 - Tested with ADFS test images
---
 fs/adfs/super.c | 32 ++++++++++++++------------------
 1 file changed, 14 insertions(+), 18 deletions(-)

diff --git a/fs/adfs/super.c b/fs/adfs/super.c
index fdccdbbfc213..96855f1086cd 100644
--- a/fs/adfs/super.c
+++ b/fs/adfs/super.c
@@ -90,14 +90,6 @@ static int adfs_checkdiscrecord(struct adfs_discrecord *dr)
 	return 0;
 }
 
-static void adfs_put_super(struct super_block *sb)
-{
-	struct adfs_sb_info *asb = ADFS_SB(sb);
-
-	adfs_free_map(sb);
-	kfree_rcu(asb, rcu);
-}
-
 static int adfs_show_options(struct seq_file *seq, struct dentry *root)
 {
 	struct adfs_sb_info *asb = ADFS_SB(root->d_sb);
@@ -246,7 +238,6 @@ static const struct super_operations adfs_sops = {
 	.free_inode	= adfs_free_inode,
 	.drop_inode	= adfs_drop_inode,
 	.write_inode	= adfs_write_inode,
-	.put_super	= adfs_put_super,
 	.statfs		= adfs_statfs,
 	.show_options	= adfs_show_options,
 };
@@ -362,7 +353,7 @@ static int adfs_fill_super(struct super_block *sb, struct fs_context *fc)
 		ret = -EINVAL;
 	}
 	if (ret)
-		goto error;
+		return ret;
 
 	/* set up enough so that we can read an inode */
 	sb->s_op = &adfs_sops;
@@ -403,15 +394,9 @@ static int adfs_fill_super(struct super_block *sb, struct fs_context *fc)
 	if (!sb->s_root) {
 		adfs_free_map(sb);
 		adfs_error(sb, "get root inode failed\n");
-		ret = -EIO;
-		goto error;
+		return -EIO;
 	}
 	return 0;
-
-error:
-	sb->s_fs_info = NULL;
-	kfree(asb);
-	return ret;
 }
 
 static int adfs_get_tree(struct fs_context *fc)
@@ -462,10 +447,21 @@ static int adfs_init_fs_context(struct fs_context *fc)
 	return 0;
 }
 
+static void adfs_kill_sb(struct super_block *sb)
+{
+	struct adfs_sb_info *asb = ADFS_SB(sb);
+
+	kill_block_super(sb);
+
+	adfs_free_map(sb);
+
+	kfree_rcu(asb, rcu);
+}
+
 static struct file_system_type adfs_fs_type = {
 	.owner		= THIS_MODULE,
 	.name		= "adfs",
-	.kill_sb	= kill_block_super,
+	.kill_sb	= adfs_kill_sb,
 	.fs_flags	= FS_REQUIRES_DEV,
 	.init_fs_context = adfs_init_fs_context,
 	.parameters	= adfs_param_spec,
-- 
2.43.0

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

[Al Viro]

On Mon, Dec 15, 2025 at 03:22:52AM +0300, Ahmet Eray Karadag wrote:

> @@ -403,15 +394,9 @@ static int adfs_fill_super(struct super_block *sb, struct fs_context *fc)
>  	if (!sb->s_root) {
>  		adfs_free_map(sb);
>  		adfs_error(sb, "get root inode failed\n");
> -		ret = -EIO;
> -		goto error;
> +		return -EIO;

Double-free again, this time of asb->s_map - adfs_free_map() is called
twice in that case, once here and once in adfs_kill_sb()...

> +static void adfs_kill_sb(struct super_block *sb)
> +{
> +	struct adfs_sb_info *asb = ADFS_SB(sb);
> +
> +	kill_block_super(sb);
> +
> +	adfs_free_map(sb);

... and calling adfs_map_relse(), which calls brelse() after the block
device has been closed.  IOW, unlike freeing the ->s_fs_info this can't
be done after kill_block_super().  And with this one we don't have that
kind of gap - it doesn't exist until adfs_fill_super(), so there's no
leak to be had anyway.  Yes, it would be possible to get rid of more
of cleanup on failure exit there, but it would require expanding
kill_block_super() and inserting adfs_free_map() call before the call
sync_blockdev() in there - more headache than you win in adfs_fill_super(),
IMO.

Leave adfs_free_map() in adfs_put_super() (or make _it_ ->put_super(),
for that matter).

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

[Ahmet Eray Karadag]

Syzbot reported a memory leak in adfs during the mount process. The issue
arises because the ownership of the allocated (struct adfs_sb_info) is
transferred from the filesystem context to the superblock via sget_fc().
This function sets fc->s_fs_info to NULL after the transfer.

The ADFS filesystem previously used the default kill_block_super for
superblock destruction. This helper performs generic cleanup but does not
free the private sb->s_fs_info data. Since fc->s_fs_info is set to
NULL during the transfer, the standard context cleanup (adfs_free_fc)
also skips freeing this memory. As a result, if the superblock is
destroyed, the allocated struct adfs_sb_info is leaked.

Fix this by implementing a custom .kill_sb callback (adfs_kill_sb)
that explicitly frees sb->s_fs_info before invoking the generic
kill_block_super.

Reported-by: syzbot+1c70732df5fd4f0e4fbb@syzkaller.appspotmail.com
Fixes: https://syzkaller.appspot.com/bug?extid=1c70732df5fd4f0e4fbb
Signed-off-by: Ahmet Eray Karadag <eraykrdg1@gmail.com>
---
v2:
 - Remove adfs_put_super
 - Remove error label in adfs_fill_super
 - Use kfree_rcu instead kfree
 - Free map in adfs_kill_sb
 - Tested with ADFS test images
---
v3:
 - Restore adfs_put_super() to handle map cleanup
 - Moving map cleanup to kill_sb() caused a double-free
---
 fs/adfs/super.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/fs/adfs/super.c b/fs/adfs/super.c
index fdccdbbfc213..51bf4652422d 100644
--- a/fs/adfs/super.c
+++ b/fs/adfs/super.c
@@ -92,10 +92,7 @@ static int adfs_checkdiscrecord(struct adfs_discrecord *dr)
 
 static void adfs_put_super(struct super_block *sb)
 {
-	struct adfs_sb_info *asb = ADFS_SB(sb);
-
 	adfs_free_map(sb);
-	kfree_rcu(asb, rcu);
 }
 
 static int adfs_show_options(struct seq_file *seq, struct dentry *root)
@@ -362,7 +359,7 @@ static int adfs_fill_super(struct super_block *sb, struct fs_context *fc)
 		ret = -EINVAL;
 	}
 	if (ret)
-		goto error;
+		return ret;
 
 	/* set up enough so that we can read an inode */
 	sb->s_op = &adfs_sops;
@@ -403,15 +400,9 @@ static int adfs_fill_super(struct super_block *sb, struct fs_context *fc)
 	if (!sb->s_root) {
 		adfs_free_map(sb);
 		adfs_error(sb, "get root inode failed\n");
-		ret = -EIO;
-		goto error;
+		return -EIO;
 	}
 	return 0;
-
-error:
-	sb->s_fs_info = NULL;
-	kfree(asb);
-	return ret;
 }
 
 static int adfs_get_tree(struct fs_context *fc)
@@ -462,10 +453,19 @@ static int adfs_init_fs_context(struct fs_context *fc)
 	return 0;
 }
 
+static void adfs_kill_sb(struct super_block *sb)
+{
+	struct adfs_sb_info *asb = ADFS_SB(sb);
+
+	kill_block_super(sb);
+
+	kfree_rcu(asb, rcu);
+}
+
 static struct file_system_type adfs_fs_type = {
 	.owner		= THIS_MODULE,
 	.name		= "adfs",
-	.kill_sb	= kill_block_super,
+	.kill_sb	= adfs_kill_sb,
 	.fs_flags	= FS_REQUIRES_DEV,
 	.init_fs_context = adfs_init_fs_context,
 	.parameters	= adfs_param_spec,
-- 
2.43.0