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 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=-8.1 required=3.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, INVALID_DATE_TZ_ABSURD,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED autolearn=no 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 7BD34C49EA6 for ; Thu, 24 Jun 2021 15:36:36 +0000 (UTC) Received: from lists.ozlabs.org (lists.ozlabs.org [112.213.38.117]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id CE90B613EB for ; Thu, 24 Jun 2021 15:36:35 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org CE90B613EB Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linutronix.de Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=linuxppc-dev-bounces+linuxppc-dev=archiver.kernel.org@lists.ozlabs.org Received: from boromir.ozlabs.org (localhost [IPv6:::1]) by lists.ozlabs.org (Postfix) with ESMTP id 4G9kkq1jTJz30B3 for ; Fri, 25 Jun 2021 01:36:35 +1000 (AEST) Authentication-Results: lists.ozlabs.org; dkim=fail reason="signature verification failed" (2048-bit key; secure) header.d=linutronix.de header.i=@linutronix.de header.a=rsa-sha256 header.s=2020 header.b=X440mlyx; dkim=fail reason="signature verification failed" header.d=linutronix.de header.i=@linutronix.de header.a=ed25519-sha256 header.s=2020e header.b=Sk5fVzsx; dkim-atps=neutral Authentication-Results: lists.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=linutronix.de (client-ip=2a0a:51c0:0:12e:550::1; helo=galois.linutronix.de; envelope-from=john.ogness@linutronix.de; receiver=) Authentication-Results: lists.ozlabs.org; dkim=pass (2048-bit key; secure) header.d=linutronix.de header.i=@linutronix.de header.a=rsa-sha256 header.s=2020 header.b=X440mlyx; dkim=pass header.d=linutronix.de header.i=@linutronix.de header.a=ed25519-sha256 header.s=2020e header.b=Sk5fVzsx; dkim-atps=neutral X-Greylist: delayed 15850 seconds by postgrey-1.36 at boromir; Fri, 25 Jun 2021 01:36:08 AEST Received: from galois.linutronix.de (Galois.linutronix.de [IPv6:2a0a:51c0:0:12e:550::1]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 4G9kkJ1Rp6z3c02 for ; Fri, 25 Jun 2021 01:36:07 +1000 (AEST) 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 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 X-BeenThere: linuxppc-dev@lists.ozlabs.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Kees Cook , "Paul E. McKenney" , Alexey Kardashevskiy , Nicholas Piggin , linux-kernel@vger.kernel.org, Steven Rostedt , kexec@lists.infradead.org, Sergey Senozhatsky , Yue Hu , Paul Mackerras , Eric Biederman , Thomas Gleixner , linuxppc-dev@lists.ozlabs.org, Andrew Morton , Tiezhu Yang , =?utf-8?Q?C=C3=A9dric?= Le Goater Errors-To: linuxppc-dev-bounces+linuxppc-dev=archiver.kernel.org@lists.ozlabs.org Sender: "Linuxppc-dev" 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 From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from galois.linutronix.de ([2a0a:51c0:0:12e:550::1]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1lwROZ-00FGxq-S4 for kexec@lists.infradead.org; Thu, 24 Jun 2021 15:36:01 +0000 From: John Ogness 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 List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "kexec" Errors-To: kexec-bounces+dwmw2=infradead.org@lists.infradead.org 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 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 _______________________________________________ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec