FAILED: patch "[PATCH] pstore: Don't use semaphores in always-atomic-context code" failed to apply to 5.4-stable tree
Subsystems (3)
[email protected]
maintainerThe patch below does not apply to the 5.4-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.
thanks,
greg k-h
------------------ original commit in Linus's tree ------------------
From 8126b1c73108bc691f5643df19071a59a69d0bc6 Mon Sep 17 00:00:00 2001
From: Jann Horn <[email protected]>
Date: Mon, 14 Mar 2022 19:59:53 +0100
Subject: [PATCH] pstore: Don't use semaphores in always-atomic-context code
pstore_dump() is *always* invoked in atomic context (nowadays in an RCU read-side critical section, before that under a spinlock). It doesn't make sense to try to use semaphores here.
This is mostly a revert of commit ea84b580b955 ("pstore: Convert buf_lock to semaphore"), except that two parts aren't restored back exactly as they were:
- keep the lock initialization in pstore_register
- in efi_pstore_write(), always set the "block" flag to false
- omit "is_locked", that was unnecessary since
commit 959217c84c27 ("pstore: Actually give up during locking failure")
- fix the bailout message
The actual problem that the buggy commit was trying to address may have been that the use of preemptible() in efi_pstore_write() was wrong - it only looks at preempt_count() and the state of IRQs, but __rcu_read_lock() doesn't touch either of those under CONFIG_PREEMPT_RCU. (Sidenote: CONFIG_PREEMPT_RCU means that the scheduler can preempt tasks in RCU read-side critical sections, but you're not allowed to actively block/reschedule.)
Lockdep probably never caught the problem because it's very rare that you actually hit the contended case, so lockdep always just sees the down_trylock(), not the down_interruptible(), and so it can't tell that there's a problem.
drivers/firmware/efi/efi-pstore.c +1 -1
@@ -266,7 +266,7 @@ static int efi_pstore_write(struct pstore_record *record) efi_name[i] = name[i]; ret =efivar_entry_set_safe (efi_name, vendor,PSTORE_EFI_ATTRIBUTES , -preemptible (), record->size, record->psi->buf); + false, record->size, record->psi->buf);if (record->reason ==KMSG_DUMP_OOPS &&try_module_get (THIS_MODULE ))if (!schedule_work (&efivar_work))
fs/pstore/platform.c +18 -20
@@ -143,21 +143,22 @@ static void pstore_timer_kick(void)mod_timer (&pstore_timer, jiffies +msecs_to_jiffies (pstore_update_ms)); } -/* -* 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) +static 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;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 spinning on +* pstore_info::buf_lock. +*/ case KMSG_DUMP_EMERG :return true;default :
@@ -389,21 +390,19 @@ static void pstore_dump(struct kmsg_dumper *dumper,unsigned long total =0 ;const char *why;unsigned int part =1 ; +unsigned long flags =0 ;int ret; why =kmsg_dump_reason_str (reason); -if (down_trylock (&psinfo->buf_lock)) { -/* Failed to acquire lock: give up if we cannot wait. */ -if (pstore_cannot_wait (reason)) { -pr_err ("dump skipped in %s path: may corrupt error record\n" , -in_nmi () ?"NMI" : why); -return ; - } -if (down_interruptible (&psinfo->buf_lock)) { -pr_err ("could not grab semaphore?!\n" ); +if (pstore_cannot_block_path (reason)) { +if (!spin_trylock_irqsave (&psinfo->buf_lock, flags)) { +pr_err ("dump skipped in %s path because of concurrent dump\n" , +in_nmi () ?"NMI" : why);return ; } + }else { +spin_lock_irqsave (&psinfo->buf_lock, flags); }kmsg_dump_rewind (&iter);
@@ -467,8 +466,7 @@ static void pstore_dump(struct kmsg_dumper *dumper, total += record.size; part++; } - -up (&psinfo->buf_lock); +spin_unlock_irqrestore (&psinfo->buf_lock, flags); }static struct kmsg_dumper pstore_dumper = {
@@ -594,7 +592,7 @@ int pstore_register(struct pstore_info *psi) psi->write_user = pstore_write_user_compat; psinfo = psi;mutex_init (&psinfo->read_mutex); -sema_init (&psinfo->buf_lock,1 ); +spin_lock_init (&psinfo->buf_lock);if (psi->flags &PSTORE_FLAGS_DMESG )allocate_buf_for_compression ();
include/linux/pstore.h +3 -3
@@ -14,7 +14,7 @@#include <linux/errno.h> #include <linux/kmsg_dump.h> #include <linux/mutex.h> -#include <linux/semaphore.h> +#include <linux/spinlock.h> #include <linux/time.h> #include <linux/types.h>
@@ -87,7 +87,7 @@ struct pstore_record {* @owner: module which is responsible for this backend driver * @name: name of the backend driver * -* @buf_lock: semaphore to serialize access to @buf +* @buf_lock: spinlock to serialize access to @buf * @buf: preallocated crash dump buffer * @bufsize: size of @buf available for crash dump bytes (must match * smallest number of bytes available for writing to a
@@ -178,7 +178,7 @@ struct pstore_info {struct module *owner;const char *name; -struct semaphore buf_lock; +spinlock_t buf_lock;char *buf;size_t bufsize;