mm: reduce noise in show_mem for lowmem allocations

bug report Dan Carpenter 2 participants 5 messages Aug 30, 2022 Memory management

Share & tools

Dan Carpenter

author

2022-08-30 06:30 UTC #0 lore raw

Hello Michal Hocko,

The patch e8fedfea3dea: "mm: reduce noise in show_mem for lowmem allocations" from Aug 23, 2022, leads to the following Smatch static checker warning:

	kernel/panic.c:190 panic_print_sys_info()
	warn: sleeping in atomic context

kernel/panic.c

    175 static void panic_print_sys_info(bool console_flush)
    176 {
    177         if (console_flush) {
    178                 if (panic_print & PANIC_PRINT_ALL_PRINTK_MSG)
    179                         console_flush_on_panic(CONSOLE_REPLAY_ALL);
    180                 return;
    181         }
    182 
    183         if (panic_print & PANIC_PRINT_ALL_CPU_BT)
    184                 trigger_all_cpu_backtrace();
    185 
    186         if (panic_print & PANIC_PRINT_TASK_INFO)
    187                 show_state();
    188 
    189         if (panic_print & PANIC_PRINT_MEM_INFO)
--> 190                 show_mem(0, NULL, GFP_HIGHUSER_MOVABLE);
                                          ^^^^^^^^^^^^^^^^^^^^

This obviously seems very deliberate and a lot of weird stuff happens during panic(). But the panic() function disables preemption so shouldn't this be GFP_ATOMIC? GFP_HIGHUSER_MOVABLE has __GFP_RECLAIM and triggering swap during a panic seems bad.

    191 
    192         if (panic_print & PANIC_PRINT_TIMER_INFO)
    193                 sysrq_timer_list_show();
    194 
    195         if (panic_print & PANIC_PRINT_LOCK_INFO)
    196                 debug_show_all_locks();
    197 
    198         if (panic_print & PANIC_PRINT_FTRACE_INFO)
    199                 ftrace_dump(DUMP_ALL);
    200 }

regards,
dan carpenter

Michal Hocko

2022-08-30 06:47 UTC #1 lore raw

Quoted 8 lines · Dan Carpenter

Hello Michal Hocko,
The patch e8fedfea3dea: "mm: reduce noise in show_mem for lowmem allocations" from Aug 23, 2022, leads to the following Smatch static checker warning:
	kernel/panic.c:190 panic_print_sys_info()
	warn: sleeping in atomic context

What is this warning saying?

Quoted 18 lines

