LinuxLists.cc - [PATCHv4 2/2] mm: use stack_depot_early

2023-01-19 01:52:23

Subject: [PATCHv4 2/2] mm: use stack_depot_early_init for kmemleak

From: Zhaoyang Huang <[email protected]>

Mirsad report bellow error which caused by stack_depot_init failed in kvcalloc.
Solve this by having stackdepot use stack_depot_early_init.

On 1/4/23 17:08, Mirsad Goran Todorovac wrote:
I hate to bring bad news again, but there seems to be a problem with the output of /sys/kernel/debug/kmemleak:

[root@pc-mtodorov ~]# cat /sys/kernel/debug/kmemleak
unreferenced object 0xffff951c118568b0 (size 16):
comm "kworker/u12:2", pid 56, jiffies 4294893952 (age 4356.548s)
hex dump (first 16 bytes):
6d 65 6d 73 74 69 63 6b 30 00 00 00 00 00 00 00 memstick0.......
backtrace:
[root@pc-mtodorov ~]#

Apparently, backtrace of called functions on the stack is no longer printed with the list of memory leaks.
This appeared on Lenovo desktop 10TX000VCR, with AlmaLinux 8.7 and BIOS version M22KT49A (11/10/2022)
and 6.2-rc1 and 6.2-rc2 builds.
This worked on 6.1 with the same CONFIG_KMEMLEAK=y and MGLRU enabled on a vanilla mainstream kernel
from Mr. Torvalds' tree. I don't know if this is deliberate feature for some reason or a bug.
Please find attached the config, lshw and kmemleak output.

reported-by: Mirsad Todorovac <[email protected]>
suggested-by: Vlastimil Babka <[email protected]>
Signed-off-by: Zhaoyang Huang <[email protected]>
---
v2: use stack_depot_want_early_init instead of CONFIG_STACKDEPOT_ALWAYS_INIT
v3: have the Kconfig changes commited in another patch
v4: select CONFIG_STACKDEPOT_ALWAYS_INIT when DEBUG_KMEMLEAK_DEFAULT_OFF is off
---

Signed-off-by: Zhaoyang Huang <[email protected]>
---
mm/Kconfig.debug | 1 +
mm/kmemleak.c | 4 +++-
2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/mm/Kconfig.debug b/mm/Kconfig.debug
index d1893ac..466a37e 100644
--- a/mm/Kconfig.debug
+++ b/mm/Kconfig.debug
@@ -219,6 +219,7 @@ config DEBUG_KMEMLEAK
select KALLSYMS
select CRC32
select STACKDEPOT
+ select STACKDEPOT_ALWAYS_INIT if !DEBUG_KMEMLEAK_DEFAULT_OFF
help
Say Y here if you want to enable the memory leak
detector. The memory allocation/freeing is traced in a way
diff --git a/mm/kmemleak.c b/mm/kmemleak.c
index 762b91f..ddc1ddf 100644
--- a/mm/kmemleak.c
+++ b/mm/kmemleak.c
@@ -2070,8 +2070,10 @@ static int __init kmemleak_boot_config(char *str)
return -EINVAL;
if (strcmp(str, "off") == 0)
kmemleak_disable();
- else if (strcmp(str, "on") == 0)
+ else if (strcmp(str, "on") == 0) {
kmemleak_skip_disable = 1;
+ stack_depot_want_early_init();
+ }
else
return -EINVAL;
return 0;
--
1.9.1

2023-01-19 10:35:23

by Catalin Marinas

[permalink] [raw]

Subject: Re: [PATCHv4 2/2] mm: use stack_depot_early_init for kmemleak

2023-01-19 10:42:15

by Vlastimil Babka

[permalink] [raw]

Subject: Re: [PATCHv4 2/2] mm: use stack_depot_early_init for kmemleak

