From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-10.3 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, INVALID_DATE_TZ_ABSURD,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id DB570C49EA5 for ; Thu, 24 Jun 2021 15:36:04 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id C3E3C613DA for ; Thu, 24 Jun 2021 15:36:04 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231194AbhFXPiW (ORCPT ); Thu, 24 Jun 2021 11:38:22 -0400 Received: from Galois.linutronix.de ([193.142.43.55]:45618 "EHLO galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230087AbhFXPiR (ORCPT ); Thu, 24 Jun 2021 11:38:17 -0400 From: John Ogness DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linutronix.de; s=2020; t=1624548957; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=cnGrieUbujWdjeESGTNL4zI8bpCaykCad9TLoO0NgzE=; b=X440mlyxOfY0l+O6I6VQVplfM6lJHk48nwLBwuBz6w5egyUun8KR9oAbbsSxx0G2tldbac +rDoNSprzi9uO1TNuUNSF/J1x11nrSbRwK6BRIs2HVPPJMGqszaFgInryCw4EqLL8m8ttt rSd+DMxUS3NAYGpAv073q2OQzdu6fV1J59wg9zOQotD8O3J5TC1iCrOjkGtJvWbWkUlY4t waWdCG/lR4XTehi++htdLi4sgfrz+GqV25zoAI3twp5xKa6e2whADE+wPFtI21fqE0b79x MUXbiAWCeXyXtdWMqlxMMYh1siZI/FNUEZ0zJCi8E9kbL8k/W6rLzBqAF5Uajw== DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=linutronix.de; s=2020e; t=1624548957; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=cnGrieUbujWdjeESGTNL4zI8bpCaykCad9TLoO0NgzE=; b=Sk5fVzsxRS3y1IRl2xYTV2TXNGf2kUa1cw+/mM4u06ApaxCKSAoHIvZFq5mBMNQXB8Rt6b nIDyXXDXd4fDCBBQ== To: Petr Mladek Cc: Sergey Senozhatsky , Steven Rostedt , Thomas Gleixner , linux-kernel@vger.kernel.org, Michael Ellerman , Benjamin Herrenschmidt , Paul Mackerras , Eric Biederman , Nicholas Piggin , Christophe Leroy , =?utf-8?Q?C=C3=A9dric?= Le Goater , Andrew Morton , Kees Cook , Tiezhu Yang , Yue Hu , Alexey Kardashevskiy , "Paul E. McKenney" , linuxppc-dev@lists.ozlabs.org, kexec@lists.infradead.org Subject: Re: [PATCH printk v3 3/6] printk: remove safe buffers In-Reply-To: References: <20210624111148.5190-1-john.ogness@linutronix.de> <20210624111148.5190-4-john.ogness@linutronix.de> Date: Thu, 24 Jun 2021 17:41:56 +0206 Message-ID: <8735t7mg0z.fsf@jogness.linutronix.de> MIME-Version: 1.0 Content-Type: text/plain Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2021-06-24, Petr Mladek 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