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 Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 0A156C433EF for ; Tue, 3 May 2022 18:11:33 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S241035AbiECSPE (ORCPT ); Tue, 3 May 2022 14:15:04 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:35714 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S241092AbiECSPC (ORCPT ); Tue, 3 May 2022 14:15:02 -0400 Received: from mail-oi1-x236.google.com (mail-oi1-x236.google.com [IPv6:2607:f8b0:4864:20::236]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id B5CE22A711 for ; Tue, 3 May 2022 11:11:29 -0700 (PDT) Received: by mail-oi1-x236.google.com with SMTP id a10so18974057oif.9 for ; Tue, 03 May 2022 11:11:29 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=R4/K6aedIUl9MohMPOlkW7bX2DydtcwCMb+CydErQh0=; b=CJu9kW5FihxnTA4wr5HdIEfymEuuekA79aJVNMM2F6kdV2+CmmuoX8R9fhQ+4vfzGu cwr0+95tIZHFde4YyKyVQIEU1uoccXZ0Wre6iNf7ZQo6N8EA9gsX+jTvyEJvXCE49T6e 235jqkHQ/du2SkGvAqn3TvfA6e2cYQxB3dcEA= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=R4/K6aedIUl9MohMPOlkW7bX2DydtcwCMb+CydErQh0=; b=F75rcwzI62+YtGHGz13r+OMYI1W3MyyCxyOzEStYRrhq0U6hhtGYyMA6JY2aLK8Qup M9ApZfTBh3rYLzGbLsRuzf4to4KwIlaNcmFSTNqPLEY8+PUjITwPQXiqRh/kwHKU65T6 IXeZxKMjqe9EROLR7eJdLseVZQFFs21d7ucM2VopDisWOT5fv3hZ/40aSsqfH9EVWgaE Ub6+/X+ZYir3y6tH/Rkz5oRkJhmXti6s24Nmq32GM5gb5aBz/fjcEvaeoZdKzjy2LGb6 OJhrecZ2549dFE+Yn/2JJiWWoSFEPE17Ate0200jqCUEAryBNJEFRoBoNVVBqYro59Yd B9Zg== X-Gm-Message-State: AOAM532j6P0uK95QM/QrhauYj3nyYwlVrgb3ctZ0g0ja3KHJddft6S2S 1gQG3KlFU1eOyO8lzULxhEWyKKDrYycT9w== X-Google-Smtp-Source: ABdhPJzfeb/KKPA0CEr21X1yS+sJrrst3jC1MlSvFs8YPePLO5hdAeobNO4T3Qz9lBeHD/+gdEIpmQ== X-Received: by 2002:a05:6808:193:b0:326:1d7f:24f7 with SMTP id w19-20020a056808019300b003261d7f24f7mr2376924oic.275.1651601489008; Tue, 03 May 2022 11:11:29 -0700 (PDT) Received: from mail-oi1-f176.google.com (mail-oi1-f176.google.com. [209.85.167.176]) by smtp.gmail.com with ESMTPSA id u3-20020a056870d58300b000e686d1389csm7719712oao.54.2022.05.03.11.11.28 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 03 May 2022 11:11:28 -0700 (PDT) Received: by mail-oi1-f176.google.com with SMTP id l16so11743617oil.6 for ; Tue, 03 May 2022 11:11:28 -0700 (PDT) X-Received: by 2002:a05:6808:d50:b0:322:fb1d:319d with SMTP id w16-20020a0568080d5000b00322fb1d319dmr2350498oik.174.1651601052906; Tue, 03 May 2022 11:04:12 -0700 (PDT) MIME-Version: 1.0 References: <20220427224924.592546-1-gpiccoli@igalia.com> <20220427224924.592546-5-gpiccoli@igalia.com> In-Reply-To: <20220427224924.592546-5-gpiccoli@igalia.com> From: Evan Green Date: Tue, 3 May 2022 11:03:37 -0700 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH 04/30] firmware: google: Convert regular spinlock into trylock on panic path To: "Guilherme G. Piccoli" Cc: Andrew Morton , bhe@redhat.com, pmladek@suse.com, kexec@lists.infradead.org, LKML , bcm-kernel-feedback-list@broadcom.com, coresight@lists.linaro.org, linuxppc-dev@lists.ozlabs.org, linux-alpha@vger.kernel.org, linux-arm Mailing List , linux-edac@vger.kernel.org, linux-hyperv@vger.kernel.org, linux-leds@vger.kernel.org, linux-mips@vger.kernel.org, linux-parisc@vger.kernel.org, Linux PM , linux-remoteproc@vger.kernel.org, linux-s390@vger.kernel.org, linux-tegra@vger.kernel.org, linux-um@lists.infradead.org, linux-xtensa@linux-xtensa.org, netdev@vger.kernel.org, openipmi-developer@lists.sourceforge.net, rcu@vger.kernel.org, sparclinux@vger.kernel.org, xen-devel@lists.xenproject.org, x86@kernel.org, kernel-dev@igalia.com, kernel@gpiccoli.net, halves@canonical.com, fabiomirmar@gmail.com, alejandro.j.jimenez@oracle.com, Andy Shevchenko , Arnd Bergmann , Borislav Petkov , Jonathan Corbet , d.hatayama@jp.fujitsu.com, dave.hansen@linux.intel.com, dyoung@redhat.com, feng.tang@intel.com, Greg Kroah-Hartman , mikelley@microsoft.com, hidehiro.kawai.ez@hitachi.com, jgross@suse.com, john.ogness@linutronix.de, Kees Cook , luto@kernel.org, mhiramat@kernel.org, mingo@redhat.com, paulmck@kernel.org, peterz@infradead.org, rostedt@goodmis.org, senozhatsky@chromium.org, Alan Stern , Thomas Gleixner , vgoyal@redhat.com, vkuznets@redhat.com, Will Deacon , Ard Biesheuvel , David Gow , Julius Werner Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-remoteproc@vger.kernel.org On Wed, Apr 27, 2022 at 3:51 PM Guilherme G. Piccoli wrote: > > Currently the gsmi driver registers a panic notifier as well as > reboot and die notifiers. The callbacks registered are called in > atomic and very limited context - for instance, panic disables > preemption, local IRQs and all other CPUs that aren't running the > current panic function. > > With that said, taking a spinlock in this scenario is a > dangerous invitation for a deadlock scenario. So, we fix > that in this commit by changing the regular spinlock with > a trylock, which is a safer approach. > > Fixes: 74c5b31c6618 ("driver: Google EFI SMI") > Cc: Ard Biesheuvel > Cc: David Gow > Cc: Evan Green > Cc: Julius Werner > Signed-off-by: Guilherme G. Piccoli > --- > drivers/firmware/google/gsmi.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/firmware/google/gsmi.c b/drivers/firmware/google/gsmi.c > index adaa492c3d2d..b01ed02e4a87 100644 > --- a/drivers/firmware/google/gsmi.c > +++ b/drivers/firmware/google/gsmi.c > @@ -629,7 +629,10 @@ static int gsmi_shutdown_reason(int reason) > if (saved_reason & (1 << reason)) > return 0; > > - spin_lock_irqsave(&gsmi_dev.lock, flags); > + if (!spin_trylock_irqsave(&gsmi_dev.lock, flags)) { > + rc = -EBUSY; > + goto out; > + } gsmi_shutdown_reason() is a common function called in other scenarios as well, like reboot and thermal trip, where it may still make sense to wait to acquire a spinlock. Maybe we should add a parameter to gsmi_shutdown_reason() so that you can get your change on panic, but we don't convert other callbacks into try-fail scenarios causing us to miss logs. Though thinking more about it, is this really a Good Change (TM)? The spinlock itself already disables interrupts, meaning the only case where this change makes a difference is if the panic happens from within the function that grabbed the spinlock (in which case the callback is also likely to panic), or in an NMI that panics within that window. The downside of this change is that if one core was politely working through an event with the lock held, and another core panics, we now might lose the panic log, even though it probably would have gone through fine assuming the other core has a chance to continue. -Evan 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 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 smtp.lore.kernel.org (Postfix) with ESMTPS id 2CBB3C433F5 for ; Tue, 3 May 2022 22:20:46 +0000 (UTC) Received: from boromir.ozlabs.org (localhost [IPv6:::1]) by lists.ozlabs.org (Postfix) with ESMTP id 4KtDth5wqrz3cDT for ; Wed, 4 May 2022 08:20:44 +1000 (AEST) Authentication-Results: lists.ozlabs.org; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=chromium.org header.i=@chromium.org header.a=rsa-sha256 header.s=google header.b=CJu9kW5F; dkim-atps=neutral Authentication-Results: lists.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=chromium.org (client-ip=2607:f8b0:4864:20::22a; helo=mail-oi1-x22a.google.com; envelope-from=evgreen@chromium.org; receiver=) Authentication-Results: lists.ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=chromium.org header.i=@chromium.org header.a=rsa-sha256 header.s=google header.b=CJu9kW5F; dkim-atps=neutral Received: from mail-oi1-x22a.google.com (mail-oi1-x22a.google.com [IPv6:2607:f8b0:4864:20::22a]) (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 4Kt7Bt1zc3z3bcv for ; Wed, 4 May 2022 04:04:19 +1000 (AEST) Received: by mail-oi1-x22a.google.com with SMTP id l16so11722833oil.6 for ; Tue, 03 May 2022 11:04:19 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=R4/K6aedIUl9MohMPOlkW7bX2DydtcwCMb+CydErQh0=; b=CJu9kW5FihxnTA4wr5HdIEfymEuuekA79aJVNMM2F6kdV2+CmmuoX8R9fhQ+4vfzGu cwr0+95tIZHFde4YyKyVQIEU1uoccXZ0Wre6iNf7ZQo6N8EA9gsX+jTvyEJvXCE49T6e 235jqkHQ/du2SkGvAqn3TvfA6e2cYQxB3dcEA= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=R4/K6aedIUl9MohMPOlkW7bX2DydtcwCMb+CydErQh0=; b=olqw/ZF6Vtre1td1WLHgm0ucC8G1jZ8B1U0o2TerlHY4d7b3/qureeOwe5kdKacLpu jYXTkjjzuLYOWp2xsmyP1rV5r7qQEE4/4K/BH9o07NVzkgcgoGMa29gj7210cMmXHhSt cqSivL0fdqgQpMih5FVLwdIqZLLnFZSgd36h2KV+vNpPR8adxsXo1Vk0fWmcxMu4JCAK M0AI+BKXRXzLm0EOyY+yvgoI65L53bnpETZ8fYrdcT3UzYisxw5rBQLd3gj6S+MTm68s +h+JZEYuLPm88+dl5EHgRmSrdekG2IG8xAPLXqIQWf8HLpMtH1jMnJQvzfvjCmqDF7zi ufGg== X-Gm-Message-State: AOAM531xJC5dEbHB2oQZsC0WgyHPC2PbnU3BPFLYwkFIV7kL02uNGJwE rKyWHXl+7D3Wf5wSqoLaO9GWG9ks49pBbg== X-Google-Smtp-Source: ABdhPJxgSl8yws5EFMkVq9AIlbZklQwcAHbYtOiYwGUX6AEMeKRzT6h8eR+Ork7YYbPl0uiqQzqnDg== X-Received: by 2002:a05:6808:1115:b0:2ec:e103:99c8 with SMTP id e21-20020a056808111500b002ece10399c8mr2308773oih.194.1651601054560; Tue, 03 May 2022 11:04:14 -0700 (PDT) Received: from mail-oi1-f181.google.com (mail-oi1-f181.google.com. [209.85.167.181]) by smtp.gmail.com with ESMTPSA id c20-20020a05687093d400b000e686d13898sm7178250oal.50.2022.05.03.11.04.13 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 03 May 2022 11:04:13 -0700 (PDT) Received: by mail-oi1-f181.google.com with SMTP id e189so18960385oia.8 for ; Tue, 03 May 2022 11:04:13 -0700 (PDT) X-Received: by 2002:a05:6808:d50:b0:322:fb1d:319d with SMTP id w16-20020a0568080d5000b00322fb1d319dmr2350498oik.174.1651601052906; Tue, 03 May 2022 11:04:12 -0700 (PDT) MIME-Version: 1.0 References: <20220427224924.592546-1-gpiccoli@igalia.com> <20220427224924.592546-5-gpiccoli@igalia.com> In-Reply-To: <20220427224924.592546-5-gpiccoli@igalia.com> From: Evan Green Date: Tue, 3 May 2022 11:03:37 -0700 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH 04/30] firmware: google: Convert regular spinlock into trylock on panic path To: "Guilherme G. Piccoli" Content-Type: text/plain; charset="UTF-8" X-Mailman-Approved-At: Wed, 04 May 2022 08:18:47 +1000 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: linux-hyperv@vger.kernel.org, halves@canonical.com, David Gow , linux-xtensa@linux-xtensa.org, peterz@infradead.org, alejandro.j.jimenez@oracle.com, linux-remoteproc@vger.kernel.org, feng.tang@intel.com, linux-mips@vger.kernel.org, hidehiro.kawai.ez@hitachi.com, sparclinux@vger.kernel.org, Will Deacon , Thomas Gleixner , linux-leds@vger.kernel.org, linux-s390@vger.kernel.org, mikelley@microsoft.com, john.ogness@linutronix.de, bhe@redhat.com, Jonathan Corbet , paulmck@kernel.org, fabiomirmar@gmail.com, x86@kernel.org, Ard Biesheuvel , mingo@redhat.com, bcm-kernel-feedback-list@broadcom.com, xen-devel@lists.xenproject.org, dyoung@redhat.com, vgoyal@redhat.com, pmladek@suse.com, dave.hansen@linux.intel.com, Kees Cook , Arnd Bergmann , Linux PM , coresight@lists.linaro.org, linux-um@lists.infradead.org, rostedt@goodmis.org, rcu@vger.kernel.org, Greg Kroah-Hartman , Borislav Petkov , luto@kernel.org, linux-tegra@vger.kernel.org, openipmi-developer@lists.sourceforge.net, Andy Shevchenko , vkuznets@redhat.com, linux-arm Mailing List , linux-edac@vger.kernel.org, jgross@suse.com, linux-parisc@vger.kernel.org, netdev@vger.kernel.org, kernel@gpiccoli.net, kexec@lists.infradead.org, LKML , Alan Stern , senozhatsky@chromium.org, d.hatayama@jp.fujitsu.com, mhiramat@kernel.org, kernel-dev@igalia.com, linux-alpha@vger.kernel.org, Julius Werner , Andrew Morton , linuxppc-dev@lists.ozlabs.org Errors-To: linuxppc-dev-bounces+linuxppc-dev=archiver.kernel.org@lists.ozlabs.org Sender: "Linuxppc-dev" On Wed, Apr 27, 2022 at 3:51 PM Guilherme G. Piccoli wrote: > > Currently the gsmi driver registers a panic notifier as well as > reboot and die notifiers. The callbacks registered are called in > atomic and very limited context - for instance, panic disables > preemption, local IRQs and all other CPUs that aren't running the > current panic function. > > With that said, taking a spinlock in this scenario is a > dangerous invitation for a deadlock scenario. So, we fix > that in this commit by changing the regular spinlock with > a trylock, which is a safer approach. > > Fixes: 74c5b31c6618 ("driver: Google EFI SMI") > Cc: Ard Biesheuvel > Cc: David Gow > Cc: Evan Green > Cc: Julius Werner > Signed-off-by: Guilherme G. Piccoli > --- > drivers/firmware/google/gsmi.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/firmware/google/gsmi.c b/drivers/firmware/google/gsmi.c > index adaa492c3d2d..b01ed02e4a87 100644 > --- a/drivers/firmware/google/gsmi.c > +++ b/drivers/firmware/google/gsmi.c > @@ -629,7 +629,10 @@ static int gsmi_shutdown_reason(int reason) > if (saved_reason & (1 << reason)) > return 0; > > - spin_lock_irqsave(&gsmi_dev.lock, flags); > + if (!spin_trylock_irqsave(&gsmi_dev.lock, flags)) { > + rc = -EBUSY; > + goto out; > + } gsmi_shutdown_reason() is a common function called in other scenarios as well, like reboot and thermal trip, where it may still make sense to wait to acquire a spinlock. Maybe we should add a parameter to gsmi_shutdown_reason() so that you can get your change on panic, but we don't convert other callbacks into try-fail scenarios causing us to miss logs. Though thinking more about it, is this really a Good Change (TM)? The spinlock itself already disables interrupts, meaning the only case where this change makes a difference is if the panic happens from within the function that grabbed the spinlock (in which case the callback is also likely to panic), or in an NMI that panics within that window. The downside of this change is that if one core was politely working through an event with the lock held, and another core panics, we now might lose the panic log, even though it probably would have gone through fine assuming the other core has a chance to continue. -Evan From mboxrd@z Thu Jan 1 00:00:00 1970 From: Evan Green Date: Tue, 3 May 2022 11:03:37 -0700 Subject: [PATCH 04/30] firmware: google: Convert regular spinlock into trylock on panic path In-Reply-To: <20220427224924.592546-5-gpiccoli@igalia.com> References: <20220427224924.592546-1-gpiccoli@igalia.com> <20220427224924.592546-5-gpiccoli@igalia.com> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: kexec@lists.infradead.org On Wed, Apr 27, 2022 at 3:51 PM Guilherme G. Piccoli wrote: > > Currently the gsmi driver registers a panic notifier as well as > reboot and die notifiers. The callbacks registered are called in > atomic and very limited context - for instance, panic disables > preemption, local IRQs and all other CPUs that aren't running the > current panic function. > > With that said, taking a spinlock in this scenario is a > dangerous invitation for a deadlock scenario. So, we fix > that in this commit by changing the regular spinlock with > a trylock, which is a safer approach. > > Fixes: 74c5b31c6618 ("driver: Google EFI SMI") > Cc: Ard Biesheuvel > Cc: David Gow > Cc: Evan Green > Cc: Julius Werner > Signed-off-by: Guilherme G. Piccoli > --- > drivers/firmware/google/gsmi.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/firmware/google/gsmi.c b/drivers/firmware/google/gsmi.c > index adaa492c3d2d..b01ed02e4a87 100644 > --- a/drivers/firmware/google/gsmi.c > +++ b/drivers/firmware/google/gsmi.c > @@ -629,7 +629,10 @@ static int gsmi_shutdown_reason(int reason) > if (saved_reason & (1 << reason)) > return 0; > > - spin_lock_irqsave(&gsmi_dev.lock, flags); > + if (!spin_trylock_irqsave(&gsmi_dev.lock, flags)) { > + rc = -EBUSY; > + goto out; > + } gsmi_shutdown_reason() is a common function called in other scenarios as well, like reboot and thermal trip, where it may still make sense to wait to acquire a spinlock. Maybe we should add a parameter to gsmi_shutdown_reason() so that you can get your change on panic, but we don't convert other callbacks into try-fail scenarios causing us to miss logs. Though thinking more about it, is this really a Good Change (TM)? The spinlock itself already disables interrupts, meaning the only case where this change makes a difference is if the panic happens from within the function that grabbed the spinlock (in which case the callback is also likely to panic), or in an NMI that panics within that window. The downside of this change is that if one core was politely working through an event with the lock held, and another core panics, we now might lose the panic log, even though it probably would have gone through fine assuming the other core has a chance to continue. -Evan From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oi1-x235.google.com ([2607:f8b0:4864:20::235]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1nlwyB-007A0a-Fw for linux-um@lists.infradead.org; Tue, 03 May 2022 18:09:56 +0000 Received: by mail-oi1-x235.google.com with SMTP id m25so5082494oih.2 for ; Tue, 03 May 2022 11:09:55 -0700 (PDT) Received: from mail-oi1-f173.google.com (mail-oi1-f173.google.com. [209.85.167.173]) by smtp.gmail.com with ESMTPSA id v4-20020a9d4e84000000b006060322124bsm4267882otk.27.2022.05.03.11.09.54 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 03 May 2022 11:09:54 -0700 (PDT) Received: by mail-oi1-f173.google.com with SMTP id l16so11739158oil.6 for ; Tue, 03 May 2022 11:09:54 -0700 (PDT) MIME-Version: 1.0 References: <20220427224924.592546-1-gpiccoli@igalia.com> <20220427224924.592546-5-gpiccoli@igalia.com> In-Reply-To: <20220427224924.592546-5-gpiccoli@igalia.com> From: Evan Green Date: Tue, 3 May 2022 11:03:37 -0700 Message-ID: Subject: Re: [PATCH 04/30] firmware: google: Convert regular spinlock into trylock on panic path List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-um" Errors-To: linux-um-bounces+geert=linux-m68k.org@lists.infradead.org To: "Guilherme G. Piccoli" Cc: Andrew Morton , bhe@redhat.com, pmladek@suse.com, kexec@lists.infradead.org, LKML , bcm-kernel-feedback-list@broadcom.com, coresight@lists.linaro.org, linuxppc-dev@lists.ozlabs.org, linux-alpha@vger.kernel.org, linux-arm Mailing List , linux-edac@vger.kernel.org, linux-hyperv@vger.kernel.org, linux-leds@vger.kernel.org, linux-mips@vger.kernel.org, linux-parisc@vger.kernel.org, Linux PM , linux-remoteproc@vger.kernel.org, linux-s390@vger.kernel.org, linux-tegra@vger.kernel.org, linux-um@lists.infradead.org, linux-xtensa@linux-xtensa.org, netdev@vger.kernel.org, openipmi-developer@lists.sourceforge.net, rcu@vger.kernel.org, sparclinux@vger.kernel.org, xen-devel@lists.xenproject.org, x86@kernel.org, kernel-dev@igalia.com, kernel@gpiccoli.net, halves@canonical.com, fabiomirmar@gmail.com, alejandro.j.jimenez@oracle.com, Andy Shevchenko , Arnd Bergmann , Borislav Petkov , Jonathan Corbet , d.hatayama@jp.fujitsu.com, dave.hansen@linux.intel.com, dyoung@redhat.com, feng.tang@intel.com, Greg Kroah-Hartman , mikelley@microsoft.com, hidehiro.kawai.ez@hitachi.com, jgross@suse.com, john.ogness@linutronix.de, Kees Cook , luto@kernel.org, mhiramat@kernel.org, mingo@redhat.com, paulmck@kernel.org, peterz@infradead.org, rostedt@goodmis.org, senozhatsky@chromium.org, Alan Stern , Thomas Gleixner , vgoyal@redhat.com, vkuznets@redhat.com, Will Deacon , Ard Biesheuvel , David Gow , Julius Werner On Wed, Apr 27, 2022 at 3:51 PM Guilherme G. Piccoli wrote: > > Currently the gsmi driver registers a panic notifier as well as > reboot and die notifiers. The callbacks registered are called in > atomic and very limited context - for instance, panic disables > preemption, local IRQs and all other CPUs that aren't running the > current panic function. > > With that said, taking a spinlock in this scenario is a > dangerous invitation for a deadlock scenario. So, we fix > that in this commit by changing the regular spinlock with > a trylock, which is a safer approach. > > Fixes: 74c5b31c6618 ("driver: Google EFI SMI") > Cc: Ard Biesheuvel > Cc: David Gow > Cc: Evan Green > Cc: Julius Werner > Signed-off-by: Guilherme G. Piccoli > --- > drivers/firmware/google/gsmi.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/firmware/google/gsmi.c b/drivers/firmware/google/gsmi.c > index adaa492c3d2d..b01ed02e4a87 100644 > --- a/drivers/firmware/google/gsmi.c > +++ b/drivers/firmware/google/gsmi.c > @@ -629,7 +629,10 @@ static int gsmi_shutdown_reason(int reason) > if (saved_reason & (1 << reason)) > return 0; > > - spin_lock_irqsave(&gsmi_dev.lock, flags); > + if (!spin_trylock_irqsave(&gsmi_dev.lock, flags)) { > + rc = -EBUSY; > + goto out; > + } gsmi_shutdown_reason() is a common function called in other scenarios as well, like reboot and thermal trip, where it may still make sense to wait to acquire a spinlock. Maybe we should add a parameter to gsmi_shutdown_reason() so that you can get your change on panic, but we don't convert other callbacks into try-fail scenarios causing us to miss logs. Though thinking more about it, is this really a Good Change (TM)? The spinlock itself already disables interrupts, meaning the only case where this change makes a difference is if the panic happens from within the function that grabbed the spinlock (in which case the callback is also likely to panic), or in an NMI that panics within that window. The downside of this change is that if one core was politely working through an event with the lock held, and another core panics, we now might lose the panic log, even though it probably would have gone through fine assuming the other core has a chance to continue. -Evan _______________________________________________ linux-um mailing list linux-um@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-um From mboxrd@z Thu Jan 1 00:00:00 1970 From: Evan Green Subject: Re: [PATCH 04/30] firmware: google: Convert regular spinlock into trylock on panic path Date: Tue, 3 May 2022 11:03:37 -0700 Message-ID: References: <20220427224924.592546-1-gpiccoli@igalia.com> <20220427224924.592546-5-gpiccoli@igalia.com> Mime-Version: 1.0 Return-path: DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=R4/K6aedIUl9MohMPOlkW7bX2DydtcwCMb+CydErQh0=; b=CJu9kW5FihxnTA4wr5HdIEfymEuuekA79aJVNMM2F6kdV2+CmmuoX8R9fhQ+4vfzGu cwr0+95tIZHFde4YyKyVQIEU1uoccXZ0Wre6iNf7ZQo6N8EA9gsX+jTvyEJvXCE49T6e 235jqkHQ/du2SkGvAqn3TvfA6e2cYQxB3dcEA= In-Reply-To: <20220427224924.592546-5-gpiccoli@igalia.com> List-ID: Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: "Guilherme G. Piccoli" Cc: Andrew Morton , bhe@redhat.com, pmladek@suse.com, kexec@lists.infradead.org, LKML , bcm-kernel-feedback-list@broadcom.com, coresight@lists.linaro.org, linuxppc-dev@lists.ozlabs.org, linux-alpha@vger.kernel.org, linux-arm Mailing List , linux-edac@vger.kernel.org, linux-hyperv@vger.kernel.org, linux-leds@vger.kernel.org, linux-mips@vger.kernel.org, linux-parisc@vger.kernel.org, Linux PM , linux-remoteproc@vger.kernel.org, linux-s390@vger.kernel.org, linux-tegra@vger.kernel.org, linux-um@lists.infradead.org, linux-xtensa@linux-xtensa.org, netdev@vger.kernel.org, openipmi-developer@lists.sourceforge.net, rcu@vger.kernel.org, sparclinux@vger.kernel.org, xen-devel@lis On Wed, Apr 27, 2022 at 3:51 PM Guilherme G. Piccoli wrote: > > Currently the gsmi driver registers a panic notifier as well as > reboot and die notifiers. The callbacks registered are called in > atomic and very limited context - for instance, panic disables > preemption, local IRQs and all other CPUs that aren't running the > current panic function. > > With that said, taking a spinlock in this scenario is a > dangerous invitation for a deadlock scenario. So, we fix > that in this commit by changing the regular spinlock with > a trylock, which is a safer approach. > > Fixes: 74c5b31c6618 ("driver: Google EFI SMI") > Cc: Ard Biesheuvel > Cc: David Gow > Cc: Evan Green > Cc: Julius Werner > Signed-off-by: Guilherme G. Piccoli > --- > drivers/firmware/google/gsmi.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/firmware/google/gsmi.c b/drivers/firmware/google/gsmi.c > index adaa492c3d2d..b01ed02e4a87 100644 > --- a/drivers/firmware/google/gsmi.c > +++ b/drivers/firmware/google/gsmi.c > @@ -629,7 +629,10 @@ static int gsmi_shutdown_reason(int reason) > if (saved_reason & (1 << reason)) > return 0; > > - spin_lock_irqsave(&gsmi_dev.lock, flags); > + if (!spin_trylock_irqsave(&gsmi_dev.lock, flags)) { > + rc = -EBUSY; > + goto out; > + } gsmi_shutdown_reason() is a common function called in other scenarios as well, like reboot and thermal trip, where it may still make sense to wait to acquire a spinlock. Maybe we should add a parameter to gsmi_shutdown_reason() so that you can get your change on panic, but we don't convert other callbacks into try-fail scenarios causing us to miss logs. Though thinking more about it, is this really a Good Change (TM)? The spinlock itself already disables interrupts, meaning the only case where this change makes a difference is if the panic happens from within the function that grabbed the spinlock (in which case the callback is also likely to panic), or in an NMI that panics within that window. The downside of this change is that if one core was politely working through an event with the lock held, and another core panics, we now might lose the panic log, even though it probably would have gone through fine assuming the other core has a chance to continue. -Evan