Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 18 Feb 2022 18:49:23 +0100
From:   Sebastian Andrzej Siewior <bigeasy@linutronix.de>
To:     Jann Horn <jannh@google.com>
Cc:     Kees Cook <keescook@chromium.org>,
        Anton Vorontsov <anton@enomsg.org>,
        Colin Cross <ccross@android.com>,
        Tony Luck <tony.luck@intel.com>,
        Ard Biesheuvel <ardb@kernel.org>, linux-efi@vger.kernel.org,
        linux-kernel@vger.kernel.org, stable@vger.kernel.org
Subject: Re: [PATCH] pstore: Don't use semaphores in always-atomic-context
 code
Message-ID: <Yg/cIzXsya8dNLYp@linutronix.de>
References: <20220218172901.1425605-1-jannh@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
In-Reply-To: <20220218172901.1425605-1-jannh@google.com>
Precedence: bulk

On 2022-02-18 18:29:01 [+0100], Jann Horn wrote:
=E2=80=A6
> Lockdep probably never caught the problem because it's very rare that you
> actually hit the contended case, and lockdep can't check a

I appears that after =C2=B4a' a few words would follow.

> Fixes: ea84b580b955 ("pstore: Convert buf_lock to semaphore")
> Cc: stable@vger.kernel.org
> Signed-off-by: Jann Horn <jannh@google.com>
> ---
> Testing on 5.15.24 (latest stable), with CONFIG_PREEMPT=3Dy, when I trigg=
er
> a BUG() via LKDTM ("echo BUG > /sys/kernel/debug/provoke-crash/DIRECT"),
> I first get the expected BUG splat, followed by this RCU warning:
>=20
> Voluntary context switch within RCU read-side critical section!

Right, scheduling not allowed within a RCU read-side section. I'm not
sure if lockdep cares but if you enable CONFIG_DEBUG_ATOMIC_SLEEP then
the might_sleep() will complain if you are in a RCU section.

> This patch makes the RCU context warning go away.

Makes sense.
Acked-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

> --- a/fs/pstore/platform.c
> +++ b/fs/pstore/platform.c
> @@ -143,27 +143,26 @@ static void pstore_timer_kick(void)
>  	mod_timer(&pstore_timer, jiffies + msecs_to_jiffies(pstore_update_ms));
>  }
> =20
> -/*
> - * Should pstore_dump() wait for a concurrent pstore_dump()? If
> - * not, the current pstore_dump() will report a failure to dump
> - * and return.
> - */
> -static bool pstore_cannot_wait(enum kmsg_dump_reason reason)
> +bool pstore_cannot_block_path(enum kmsg_dump_reason reason)
>  {
> -	/* In NMI path, pstore shouldn't block regardless of reason. */
> +	/*
> +	 * In case of NMI path, pstore shouldn't be blocked
> +	 * regardless of reason.
> +	 */
>  	if (in_nmi())
>  		return true;
> =20
>  	switch (reason) {
>  	/* In panic case, other cpus are stopped by smp_send_stop(). */
>  	case KMSG_DUMP_PANIC:
> -	/* Emergency restart shouldn't be blocked. */
> +	/* Emergency restart shouldn't be blocked by spin lock. */

 by spinning on pstore_info::buf_lock

>  	case KMSG_DUMP_EMERG:
>  		return true;
>  	default:
>  		return false;
>  	}
>  }
> +EXPORT_SYMBOL_GPL(pstore_cannot_block_path);
> =20
>  #if IS_ENABLED(CONFIG_PSTORE_DEFLATE_COMPRESS)
>  static int zbufsize_deflate(size_t size)

Sebastian