Re: BUG FIX: [PATCH RFC v3] [TESTED OK] memstick_check() memleak in kernel 6.1.0+ introduced pre 4.17
From: Mirsad Goran Todorovac
Date: Tue Apr 04 2023 - 06:37:31 EST
On 4/1/2023 16:56, Greg KH wrote:
On Sat, Apr 01, 2023 at 01:25:21PM +0200, Mirsad Goran Todorovac wrote:
On 01. 04. 2023. 11:23, Greg KH wrote:
On Sat, Apr 01, 2023 at 11:18:19AM +0200, Greg KH wrote:
On Sat, Apr 01, 2023 at 08:33:36AM +0200, Greg KH wrote:
On Sat, Apr 01, 2023 at 08:28:07AM +0200, Greg KH wrote:
On Sat, Apr 01, 2023 at 08:23:26AM +0200, Mirsad Goran Todorovac wrote:
This patch is implying that anyone who calls "dev_set_name()" also has
to do this hack, which shouldn't be the case at all.
thanks,
greg k-h
This is my best guess. Unless there is dev_free_name() or kobject_free_name(), I don't
see a more sensible way to patch this up.
In sleeping on this, I think this has to move to the driver core. I
don't understand why we haven't seen this before, except maybe no one
has really noticed before (i.e. we haven't had good leak detection tools
that run with removable devices?)
Anyway, let me see if I can come up with something this weekend, give me
a chance...
Wait, no, this already should be handled by the kobject core, look at
kobject_cleanup(), at the bottom. So your change should be merely
duplicating the logic there that already runs when the struct device is
freed, right?
So I don't understand why your change works, odd. I need more coffee...
I think you got half of the change correctly. This init code is a maze
of twisty passages, let me take your patch and tweak it a bit into
something that I think should work. This looks to be only a memstick
issue, not a driver core issue (which makes me feel better.)
Oops, forgot the patch. Can you try this change here and let me know if
that solves the problem or not? I have compile-tested it only, so I
have no idea if it works.
If this does work, I'll make up a "real" function to replace the
horrible dev.kobj.name mess that a driver would have to do here as it
shouldn't be required that a driver author knows the internals of the
driver core that well...
thanks,
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:
RESULTS:
w/o patch:
[root@pc-mtodorov marvin]# echo scan > /sys/kernel/debug/kmemleak
[root@pc-mtodorov marvin]# cat !$
cat /sys/kernel/debug/kmemleak
[root@pc-mtodorov marvin]# echo scan > /sys/kernel/debug/kmemleak
[root@pc-mtodorov marvin]# cat /sys/kernel/debug/kmemleak
unreferenced object 0xffffa09a93249590 (size 16):
comm "kworker/u12:4", pid 371, jiffies 4294896466 (age 52.748s)
hex dump (first 16 bytes):
6d 65 6d 73 74 69 63 6b 30 00 cc cc cc cc cc cc memstick0.......
backtrace:
[<ffffffff936fb25c>] slab_post_alloc_hook+0x8c/0x3e0
[<ffffffff93702b39>] __kmem_cache_alloc_node+0x1d9/0x2a0
[<ffffffff936773b9>] __kmalloc_node_track_caller+0x59/0x180
[<ffffffff93666a0a>] kstrdup+0x3a/0x70
[<ffffffff93666a7c>] kstrdup_const+0x2c/0x40
[<ffffffff93aa99dc>] kvasprintf_const+0x7c/0xb0
[<ffffffff9443b427>] kobject_set_name_vargs+0x27/0xa0
[<ffffffff93d8ee37>] dev_set_name+0x57/0x80
[<ffffffffc0aeff0f>] memstick_check+0x10f/0x3b0 [memstick]
[<ffffffff933cb4c0>] process_one_work+0x250/0x530
[<ffffffff933cb7f8>] worker_thread+0x48/0x3a0
[<ffffffff933d6dff>] kthread+0x10f/0x140
[<ffffffff93202fa9>] ret_from_fork+0x29/0x50
unreferenced object 0xffffa09a97205990 (size 16):
comm "kworker/u12:4", pid 371, jiffies 4294896471 (age 52.728s)
hex dump (first 16 bytes):
6d 65 6d 73 74 69 63 6b 30 00 cc cc cc cc cc cc memstick0.......
backtrace:
[<ffffffff936fb25c>] slab_post_alloc_hook+0x8c/0x3e0
[<ffffffff93702b39>] __kmem_cache_alloc_node+0x1d9/0x2a0
[<ffffffff936773b9>] __kmalloc_node_track_caller+0x59/0x180
[<ffffffff93666a0a>] kstrdup+0x3a/0x70
[<ffffffff93666a7c>] kstrdup_const+0x2c/0x40
[<ffffffff93aa99dc>] kvasprintf_const+0x7c/0xb0
[<ffffffff9443b427>] kobject_set_name_vargs+0x27/0xa0
[<ffffffff93d8ee37>] dev_set_name+0x57/0x80
[<ffffffffc0aeff0f>] memstick_check+0x10f/0x3b0 [memstick]
[<ffffffff933cb4c0>] process_one_work+0x250/0x530
[<ffffffff933cb7f8>] worker_thread+0x48/0x3a0
[<ffffffff933d6dff>] kthread+0x10f/0x140
[<ffffffff93202fa9>] ret_from_fork+0x29/0x50
[root@pc-mtodorov marvin]# uname -rms
Linux 6.3.0-rc4-mt-20230401-00199-g7b50567bdcad-dirty x86_64
[root@pc-mtodorov marvin]#
After the patch:
[root@pc-mtodorov marvin]# echo scan > /sys/kernel/debug/kmemleak
[root@pc-mtodorov marvin]# cat /sys/kernel/debug/kmemleak
[root@pc-mtodorov marvin]# echo scan > /sys/kernel/debug/kmemleak
[root@pc-mtodorov marvin]# cat /sys/kernel/debug/kmemleak
[root@pc-mtodorov marvin]# echo scan > /sys/kernel/debug/kmemleak
[root@pc-mtodorov marvin]# cat /sys/kernel/debug/kmemleak
So, congratulations, this did it!
Great, thanks for testing! And for working to narrow this down, that's
the hard part here.
This bug I detected on 2022-11-04, but it took me four months to find the leak,
before I was "blessed by the Source". You have asked me whether I would
help the memstick developers find a solution, and I like to keep promises. :-)
At your convenience, you might add in the patch:
Tested-by: Mirsad Goran Todorovac <mirsad.todorovac@xxxxxxxxxxxx>
It's been an honour serving with the memstick community with you and it was a real
brainstorming session for me.
Thanks, as you did way over half the work here, I think a co-developed
tag would be better. I'll send it out with that and you can provide a
signed-off-by on it that would be great.
thanks,
greg k-h
Sorry, only seen the mail today.
My Thunderbird still amuses me with its threading of emails :-|
Yes, the
Signed-of-by: Mirsad Todorovac <mirsad.todorovac@xxxxxxxxxxxx>
would be neat.
Thank you,
Mirsad
--
Mirsad Goran Todorovac
Sistem inženjer
Grafički fakultet | Akademija likovnih umjetnosti
Sveučilište u Zagrebu
--
System engineer
Faculty of Graphic Arts | Academy of Fine Arts
University of Zagreb, Republic of Croatia
tel. +385 (0)1 3711 451
mob. +385 91 57 88 355