kernel/panic.c

    175 static void panic_print_sys_info(bool console_flush)
    176 {
    177         if (console_flush) {
    178                 if (panic_print & PANIC_PRINT_ALL_PRINTK_MSG)
    179                         console_flush_on_panic(CONSOLE_REPLAY_ALL);
    180                 return;
    181         }
    182 
    183         if (panic_print & PANIC_PRINT_ALL_CPU_BT)
    184                 trigger_all_cpu_backtrace();
    185 
    186         if (panic_print & PANIC_PRINT_TASK_INFO)
    187                 show_state();
    188 
    189         if (panic_print & PANIC_PRINT_MEM_INFO)
--> 190                 show_mem(0, NULL, GFP_HIGHUSER_MOVABLE);
                                          ^^^^^^^^^^^^^^^^^^^^

Quoted 4 lines

This obviously seems very deliberate and a lot of weird stuff happens during panic(). But the panic() function disables preemption so shouldn't this be GFP_ATOMIC? GFP_HIGHUSER_MOVABLE has __GFP_RECLAIM and triggering swap during a panic seems bad.

This function shouldn't ever be allocating any memory. The flag is solely to infer which memory zones should be displayed. It acts as a filter. Is it possible that the checker misinterprets the parameter's meaning?

Signature · 2 lines

Michal Hocko
SUSE Labs

Dan Carpenter

author

2022-08-30 07:02 UTC #2 lore raw

Quoted 12 lines · Michal Hocko

Quoted 8 lines · Dan Carpenter
Hello Michal Hocko,
The patch e8fedfea3dea: "mm: reduce noise in show_mem for lowmem allocations" from Aug 23, 2022, leads to the following Smatch static checker warning:
	kernel/panic.c:190 panic_print_sys_info()
	warn: sleeping in atomic context
What is this warning saying?

This is a Smatch warning.

Quoted 8 lines

Quoted 7 lines
189 if (panic_print & PANIC_PRINT_MEM_INFO)
--> 190                 show_mem(0, NULL, GFP_HIGHUSER_MOVABLE);
                                          ^^^^^^^^^^^^^^^^^^^^
This obviously seems very deliberate and a lot of weird stuff happens during panic(). But the panic() function disables preemption so shouldn't this be GFP_ATOMIC? GFP_HIGHUSER_MOVABLE has __GFP_RECLAIM and triggering swap during a panic seems bad.

Quoted 4 lines

This function shouldn't ever be allocating any memory. The flag is solely to infer which memory zones should be displayed. It acts as a filter. Is it possible that the checker misinterprets the parameter's meaning?

Ah. Yes. Smatch sees every gfp_t as a sleep/no sleep marker. I didn't realize it wasn't used like that here. Thanks!

regards,
dan carpenter

Michal Hocko

2022-08-30 07:11 UTC #3 lore raw

Quoted 24 lines · Dan Carpenter

Quoted 12 lines · Michal Hocko
Quoted 8 lines · Dan Carpenter
Hello Michal Hocko,
The patch e8fedfea3dea: "mm: reduce noise in show_mem for lowmem allocations" from Aug 23, 2022, leads to the following Smatch static checker warning:
	kernel/panic.c:190 panic_print_sys_info()
	warn: sleeping in atomic context
What is this warning saying?
This is a Smatch warning.
Quoted 8 lines
Quoted 7 lines
189 if (panic_print & PANIC_PRINT_MEM_INFO)
--> 190                 show_mem(0, NULL, GFP_HIGHUSER_MOVABLE);
                                          ^^^^^^^^^^^^^^^^^^^^
This obviously seems very deliberate and a lot of weird stuff happens during panic(). But the panic() function disables preemption so shouldn't this be GFP_ATOMIC? GFP_HIGHUSER_MOVABLE has __GFP_RECLAIM and triggering swap during a panic seems bad.

Quoted 7 lines

Quoted 4 lines
This function shouldn't ever be allocating any memory. The flag is solely to infer which memory zones should be displayed. It acts as a filter. Is it possible that the checker misinterprets the parameter's meaning?
Ah. Yes. Smatch sees every gfp_t as a sleep/no sleep marker. I didn't realize it wasn't used like that here. Thanks!

OK, fair enough and I can actually see how that can turn out into a real allocation in a distant future when the original intention has been lost in the past. Let me re-open the discussion for that patch and CC you there.

Thanks for the report.

Signature · 2 lines

Michal Hocko
SUSE Labs

Dan Carpenter

author

2022-08-30 08:00 UTC #4 lore raw

Quoted 30 lines · Michal Hocko

Quoted 29 lines · Dan Carpenter
Quoted 12 lines · Michal Hocko
Quoted 8 lines · Dan Carpenter
Hello Michal Hocko,
The patch e8fedfea3dea: "mm: reduce noise in show_mem for lowmem allocations" from Aug 23, 2022, leads to the following Smatch static checker warning:
	kernel/panic.c:190 panic_print_sys_info()
	warn: sleeping in atomic context
What is this warning saying?
This is a Smatch warning.
Quoted 12 lines
Quoted 7 lines
189 if (panic_print & PANIC_PRINT_MEM_INFO)
--> 190                 show_mem(0, NULL, GFP_HIGHUSER_MOVABLE);
                                          ^^^^^^^^^^^^^^^^^^^^
This obviously seems very deliberate and a lot of weird stuff happens during panic(). But the panic() function disables preemption so shouldn't this be GFP_ATOMIC? GFP_HIGHUSER_MOVABLE has __GFP_RECLAIM and triggering swap during a panic seems bad.
This function shouldn't ever be allocating any memory. The flag is solely to infer which memory zones should be displayed. It acts as a filter. Is it possible that the checker misinterprets the parameter's meaning?

Quoted 7 lines

Quoted 2 lines
Ah. Yes. Smatch sees every gfp_t as a sleep/no sleep marker. I didn't realize it wasn't used like that here. Thanks!
OK, fair enough and I can actually see how that can turn out into a real allocation in a distant future when the original intention has been lost in the past. Let me re-open the discussion for that patch and CC you there.

No no. I can silence this in Smatch. I hadn't really wanted to bother because this one false positive doesn't stand out from all the other false positives.

The sleeping in atomic check in Smatch is generally pretty good, but when the call tree is five or more functions deep between disabling preemption and the sleep then that's the main source of false positives.

regards,
dan carpenter