* [bug report] mm: reduce noise in show_mem for lowmem allocations
@ 2022-08-30 6:30 Dan Carpenter
2022-08-30 6:46 ` Michal Hocko
0 siblings, 1 reply; 5+ messages in thread
From: Dan Carpenter @ 2022-08-30 6:30 UTC (permalink / raw)
To: mhocko; +Cc: linux-mm
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [bug report] mm: reduce noise in show_mem for lowmem allocations
2022-08-30 6:30 [bug report] mm: reduce noise in show_mem for lowmem allocations Dan Carpenter
@ 2022-08-30 6:46 ` Michal Hocko
2022-08-30 7:02 ` Dan Carpenter
0 siblings, 1 reply; 5+ messages in thread
From: Michal Hocko @ 2022-08-30 6:46 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-mm
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [bug report] mm: reduce noise in show_mem for lowmem allocations
2022-08-30 6:46 ` Michal Hocko
@ 2022-08-30 7:02 ` Dan Carpenter
2022-08-30 7:11 ` Michal Hocko
0 siblings, 1 reply; 5+ messages in thread
From: Dan Carpenter @ 2022-08-30 7:02 UTC (permalink / raw)
To: Michal Hocko; +Cc: linux-mm
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [bug report] mm: reduce noise in show_mem for lowmem allocations
2022-08-30 7:02 ` Dan Carpenter
@ 2022-08-30 7:11 ` Michal Hocko
2022-08-30 7:59 ` Dan Carpenter
0 siblings, 1 reply; 5+ messages in thread
From: Michal Hocko @ 2022-08-30 7:11 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-mm
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [bug report] mm: reduce noise in show_mem for lowmem allocations
2022-08-30 7:11 ` Michal Hocko
@ 2022-08-30 7:59 ` Dan Carpenter
0 siblings, 0 replies; 5+ messages in thread
From: Dan Carpenter @ 2022-08-30 7:59 UTC (permalink / raw)
To: Michal Hocko; +Cc: linux-mm
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-08-30 8:00 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-30 6:30 [bug report] mm: reduce noise in show_mem for lowmem allocations Dan Carpenter
2022-08-30 6:46 ` Michal Hocko
2022-08-30 7:02 ` Dan Carpenter
2022-08-30 7:11 ` Michal Hocko
2022-08-30 7:59 ` Dan Carpenter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox