[bug report] mm: reduce noise in show_mem for lowmem allocations

[bug report] mm: reduce noise in show_mem for lowmem allocations

[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

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

Re: [bug report] mm: reduce noise in show_mem for lowmem allocations

[Michal Hocko]

On Tue 30-08-22 09:30:26, Dan Carpenter wrote:
> 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?

> 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.

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?
-- 
Michal Hocko
SUSE Labs

Re: [bug report] mm: reduce noise in show_mem for lowmem allocations

[Dan Carpenter]

On Tue, Aug 30, 2022 at 08:46:58AM +0200, Michal Hocko wrote:
> On Tue 30-08-22 09:30:26, Dan Carpenter wrote:
> > 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.

> >     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?

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

Re: [bug report] mm: reduce noise in show_mem for lowmem allocations

[Michal Hocko]

On Tue 30-08-22 10:02:05, Dan Carpenter wrote:
> On Tue, Aug 30, 2022 at 08:46:58AM +0200, Michal Hocko wrote:
> > On Tue 30-08-22 09:30:26, Dan Carpenter wrote:
> > > 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.
> 
> > >     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?
> 
> 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.
-- 
Michal Hocko
SUSE Labs

Re: [bug report] mm: reduce noise in show_mem for lowmem allocations

[Dan Carpenter]

On Tue, Aug 30, 2022 at 09:11:03AM +0200, Michal Hocko wrote:
> On Tue 30-08-22 10:02:05, Dan Carpenter wrote:
> > On Tue, Aug 30, 2022 at 08:46:58AM +0200, Michal Hocko wrote:
> > > On Tue 30-08-22 09:30:26, Dan Carpenter wrote:
> > > > 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.
> > 
> > > >     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?
> > 
> > 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