linux-kernel - Re: [PATCH 1/2] printk: Introduce LOUD

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <533f6a578d8489c1107ba83ce1e49b6e2d5c84ce.camel@suse.com>
Date: Wed, 23 Oct 2024 17:36:04 -0300
From: Marcos Paulo de Souza <mpdesouza@...e.com>
To: John Ogness <john.ogness@...utronix.de>, Petr Mladek <pmladek@...e.com>
Cc: Steven Rostedt <rostedt@...dmis.org>, Sergey Senozhatsky	
 <senozhatsky@...omium.org>, Greg Kroah-Hartman
 <gregkh@...uxfoundation.org>,  Jiri Slaby <jirislaby@...nel.org>,
 linux-kernel@...r.kernel.org, linux-serial@...r.kernel.org
Subject: Re: [PATCH 1/2] printk: Introduce LOUD_CON flag

On Mon, 2024-10-21 at 16:17 +0206, John Ogness wrote:
> On 2024-10-21, Petr Mladek <pmladek@...e.com> wrote:
> > > That will not work because migrate_enable() can only be called
> > > from
> > > can_sleep context. Instead, the migrate_disable()/enable() should
> > > be at
> > > the few (one?) call sites where
> > > printk_loud_console_enter()/exit() is
> > > used from task context.
> > 
> > Hmm, if I get it correctly, we could not use migrate_disable() in
> > __handle_sysrq() because it can be called also in atomic context,
> 
> I am talking about callers of __handle_sysrq() and/or their callers.
> 
> For example write_sysrq_trigger() could do:
> 
> 	migrate_disable();
> 	__handle_sysrq(c, false);
> 	migrate_enable();
> 
> Or a new wrapper could be introduced for this purpose:
> 
> static inline void wrapper handle_sysrq_task(u8 key, bool check_mask)
> {
> 	migrate_disable();
> 	__handle_sysrq(key, check_mask);
> 	migrate_enable();
> }
> 
> A quick grep shows about 25 call sites to check.
> 
> > I do not see any easy way how to distinguish whether it was called
> > in
> > an atomic context or not.
> 
> There is no clean way to do that. If this information is needed, it
> must
> be tracked by the call chain.
> 
> > So, I see three possibilities:
> > 
> >   1. Explicitly call preempt_disable() in __handle_sysrq().
> > 
> >      It would be just around the the single line or the help. But
> > still,
> >      I do not like it much.
> 
> Not acceptable for PREEMPT_RT since sysrq is exposed to external
> inputs.
> 
> >   2. Avoid the per-CPU variable. Force adding the
> > LOUD_CON/FORCE_CON
> >      flag using a global variable, e.g. printk_force_console.
> > 
> >      The problem is that it might affect also messages printed by
> >      other CPUs. And there might be many.
> > 
> >      Well, console_loglevel is a global variable. The original code
> >      had a similar problem.
> 
> If disabling migration is not an option for you, this would be my
> second
> choice. I assume tagging too many messages is better than not tagging
> enough. And, as you say, this is effectively what the current code is
> trying to do.

Thanks for your input John. I talked with Petr and he suggested to
follow this option. I'll prepare the changes and send them after
reviewing them with Petr.

Thanks,
  Marcos

> 
> >   3. Add the LOUD_CON/FLUSH_CON flag via a parameter. For example,
> >      by a special LOGLEVEL_FORCE_CON, similar to LOGLEVEL_SCHED.
> > 
> >      I might work well for __handle_sysrq() which calls the
> > affected
> >      printk() directly.
> > 
> >      But it won't work, for example, for kdb_show_stack(). It wants
> >      to show messages printed by a nested functions.
> 
> Right, this has limited usefulness and might miss the important
> things,
> which tend to be within helper functions.
> 
> John