On 1/19/23 02:22, zhaoyang.huang wrote:
> From: Zhaoyang Huang <[email protected]>
>
> Mirsad report bellow error which caused by stack_depot_init failed in kvcalloc.
> Solve this by having stackdepot use stack_depot_early_init.
>
> On 1/4/23 17:08, Mirsad Goran Todorovac wrote:
> I hate to bring bad news again, but there seems to be a problem with the output of /sys/kernel/debug/kmemleak:
>
> [root@pc-mtodorov ~]# cat /sys/kernel/debug/kmemleak
> unreferenced object 0xffff951c118568b0 (size 16):
> comm "kworker/u12:2", pid 56, jiffies 4294893952 (age 4356.548s)
> hex dump (first 16 bytes):
> 6d 65 6d 73 74 69 63 6b 30 00 00 00 00 00 00 00 memstick0.......
> backtrace:
> [root@pc-mtodorov ~]#
>
> Apparently, backtrace of called functions on the stack is no longer printed with the list of memory leaks.
> This appeared on Lenovo desktop 10TX000VCR, with AlmaLinux 8.7 and BIOS version M22KT49A (11/10/2022)
> and 6.2-rc1 and 6.2-rc2 builds.
> This worked on 6.1 with the same CONFIG_KMEMLEAK=y and MGLRU enabled on a vanilla mainstream kernel
> from Mr. Torvalds' tree. I don't know if this is deliberate feature for some reason or a bug.
> Please find attached the config, lshw and kmemleak output.

I think we could replace the full quote of the report with

Link: https://lore.kernel.org/all/[email protected]/

also

Fixes: 56a61617dd22 ("mm: use stack_depot for recording kmemleak's backtrace")

(Andrew can do that when picking up, no need to send v5)

> reported-by: Mirsad Todorovac <[email protected]>
> suggested-by: Vlastimil Babka <[email protected]>
> Signed-off-by: Zhaoyang Huang <[email protected]>

Acked-by: Vlastimil Babka <[email protected]>

But to be cleaner I'd also suggest Andrew adds the hunk below. The call
to stack_depot_init() becomes no-op after this patch so it's not a bug
to leave it there, but it's just misleading now.

---8<---
diff --git a/mm/kmemleak.c b/mm/kmemleak.c
index 91dda5c2753a..55dc8b8b0616 100644
--- a/mm/kmemleak.c
+++ b/mm/kmemleak.c
@@ -2095,7 +2095,6 @@ void __init kmemleak_init(void)
if (kmemleak_error)
return;

- stack_depot_init();
jiffies_min_age = msecs_to_jiffies(MSECS_MIN_AGE);
jiffies_scan_wait = msecs_to_jiffies(SECS_SCAN_WAIT * 1000);

2023-01-19 23:01:02

by Andrew Morton

[permalink] [raw]

Subject: Re: [PATCHv4 2/2] mm: use stack_depot_early_init for kmemleak

On Thu, 19 Jan 2023 11:32:36 +0100 Vlastimil Babka <[email protected]> wrote:

> But to be cleaner I'd also suggest Andrew adds the hunk below. The call
> to stack_depot_init() becomes no-op after this patch so it's not a bug
> to leave it there, but it's just misleading now.
>
> ---8<---
> diff --git a/mm/kmemleak.c b/mm/kmemleak.c
> index 91dda5c2753a..55dc8b8b0616 100644
> --- a/mm/kmemleak.c
> +++ b/mm/kmemleak.c
> @@ -2095,7 +2095,6 @@ void __init kmemleak_init(void)
> if (kmemleak_error)
> return;
>
> - stack_depot_init();
> jiffies_min_age = msecs_to_jiffies(MSECS_MIN_AGE);
> jiffies_scan_wait = msecs_to_jiffies(SECS_SCAN_WAIT * 1000);
>

I added your signoff to this.

I used not to bother for such minor to-be-folded fixups, but now
Stephen sends me automated nags when his scripts detect this.

2023-01-20 05:53:19

by Mike Rapoport

[permalink] [raw]

Subject: Re: [PATCHv4 2/2] mm: use stack_depot_early_init for kmemleak

On Thu, Jan 19, 2023 at 09:22:25AM +0800, zhaoyang.huang wrote:
> From: Zhaoyang Huang <[email protected]>
>
> Mirsad report bellow error which caused by stack_depot_init failed in kvcalloc.
> Solve this by having stackdepot use stack_depot_early_init.
>
> On 1/4/23 17:08, Mirsad Goran Todorovac wrote:
> I hate to bring bad news again, but there seems to be a problem with the output of /sys/kernel/debug/kmemleak:
>
> [root@pc-mtodorov ~]# cat /sys/kernel/debug/kmemleak
> unreferenced object 0xffff951c118568b0 (size 16):
> comm "kworker/u12:2", pid 56, jiffies 4294893952 (age 4356.548s)
> hex dump (first 16 bytes):
> 6d 65 6d 73 74 69 63 6b 30 00 00 00 00 00 00 00 memstick0.......
> backtrace:
> [root@pc-mtodorov ~]#
>
> Apparently, backtrace of called functions on the stack is no longer printed with the list of memory leaks.
> This appeared on Lenovo desktop 10TX000VCR, with AlmaLinux 8.7 and BIOS version M22KT49A (11/10/2022)
> and 6.2-rc1 and 6.2-rc2 builds.
> This worked on 6.1 with the same CONFIG_KMEMLEAK=y and MGLRU enabled on a vanilla mainstream kernel
> from Mr. Torvalds' tree. I don't know if this is deliberate feature for some reason or a bug.
> Please find attached the config, lshw and kmemleak output.
>
> reported-by: Mirsad Todorovac <[email protected]>
> suggested-by: Vlastimil Babka <[email protected]>
> Signed-off-by: Zhaoyang Huang <[email protected]>

Acked-by: Mike Rapoport (IBM) <[email protected]>

> ---
> v2: use stack_depot_want_early_init instead of CONFIG_STACKDEPOT_ALWAYS_INIT
> v3: have the Kconfig changes commited in another patch
> v4: select CONFIG_STACKDEPOT_ALWAYS_INIT when DEBUG_KMEMLEAK_DEFAULT_OFF is off
> ---
>
> Signed-off-by: Zhaoyang Huang <[email protected]>
> ---
> mm/Kconfig.debug | 1 +
> mm/kmemleak.c | 4 +++-
> 2 files changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/mm/Kconfig.debug b/mm/Kconfig.debug
> index d1893ac..466a37e 100644
> --- a/mm/Kconfig.debug
> +++ b/mm/Kconfig.debug
> @@ -219,6 +219,7 @@ config DEBUG_KMEMLEAK
> select KALLSYMS
> select CRC32
> select STACKDEPOT
> + select STACKDEPOT_ALWAYS_INIT if !DEBUG_KMEMLEAK_DEFAULT_OFF
> help
> Say Y here if you want to enable the memory leak
> detector. The memory allocation/freeing is traced in a way
> diff --git a/mm/kmemleak.c b/mm/kmemleak.c
> index 762b91f..ddc1ddf 100644
> --- a/mm/kmemleak.c
> +++ b/mm/kmemleak.c
> @@ -2070,8 +2070,10 @@ static int __init kmemleak_boot_config(char *str)
> return -EINVAL;
> if (strcmp(str, "off") == 0)
> kmemleak_disable();
> - else if (strcmp(str, "on") == 0)
> + else if (strcmp(str, "on") == 0) {
> kmemleak_skip_disable = 1;
> + stack_depot_want_early_init();
> + }
> else
> return -EINVAL;
> return 0;
> --
> 1.9.1
>
>

--
Sincerely yours,
Mike.

2023-01-20 13:21:02

by Vlastimil Babka

[permalink] [raw]

Subject: Re: [PATCHv4 2/2] mm: use stack_depot_early_init for kmemleak

On 1/19/23 23:42, Andrew Morton wrote:
> On Thu, 19 Jan 2023 11:32:36 +0100 Vlastimil Babka <[email protected]> wrote:
>
>> But to be cleaner I'd also suggest Andrew adds the hunk below. The call
>> to stack_depot_init() becomes no-op after this patch so it's not a bug
>> to leave it there, but it's just misleading now.
>>
>> ---8<---
>> diff --git a/mm/kmemleak.c b/mm/kmemleak.c
>> index 91dda5c2753a..55dc8b8b0616 100644
>> --- a/mm/kmemleak.c
>> +++ b/mm/kmemleak.c
>> @@ -2095,7 +2095,6 @@ void __init kmemleak_init(void)
>> if (kmemleak_error)
>> return;
>>
>> - stack_depot_init();
>> jiffies_min_age = msecs_to_jiffies(MSECS_MIN_AGE);
>> jiffies_scan_wait = msecs_to_jiffies(SECS_SCAN_WAIT * 1000);
>>
>
> I added your signoff to this.

Fine with me, thanks.

> I used not to bother for such minor to-be-folded fixups, but now
> Stephen sends me automated nags when his scripts detect this.

Ack, will try to remember that next time.