Re: [RESEND PATCH] memstick: fix memory leak if card device is never registered

[Mirsad Goran Todorovac]

On 1.4.2023. 22:03, Greg Kroah-Hartman wrote:
> When calling dev_set_name() memory is allocated for the name for the
> struct device.  Once that structure device is registered, or attempted
> to be registerd, with the driver core, the driver core will handle
> cleaning up that memory when the device is removed from the system.
>
> Unfortunatly for the memstick code, there is an error path that causes
> the struct device to never be registered, and so the memory allocated in
> dev_set_name will be leaked.  Fix that leak by manually freeing it right
> before the memory for the device is freed.
>
> Cc: Maxim Levitsky <maximlevitsky@xxxxxxxxx>
> Cc: Alex Dubov <oakad@xxxxxxxxx>
> Cc: Ulf Hansson <ulf.hansson@xxxxxxxxxx>
> Cc: "Rafael J. Wysocki" <rafael@xxxxxxxxxx>
> Cc: Hans de Goede <hdegoede@xxxxxxxxxx>
> Cc: Kay Sievers <kay.sievers@xxxxxxxx>
> Cc: linux-mmc@xxxxxxxxxxxxxxx
> Fixes: 0252c3b4f018 ("memstick: struct device - replace bus_id with dev_name(),
> Cc: stable <stable@xxxxxxxxxx>
> Co-developed-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> Co-developed-by: Mirsad Goran Todorovac <mirsad.todorovac@xxxxxxxxxxxx>
> ---
> RESEND as the first version had a corrupted message id and never made it
> to the mailing lists or lore.kernel.org
>
>   drivers/memstick/core/memstick.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/memstick/core/memstick.c b/drivers/memstick/core/memstick.c
> index bf7667845459..bbfaf6536903 100644
> --- a/drivers/memstick/core/memstick.c
> +++ b/drivers/memstick/core/memstick.c
> @@ -410,6 +410,7 @@ static struct memstick_dev *memstick_alloc_card(struct memstick_host *host)
>   	return card;
>   err_out:
>   	host->card = old_card;
> +	kfree_const(card->dev.kobj.name);
>   	kfree(card);
>   	return NULL;
>   }
> @@ -468,8 +469,10 @@ static void memstick_check(struct work_struct *work)
>   				put_device(&card->dev);
>   				host->card = NULL;
>   			}
> -		} else
> +		} else {
> +			kfree_const(card->dev.kobj.name);
>   			kfree(card);
> +		}
>   	}
>   
>   out_power_off:

FYI, the applied patch tested OK, no memstick leaks:

[root@pc-mtodorov kernel]# uname -rms
Linux 6.3.0-rc5-mt-20230401-00007-g268a637be362 x86_64
[root@pc-mtodorov kernel]# echo scan > /sys/kernel/debug/kmemleak
[root@pc-mtodorov kernel]# cat /sys/kernel/debug/kmemleak
[root@pc-mtodorov kernel]# echo scan > /sys/kernel/debug/kmemleak
[root@pc-mtodorov kernel]# cat /sys/kernel/debug/kmemleak
[root@pc-mtodorov kernel]# echo scan > /sys/kernel/debug/kmemleak
[root@pc-mtodorov kernel]# cat /sys/kernel/debug/kmemleak
[root@pc-mtodorov kernel]# echo scan > /sys/kernel/debug/kmemleak
[root@pc-mtodorov kernel]# cat /sys/kernel/debug/kmemleak

What was applied is:

mtodorov@domac:~/linux/kernel/linux_torvalds$ git diff master..origin/master | head -24
diff --git a/drivers/memstick/core/memstick.c b/drivers/memstick/core/memstick.c
index bbfaf6536903..bf7667845459 100644
--- a/drivers/memstick/core/memstick.c
+++ b/drivers/memstick/core/memstick.c
@@ -410,7 +410,6 @@ static struct memstick_dev *memstick_alloc_card(struct memstick_host *host)
        return card;
 err_out:
        host->card = old_card;
-       kfree_const(card->dev.kobj.name);
        kfree(card);
        return NULL;
 }
@@ -469,10 +468,8 @@ static void memstick_check(struct work_struct *work)
                                put_device(&card->dev);
                                host->card = NULL;
                        }
-               } else {
-                       kfree_const(card->dev.kobj.name);
+               } else
                        kfree(card);
-               }
        }

 out_power_off:

APPENDIX

However, please note that the patch SHA-1's truncated to 12 chars might not
be the same on the other systems, so the build ID says nothing about which
mainline kernel had the patches been applied against. So `uname -rms` is
telling pretty nothing about which kernel I'm running, except that it helps
me distinguish between the builds.

Nothing to prove that:

- I am not testing the wrong kernel and
- that the right patches have been applied.

Changing CONFIG_LOCALVERSION causes copious recompilations, even with ccache
it takes about 4x the time needed to compile CONFIG_LOCALVERSION_AUTO=y.

Do I make any sense with this?

I am adding Cc: to Mr. Bagas, for we spoke already about kernel versioning
in the case of manually applied patches.

Regards,
Mirsad

--
Mirsad Todorovac
System engineer
Faculty of Graphic Arts | Academy of Fine Arts
University of Zagreb
Republic of Croatia, the European Union

Sistem inženjer
Grafički fakultet | Akademija likovnih umjetnosti
Sveučilište u Zagrebu