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.

Fixes: ea84b580b955 ("pstore: Convert buf_lock to semaphore")
Cc: stable@vger.kernel.org
Acked-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Jann Horn <jannh@google.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
Link: https://lore.kernel.org/r/20220314185953.2068993-1-jannh@google.com

diff --git a/drivers/firmware/efi/efi-pstore.c b/drivers/firmware/efi/efi-pstore.c
index 0ef086e43090bb14f3789466bedf0af049e3d18d..7e771c56c13c6194cbda0ac31a1359dd9d2d8d8f 100644 (file)
--- a/drivers/firmware/efi/efi-pstore.c
+++ b/drivers/firmware/efi/efi-pstore.c
@@ -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))

diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index f243cb5e6a4fb5a989f6de2355df982f543bd57a..e26162f102ffe51bd0cae29db0962e0886d97ac2 100644 (file)
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -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();

diff --git a/include/linux/pstore.h b/include/linux/pstore.h
index eb93a54cff31fa21d49b77012f14888b51e01bca..e97a8188f0fd8bcac0e0edd8a5570c7aa00edf9d 100644 (file)
--- a/include/linux/pstore.h
+++ b/include/linux/pstore.h
@@ -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;