| Message ID | 20230401200327.16800-1-gregkh@linuxfoundation.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RESEND] memstick: fix memory leak if card device is never registered | expand |
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@gmail.com> > Cc: Alex Dubov <oakad@yahoo.com> > Cc: Ulf Hansson <ulf.hansson@linaro.org> > Cc: "Rafael J. Wysocki" <rafael@kernel.org> > Cc: Hans de Goede <hdegoede@redhat.com> > Cc: Kay Sievers <kay.sievers@vrfy.org> > Cc: linux-mmc@vger.kernel.org > Fixes: 0252c3b4f018 ("memstick: struct device - replace bus_id with dev_name(), > Cc: stable <stable@kernel.org> > Co-developed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Co-developed-by: Mirsad Goran Todorovac <mirsad.todorovac@alu.unizg.hr> > --- > 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
On Sat, 1 Apr 2023 at 22:03, Greg Kroah-Hartman <gregkh@linuxfoundation.org> 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@gmail.com> > Cc: Alex Dubov <oakad@yahoo.com> > Cc: Ulf Hansson <ulf.hansson@linaro.org> > Cc: "Rafael J. Wysocki" <rafael@kernel.org> > Cc: Hans de Goede <hdegoede@redhat.com> > Cc: Kay Sievers <kay.sievers@vrfy.org> > Cc: linux-mmc@vger.kernel.org > Fixes: 0252c3b4f018 ("memstick: struct device - replace bus_id with dev_name(), > Cc: stable <stable@kernel.org> > Co-developed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Co-developed-by: Mirsad Goran Todorovac <mirsad.todorovac@alu.unizg.hr> Applied for fixes and by adding Mirsad's sob tag (according to the other thread [1]), thanks! Kind regards Uffe [1] https://lore.kernel.org/lkml/c059f486-98a6-aecd-c135-c033612e6b4f@alu.unizg.hr/ > --- > 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: > -- > 2.40.0 >
On Tue, Apr 04, 2023 at 01:54:03PM +0200, Ulf Hansson wrote: > On Sat, 1 Apr 2023 at 22:03, Greg Kroah-Hartman > <gregkh@linuxfoundation.org> 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@gmail.com> > > Cc: Alex Dubov <oakad@yahoo.com> > > Cc: Ulf Hansson <ulf.hansson@linaro.org> > > Cc: "Rafael J. Wysocki" <rafael@kernel.org> > > Cc: Hans de Goede <hdegoede@redhat.com> > > Cc: Kay Sievers <kay.sievers@vrfy.org> > > Cc: linux-mmc@vger.kernel.org > > Fixes: 0252c3b4f018 ("memstick: struct device - replace bus_id with dev_name(), > > Cc: stable <stable@kernel.org> > > Co-developed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > Co-developed-by: Mirsad Goran Todorovac <mirsad.todorovac@alu.unizg.hr> > > Applied for fixes and by adding Mirsad's sob tag (according to the > other thread [1]), thanks! Wonderful, thanks for picking up that tag, and the patch. greg k-h
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:
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@gmail.com> Cc: Alex Dubov <oakad@yahoo.com> Cc: Ulf Hansson <ulf.hansson@linaro.org> Cc: "Rafael J. Wysocki" <rafael@kernel.org> Cc: Hans de Goede <hdegoede@redhat.com> Cc: Kay Sievers <kay.sievers@vrfy.org> Cc: linux-mmc@vger.kernel.org Fixes: 0252c3b4f018 ("memstick: struct device - replace bus_id with dev_name(), Cc: stable <stable@kernel.org> Co-developed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Co-developed-by: Mirsad Goran Todorovac <mirsad.todorovac@alu.unizg.hr> --- 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(-)