From: John Ogness <john.ogness@linutronix.de>
To: Petr Mladek <pmladek@suse.com>
Cc: "Sergey Senozhatsky" <senozhatsky@chromium.org>,
"Steven Rostedt" <rostedt@goodmis.org>,
"Thomas Gleixner" <tglx@linutronix.de>,
linux-kernel@vger.kernel.org,
"Michael Ellerman" <mpe@ellerman.id.au>,
"Benjamin Herrenschmidt" <benh@kernel.crashing.org>,
"Paul Mackerras" <paulus@samba.org>,
"Eric Biederman" <ebiederm@xmission.com>,
"Nicholas Piggin" <npiggin@gmail.com>,
"Christophe Leroy" <christophe.leroy@csgroup.eu>,
"Cédric Le Goater" <clg@kaod.org>,
"Andrew Morton" <akpm@linux-foundation.org>,
"Kees Cook" <keescook@chromium.org>,
"Tiezhu Yang" <yangtiezhu@loongson.cn>,
"Yue Hu" <huyue2@yulong.com>,
"Alexey Kardashevskiy" <aik@ozlabs.ru>,
"Paul E. McKenney" <paulmck@kernel.org>,
linuxppc-dev@lists.ozlabs.org, kexec@lists.infradead.org
Subject: Re: [PATCH printk v3 3/6] printk: remove safe buffers
Date: Thu, 24 Jun 2021 17:41:56 +0206 [thread overview]
Message-ID: <8735t7mg0z.fsf@jogness.linutronix.de> (raw)
In-Reply-To: <YNSbd68YJ+0wxayx@alley>
On 2021-06-24, Petr Mladek <pmladek@suse.com> wrote:
>> --- a/kernel/printk/printk.c
>> +++ b/kernel/printk/printk.c
>> @@ -1852,7 +1839,7 @@ static int console_trylock_spinning(void)
>> if (console_trylock())
>> return 1;
>>
>> - printk_safe_enter_irqsave(flags);
>> + local_irq_save(flags);
>>
>> raw_spin_lock(&console_owner_lock);
>
> This spin_lock is in the printk() path. We must make sure that
> it does not cause deadlock.
>
> printk_safe_enter_irqsave(flags) prevented the recursion because
> it deferred the console handling.
>
> One danger might be a lockdep report triggered by
> raw_spin_lock(&console_owner_lock) itself. But it should be safe.
> lockdep is checked before the lock is actually taken
> and lockdep should disable itself before printing anything.
>
> Another danger might be any printk() called under the lock.
> The code just compares and assigns values to some variables
> (static, on stack) so we should be on the safe side.
>
> Well, I would feel more comfortable if we add printk_safe_enter_irqsave()
> back around the sections guarded by this lock. It can be done
> in a separate patch. The code looks safe at the moment.
You are correct. printk_safe should also be wrapping @console_owner_lock
locking.
>> @@ -2716,19 +2700,22 @@ void console_unlock(void)
>> * were to occur on another CPU, it may wait for this one to
>> * finish. This task can not be preempted if there is a
>> * waiter waiting to take over.
>> + *
>> + * Interrupts are disabled because the hand over to a waiter
>> + * must not be interrupted until the hand over is completed
>> + * (@console_waiter is cleared).
>> */
>> + local_irq_save(flags);
>> console_lock_spinning_enable();
>
> Same here. console_lock_spinning_enable() takes console_owner_lock.
> I would feel more comfortable if we added printk_safe_enter_irqsave(flags)
> inside console_lock_spinning_enable() around the locked code. The code
> is safe at the moment but...
Agreed.
>> stop_critical_timings(); /* don't trace print latency */
>> call_console_drivers(ext_text, ext_len, text, len);
>> start_critical_timings();
>>
>> - if (console_lock_spinning_disable_and_check()) {
>> - printk_safe_exit_irqrestore(flags);
>> + handover = console_lock_spinning_disable_and_check();
>
> Same here. Also console_lock_spinning_disable_and_check() takes
> console_owner_lock. It looks safe at the moment but...
Agreed.
>> --- a/kernel/printk/printk_safe.c
>> +++ b/kernel/printk/printk_safe.c
>> @@ -369,7 +70,10 @@ asmlinkage int vprintk(const char *fmt, va_list args)
>> * Use the main logbuf even in NMI. But avoid calling console
>> * drivers that might have their own locks.
>> */
>> - if ((this_cpu_read(printk_context) & PRINTK_NMI_DIRECT_CONTEXT_MASK)) {
>> + if (this_cpu_read(printk_context) &
>> + (PRINTK_NMI_DIRECT_CONTEXT_MASK |
>> + PRINTK_NMI_CONTEXT_MASK |
>> + PRINTK_SAFE_CONTEXT_MASK)) {
>> unsigned long flags;
>> int len;
>>
>
> There is the following code right below:
>
> printk_safe_enter_irqsave(flags);
> len = vprintk_store(0, LOGLEVEL_DEFAULT, NULL, fmt, args);
> printk_safe_exit_irqrestore(flags);
> defer_console_output();
> return len;
>
> printk_safe_enter_irqsave(flags) is not needed here. Any nested
> printk() ends here as well.
Ah, I missed that one. Good eye!
> Against this can be done in a separate patch. Well, the commit message
> mentions that the printk_safe context is removed everywhere except
> for the code manipulating console lock. But is it just a detail.
I would prefer a v4 with these fixes:
- wrap @console_owner_lock with printk_safe usage
- remove unnecessary printk_safe usage from printk_safe.c
- update commit message to say that safe context tracking is left in
place for both the console and console_owner locks
John Ogness
next prev parent reply other threads:[~2021-06-24 15:36 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-24 11:11 [PATCH printk v3 0/6] printk: remove safe buffers John Ogness
2021-06-24 11:11 ` [PATCH printk v3 1/6] lib/nmi_backtrace: explicitly serialize banner and regs John Ogness
2021-06-24 12:26 ` Petr Mladek
2021-06-24 11:11 ` [PATCH printk v3 2/6] printk: track/limit recursion John Ogness
2021-06-24 12:55 ` Petr Mladek
2021-06-24 11:11 ` [PATCH printk v3 3/6] printk: remove safe buffers John Ogness
2021-06-24 14:49 ` Petr Mladek
2021-06-24 15:35 ` John Ogness [this message]
2021-06-25 12:41 ` Petr Mladek
2021-06-24 11:11 ` [PATCH printk v3 4/6] printk: remove NMI tracking John Ogness
2021-06-25 12:36 ` Petr Mladek
2021-06-25 13:34 ` Russell King (Oracle)
2021-06-24 11:11 ` [PATCH printk v3 5/6] printk: convert @syslog_lock to mutex John Ogness
2021-06-24 11:11 ` [PATCH printk v3 6/6] printk: syslog: close window between wait and read John Ogness
2021-06-24 14:57 ` Petr Mladek
2021-06-24 15:25 ` Petr Mladek
2021-06-25 8:11 ` John Ogness
2021-06-25 14:55 ` Petr Mladek
2021-06-25 13:33 ` Steven Rostedt
2021-06-25 14:14 ` John Ogness
2021-06-28 14:35 ` Petr Mladek
2021-06-28 14:52 ` Steven Rostedt
2021-06-28 15:00 ` John Ogness
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=8735t7mg0z.fsf@jogness.linutronix.de \
--to=john.ogness@linutronix.de \
--cc=aik@ozlabs.ru \
--cc=akpm@linux-foundation.org \
--cc=benh@kernel.crashing.org \
--cc=christophe.leroy@csgroup.eu \
--cc=clg@kaod.org \
--cc=ebiederm@xmission.com \
--cc=huyue2@yulong.com \
--cc=keescook@chromium.org \
--cc=kexec@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=mpe@ellerman.id.au \
--cc=npiggin@gmail.com \
--cc=paulmck@kernel.org \
--cc=paulus@samba.org \
--cc=pmladek@suse.com \
--cc=rostedt@goodmis.org \
--cc=senozhatsky@chromium.org \
--cc=tglx@linutronix.de \
--cc=yangtiezhu@loongson.cn \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).