pstore: Convert buf_lock to semaphore

PATCH In review Kees Cook 4 participants 12 messages Nov 30, 2018 – Dec 21, 2018

Subsystems (5)

PowerPC ACPI EFI ACPI APEI +1 more subsystems

lore

Share & tools

lore

Kees Cook

author

2018-11-30 22:47 UTC #0 lore raw

PATCH 1/1 pstore: Convert buf_lock to semaphore

Instead of running with interrupts disabled, use a semaphore. This should make it easier for backends that may need to sleep (e.g. EFI) when performing a write:

|BUG: sleeping function called from invalid context at kernel/sched/completion.c:99
|in_atomic(): 1, irqs_disabled(): 1, pid: 2236, name: sig-xstate-bum
|Preemption disabled at:
|[<ffffffff99d60512>] pstore_dump+0x72/0x330
|CPU: 26 PID: 2236 Comm: sig-xstate-bum Tainted: G      D           4.20.0-rc3 #45
|Call Trace:
| dump_stack+0x4f/0x6a
| ___might_sleep.cold.91+0xd3/0xe4
| __might_sleep+0x50/0x90
| wait_for_completion+0x32/0x130
| virt_efi_query_variable_info+0x14e/0x160
| efi_query_variable_store+0x51/0x1a0
| efivar_entry_set_safe+0xa3/0x1b0
| efi_pstore_write+0x109/0x140
| pstore_dump+0x11c/0x330
| kmsg_dump+0xa4/0xd0
| oops_exit+0x22/0x30

...

Reported-by Sebastian Andrzej Siewior <[email protected]>

Fixes 21b3ddd39fee ("efi: Don't use spinlocks for efi vars")

Signed-off-by Kees Cook <[email protected]>

---
 arch/powerpc/kernel/nvram_64.c    |  2 --
 drivers/acpi/apei/erst.c          |  1 -
 drivers/firmware/efi/efi-pstore.c |  4 +--
 fs/pstore/platform.c              | 41 +++++++++++++++----------------
 fs/pstore/ram.c                   |  1 -
 include/linux/pstore.h            |  7 +++---
 6 files changed, 24 insertions(+), 32 deletions(-)

arch/powerpc/kernel/nvram_64.c +0 -2

index 22e9d281324d..e7d4ce6964ae 100644
@@ -563,8 +563,6 @@ static int nvram_pstore_init(void)
 	nvram_pstore_info.buf = oops_data;
 	nvram_pstore_info.bufsize = oops_data_sz;
 
-	spin_lock_init(&nvram_pstore_info.buf_lock);
-
 	rc = pstore_register(&nvram_pstore_info);
 	if (rc && (rc != -EPERM))
 		/* Print error only when pstore.backend == nvram */

drivers/acpi/apei/erst.c +0 -1

index a5e1d963208e..9953e50667ec 100644
@@ -1176,7 +1176,6 @@ static int __init erst_init(void)
 	"Error Record Serialization Table (ERST) support is initialized.\n");
 
 	buf = kmalloc(erst_erange.size, GFP_KERNEL);
-	spin_lock_init(&erst_info.buf_lock);
 	if (buf) {
 		erst_info.buf = buf + sizeof(struct cper_pstore_record);
 		erst_info.bufsize = erst_erange.size -

drivers/firmware/efi/efi-pstore.c +1 -3

index cfe87b465819..0f7d97917197 100644
@@ -259,8 +259,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,
-			      !pstore_cannot_block_path(record->reason),
-			      record->size, record->psi->buf);
+			      preemptible(), record->size, record->psi->buf);
 
 	if (record->reason == KMSG_DUMP_OOPS)
 		efivar_run_worker();

@@ -369,7 +368,6 @@ static __init int efivars_pstore_init(void)
 		return -ENOMEM;
 
 	efi_pstore_info.bufsize = 1024;
-	spin_lock_init(&efi_pstore_info.buf_lock);
 
 	if (pstore_register(&efi_pstore_info)) {
 		kfree(efi_pstore_info.buf);

fs/pstore/platform.c +20 ~~-21~~

97 lines not on this page · raw message

fs/pstore/ram.c +0 -1

index 202eaa82bcc6..e6d9560ea455 100644
@@ -815,7 +815,6 @@ static int ramoops_probe(struct platform_device *pdev)
 		err = -ENOMEM;
 		goto fail_clear;
 	}
-	spin_lock_init(&cxt->pstore.buf_lock);
 
 	cxt->pstore.flags = PSTORE_FLAGS_DMESG;
 	if (cxt->console_size)

include/linux/pstore.h +3 -4

index a9ec285d85d1..b146181e8709 100644
@@ -26,7 +26,7 @@
 #include <linux/errno.h>
 #include <linux/kmsg_dump.h>
 #include <linux/mutex.h>
-#include <linux/spinlock.h>
+#include <linux/semaphore.h>
 #include <linux/time.h>
 #include <linux/types.h>

@@ -99,7 +99,7 @@ struct pstore_record {
  * @owner:	module which is responsible for this backend driver
  * @name:	name of the backend driver
  *
- * @buf_lock:	spinlock to serialize access to @buf
+ * @buf_lock:	semaphore 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

@@ -184,7 +184,7 @@ struct pstore_info {
 	struct module	*owner;
 	char		*name;
 
-	spinlock_t	buf_lock;
+	struct semaphore buf_lock;
 	char		*buf;
 	size_t		bufsize;

@@ -210,7 +210,6 @@ struct pstore_info {
 
 extern int pstore_register(struct pstore_info *);
 extern void pstore_unregister(struct pstore_info *);
-extern bool pstore_cannot_block_path(enum kmsg_dump_reason reason);
 
 struct pstore_ftrace_record {
 	unsigned long ip;

Signature · 5 lines

2.17.1


-- 
Kees Cook

Arnd Bergmann

2018-11-30 22:51 UTC #1 lore raw

Quoted 22 lines · Kees Cook

Instead of running with interrupts disabled, use a semaphore. This should make it easier for backends that may need to sleep (e.g. EFI) when performing a write:

|BUG: sleeping function called from invalid context at kernel/sched/completion.c:99
|in_atomic(): 1, irqs_disabled(): 1, pid: 2236, name: sig-xstate-bum
|Preemption disabled at:
|[<ffffffff99d60512>] pstore_dump+0x72/0x330
|CPU: 26 PID: 2236 Comm: sig-xstate-bum Tainted: G      D           4.20.0-rc3 #45
|Call Trace:
| dump_stack+0x4f/0x6a
| ___might_sleep.cold.91+0xd3/0xe4
| __might_sleep+0x50/0x90
| wait_for_completion+0x32/0x130
| virt_efi_query_variable_info+0x14e/0x160
| efi_query_variable_store+0x51/0x1a0
| efivar_entry_set_safe+0xa3/0x1b0
| efi_pstore_write+0x109/0x140
| pstore_dump+0x11c/0x330
| kmsg_dump+0xa4/0xd0
| oops_exit+0x22/0x30

Quoted 5 lines

...
Reported-by Sebastian Andrzej Siewior <[email protected]>
Fixes 21b3ddd39fee ("efi: Don't use spinlocks for efi vars")
Signed-off-by Kees Cook <[email protected]>

Hmm, I've actually been working on a patch set recently to deprecate all semaphores from the kernel and replace them with something else as much as possible.

Why can't this be a mutex instead?

Arnd

Kees Cook

author

2018-12-01 02:42 UTC #2 lore raw

Quoted 30 lines · Arnd Bergmann

Quoted 22 lines · Kees Cook
Instead of running with interrupts disabled, use a semaphore. This should make it easier for backends that may need to sleep (e.g. EFI) when performing a write:
|BUG: sleeping function called from invalid context at kernel/sched/completion.c:99
|in_atomic(): 1, irqs_disabled(): 1, pid: 2236, name: sig-xstate-bum
|Preemption disabled at:
|[<ffffffff99d60512>] pstore_dump+0x72/0x330
|CPU: 26 PID: 2236 Comm: sig-xstate-bum Tainted: G      D           4.20.0-rc3 #45
|Call Trace:
| dump_stack+0x4f/0x6a
| ___might_sleep.cold.91+0xd3/0xe4
| __might_sleep+0x50/0x90
| wait_for_completion+0x32/0x130
| virt_efi_query_variable_info+0x14e/0x160
| efi_query_variable_store+0x51/0x1a0
| efivar_entry_set_safe+0xa3/0x1b0
| efi_pstore_write+0x109/0x140
| pstore_dump+0x11c/0x330
| kmsg_dump+0xa4/0xd0
| oops_exit+0x22/0x30
Quoted 5 lines
...
Reported-by Sebastian Andrzej Siewior <[email protected]>
Fixes 21b3ddd39fee ("efi: Don't use spinlocks for efi vars")
Signed-off-by Kees Cook <[email protected]>

Quoted 5 lines

Hmm, I've actually been working on a patch set recently to deprecate all semaphores from the kernel and replace them with something else as much as possible.
Why can't this be a mutex instead?

My understanding is that I can't use a mutex in interrupt context (Documentation/kernel-hacking/locking.rst) and pstore_dump() needs to handle being called from anywhere. I'm surprised it's managed to get away with using a spinlock for this long. :P

-Kees

Signature · 1 line

Kees Cook

Arnd Bergmann

2018-12-01 08:42 UTC #3 lore raw

Quoted 30 lines · Kees Cook

Quoted 29 lines · Arnd Bergmann
Quoted 18 lines · Kees Cook
|BUG: sleeping function called from invalid context at kernel/sched/completion.c:99
|in_atomic(): 1, irqs_disabled(): 1, pid: 2236, name: sig-xstate-bum
|Preemption disabled at:
|[<ffffffff99d60512>] pstore_dump+0x72/0x330
|CPU: 26 PID: 2236 Comm: sig-xstate-bum Tainted: G      D           4.20.0-rc3 #45
|Call Trace:
| dump_stack+0x4f/0x6a
| ___might_sleep.cold.91+0xd3/0xe4
| __might_sleep+0x50/0x90
| wait_for_completion+0x32/0x130
| virt_efi_query_variable_info+0x14e/0x160
| efi_query_variable_store+0x51/0x1a0
| efivar_entry_set_safe+0xa3/0x1b0
| efi_pstore_write+0x109/0x140
| pstore_dump+0x11c/0x330
| kmsg_dump+0xa4/0xd0
| oops_exit+0x22/0x30
Quoted 5 lines
...
Reported-by Sebastian Andrzej Siewior <[email protected]>
Fixes 21b3ddd39fee ("efi: Don't use spinlocks for efi vars")
Signed-off-by Kees Cook <[email protected]>
Hmm, I've actually been working on a patch set recently to deprecate all semaphores from the kernel and replace them with something else as much as possible.

Quoted 6 lines

Quoted 1 line
Why can't this be a mutex instead?
My understanding is that I can't use a mutex in interrupt context (Documentation/kernel-hacking/locking.rst) and pstore_dump() needs to handle being called from anywhere. I'm surprised it's managed to get away with using a spinlock for this long. :P

You are right that you can't take (or release) a mutex from interrupt context. However, I don't think converting a spinlock to a semaphore is going to help here either.

spinlock (or raw_spinlock) is generally the only thing that can be the innermost lock that you take in any atomic context, and using down_trylock doesn't make the context less atomic than it already is.

virt_efi_query_variable_info() however waits for a completion and a semaphore, so that must not be called in atomic context. Holding a semaphore instead of a spinlock is not going to help you here, since the interrupt context means you might already be holding arbitrary locks.

Arnd

Sebastian Andrzej Siewior

2018-12-03 11:18 UTC #4 lore raw

Quoted 3 lines · Arnd Bergmann

You are right that you can't take (or release) a mutex from interrupt context. However, I don't think converting a spinlock to a semaphore is going to help here either.

you can acquire a semaphore with a try_lock from interrupts context but you can't do that with a mutex. You can also a acquire a semaphore in one context and release in another. Not that I'm a fan of those things but those two are often the reasons why people stick with a semaphore.

I haven't looked a general picture yet, will try to do so later today or tomorrow.

Quoted 1 line

Arnd

Sebastian

Arnd Bergmann

2018-12-03 16:26 UTC #5 lore raw

Quoted 9 lines · Sebastian Andrzej Siewior

Quoted 3 lines · Arnd Bergmann
You are right that you can't take (or release) a mutex from interrupt context. However, I don't think converting a spinlock to a semaphore is going to help here either.
you can acquire a semaphore with a try_lock from interrupts context but you can't do that with a mutex. You can also a acquire a semaphore in one context and release in another.

Right, that is the obvious part.

Quoted 2 lines

I haven't looked a general picture yet, will try to do so later today or tomorrow.

To speed this up, the problem I'm referring to is in virt_efi_query_variable_info() and efi_queue_work(), as in the original BUG_ON() that Kees quoted:

Quoted 9 lines

BUG: sleeping function called from invalid context at kernel/sched/completion.c:99
|in_atomic(): 1, irqs_disabled(): 1, pid: 2236, name: sig-xstate-bum
|Preemption disabled at:
|[<ffffffff99d60512>] pstore_dump+0x72/0x330
|CPU: 26 PID: 2236 Comm: sig-xstate-bum Tainted: G      D           4.20.0-rc3 #45
|Call Trace:
| dump_stack+0x4f/0x6a
| ___might_sleep.cold.91+0xd3/0xe4
| __might_sleep+0x50/0x90

Quoted 8 lines

| wait_for_completion+0x32/0x130
| virt_efi_query_variable_info+0x14e/0x160
| efi_query_variable_store+0x51/0x1a0
| efivar_entry_set_safe+0xa3/0x1b0
| efi_pstore_write+0x109/0x140
| pstore_dump+0x11c/0x330
| kmsg_dump+0xa4/0xd0
| oops_exit+0x22/0x30

This will no longer happen when pstore is called from process context with his patch, but we still get the same thing if we call pstore from interrupt context, unless both the down_interruptible and wait_for_completion in there are also changed to nonblocking calls. However, once they are no longer blocking, we don't need the outer lock to be changed from spinlock to semaphore any more either.

Arnd

Kees Cook

author

2018-12-04 00:28 UTC #6 lore raw

Quoted 40 lines · Arnd Bergmann

Quoted 9 lines · Sebastian Andrzej Siewior
Quoted 3 lines · Arnd Bergmann
You are right that you can't take (or release) a mutex from interrupt context. However, I don't think converting a spinlock to a semaphore is going to help here either.
you can acquire a semaphore with a try_lock from interrupts context but you can't do that with a mutex. You can also a acquire a semaphore in one context and release in another.
Right, that is the obvious part.
Quoted 2 lines
I haven't looked a general picture yet, will try to do so later today or tomorrow.
To speed this up, the problem I'm referring to is in virt_efi_query_variable_info() and efi_queue_work(), as in the original BUG_ON() that Kees quoted:
Quoted 9 lines
BUG: sleeping function called from invalid context at kernel/sched/completion.c:99
|in_atomic(): 1, irqs_disabled(): 1, pid: 2236, name: sig-xstate-bum
|Preemption disabled at:
|[<ffffffff99d60512>] pstore_dump+0x72/0x330
|CPU: 26 PID: 2236 Comm: sig-xstate-bum Tainted: G      D           4.20.0-rc3 #45
|Call Trace:
| dump_stack+0x4f/0x6a
| ___might_sleep.cold.91+0xd3/0xe4
| __might_sleep+0x50/0x90
Quoted 8 lines
| wait_for_completion+0x32/0x130
| virt_efi_query_variable_info+0x14e/0x160
| efi_query_variable_store+0x51/0x1a0
| efivar_entry_set_safe+0xa3/0x1b0
| efi_pstore_write+0x109/0x140
| pstore_dump+0x11c/0x330
| kmsg_dump+0xa4/0xd0
| oops_exit+0x22/0x30

Quoted 7 lines

This will no longer happen when pstore is called from process context with his patch, but we still get the same thing if we call pstore from interrupt context, unless both the down_interruptible and wait_for_completion in there are also changed to nonblocking calls. However, once they are no longer blocking, we don't need the outer lock to be changed from spinlock to semaphore any more either.

My proposed patch was trying to do two things:

- have pstore not make things _worse_ (do not hold a spin lock)
- use preemptible() in efi pstore backend to get it right:

ret = efivar_entry_set_safe(efi_name, vendor, PSTORE_EFI_ATTRIBUTES,

-                             !pstore_cannot_block_path(record->reason),
-                             record->size, record->psi->buf);
+                             preemptible(), record->size, record->psi->buf);

IIUC, that would make efi-vars take the nonblocking path so we don't trip over the might_sleep().

-Kees

Signature · 1 line

Kees Cook

Sebastian Andrzej Siewior

2018-12-04 15:40 UTC #7 lore raw

In reply to Kees Cook

drivers/firmware/efi/efi-pstore.c static int efi_pstore_write(struct pstore_record *record) · line 262

Quoted 11 lines · Kees Cook

drivers/firmware/efi/efi-pstore.c +1 -2

index cfe87b465819..0f7d97917197 100644
@@ -259,8 +259,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,
-			      !pstore_cannot_block_path(record->reason),
-			      record->size, record->psi->buf);
+			      preemptible(), record->size, record->psi->buf);

Well. Better I think.
might_sleep() / preempt_count_equals() checks for preemptible() + rcu_preempt_depth(). kmsg_dump() starts with rcu_read_lock() which means with this patch applied I got:

| BUG: sleeping function called from invalid context at kernel/sched/completion.c:99
| in_atomic(): 0, irqs_disabled(): 0, pid: 2286, name: sig-xstate-bum PC: 0 RCU: 1
| Preemption disabled at:
| [<ffffffff9b959085>] __queue_work+0x95/0x440
| CPU: 30 PID: 2286 Comm: sig-xstate-bum Tainted: G      D           4.20.0-rc3+ #90
| Call Trace:
|  dump_stack+0x4f/0x6a
|  ___might_sleep.cold.91+0xef/0x100
|  __might_sleep+0x50/0x90
|  wait_for_completion+0x32/0x130
|  virt_efi_query_variable_info+0x14e/0x160
|  efi_query_variable_store+0x51/0x1a0
|  efivar_entry_set_safe+0xa3/0x1b0
|  efi_pstore_write+0x110/0x140
|  pstore_dump+0x114/0x320
|  kmsg_dump+0xa4/0xd0
|  oops_exit+0x7f/0x90
|  oops_end+0x67/0xd0
|  die+0x41/0x4a
|  do_general_protection+0xc1/0x150
|  general_protection+0x1e/0x30
| RIP: 0010:__fpu__restore_sig+0x1c1/0x540

just in case you wonder why both counter are zero and it still creates this backtrace.

Quoted 22 lines

drivers/firmware/efi/efi-pstore.c +0 -0

 	if (record->reason == KMSG_DUMP_OOPS)
 		efivar_run_worker();

fs/pstore/platform.c +1 -7

index 2387cb74f729..afdfd3687f94 100644
@@ -400,23 +401,20 @@ static void pstore_dump(struct kmsg_dumper *dumper,
 	unsigned long	total = 0;
 	const char	*why;
 	unsigned int	part = 1;
-	unsigned long	flags = 0;
-	int		is_locked;
 	int		ret;
 
 	why = get_reason_str(reason);
 
-	if (pstore_cannot_block_path(reason)) {
-		is_locked = spin_trylock_irqsave(&psinfo->buf_lock, flags);
-		if (!is_locked) {
-			pr_err("pstore dump routine blocked in %s path, may corrupt error record\n"
-				       , in_nmi() ? "NMI" : why);
+	if (down_trylock(&psinfo->buf_lock)) {

fs/pstore/platform.c static void pstore_dump(struct kmsg_dumper *dumper, · line 415

Quoted 10 lines

fs/pstore/platform.c +5 -3

+		/* 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;
 		}
-	} else {
-		spin_lock_irqsave(&psinfo->buf_lock, flags);
-		is_locked = 1;
+		down_interruptible(&psinfo->buf_lock);

In function ‘pstore_dump’:
fs/pstore/platform.c:393:3: warning: ignoring return value of ‘down_interruptible’, declared with attribute warn_unused_result [-Wunused-result]
down_interruptible(&psinfo->buf_lock);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

fs/pstore/platform.c static void pstore_dump(struct kmsg_dumper *dumper, · line 416

Quoted 1 line

fs/pstore/platform.c +0 -0
 	}

Sebastian

Kees Cook

author

2018-12-04 16:17 UTC #8 lore raw

Quoted 42 lines · Sebastian Andrzej Siewior

drivers/firmware/efi/efi-pstore.c static int efi_pstore_write(struct pstore_record *record) · line 262

Quoted 11 lines · Kees Cook

drivers/firmware/efi/efi-pstore.c +1 -2

index cfe87b465819..0f7d97917197 100644
@@ -259,8 +259,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,
-                           !pstore_cannot_block_path(record->reason),
-                           record->size, record->psi->buf);
+                           preemptible(), record->size, record->psi->buf);

| BUG: sleeping function called from invalid context at kernel/sched/completion.c:99
| in_atomic(): 0, irqs_disabled(): 0, pid: 2286, name: sig-xstate-bum PC: 0 RCU: 1
| Preemption disabled at:
| [<ffffffff9b959085>] __queue_work+0x95/0x440
| CPU: 30 PID: 2286 Comm: sig-xstate-bum Tainted: G      D           4.20.0-rc3+ #90
| Call Trace:
|  dump_stack+0x4f/0x6a
|  ___might_sleep.cold.91+0xef/0x100
|  __might_sleep+0x50/0x90
|  wait_for_completion+0x32/0x130
|  virt_efi_query_variable_info+0x14e/0x160
|  efi_query_variable_store+0x51/0x1a0
|  efivar_entry_set_safe+0xa3/0x1b0
|  efi_pstore_write+0x110/0x140
|  pstore_dump+0x114/0x320
|  kmsg_dump+0xa4/0xd0
|  oops_exit+0x7f/0x90
|  oops_end+0x67/0xd0
|  die+0x41/0x4a
|  do_general_protection+0xc1/0x150
|  general_protection+0x1e/0x30
| RIP: 0010:__fpu__restore_sig+0x1c1/0x540

Quoted 2 lines

just in case you wonder why both counter are zero and it still creates this backtrace.

Oh, hmm. That didn't show up in my testing. Any thoughts on a solution?

Quoted 36 lines

Quoted 23 lines

drivers/firmware/efi/efi-pstore.c +0 -0

      if (record->reason == KMSG_DUMP_OOPS)
              efivar_run_worker();

fs/pstore/platform.c +1 -7

index 2387cb74f729..afdfd3687f94 100644
@@ -400,23 +401,20 @@ static void pstore_dump(struct kmsg_dumper *dumper,
      unsigned long   total = 0;
      const char      *why;
      unsigned int    part = 1;
-     unsigned long   flags = 0;
-     int             is_locked;
      int             ret;
 
      why = get_reason_str(reason);
 
-     if (pstore_cannot_block_path(reason)) {
-             is_locked = spin_trylock_irqsave(&psinfo->buf_lock, flags);
-             if (!is_locked) {
-                     pr_err("pstore dump routine blocked in %s path, may corrupt error record\n"
-                                    , in_nmi() ? "NMI" : why);
+     if (down_trylock(&psinfo->buf_lock)) {

fs/pstore/platform.c static void pstore_dump(struct kmsg_dumper *dumper, · line 415

Quoted 10 lines

fs/pstore/platform.c +5 -3

+             /* 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;
              }
-     } else {
-             spin_lock_irqsave(&psinfo->buf_lock, flags);
-             is_locked = 1;
+             down_interruptible(&psinfo->buf_lock);

In function ‘pstore_dump’:
fs/pstore/platform.c:393:3: warning: ignoring return value of ‘down_interruptible’, declared with attribute warn_unused_result [-Wunused-result]

Quoted 3 lines

   down_interruptible(&psinfo->buf_lock);
   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Thanks, yes. I've fixed this in the version in -next.

-Kees

Signature · 1 line

Kees Cook

Kees Cook

author

2018-12-04 17:23 UTC #9 lore raw

In reply to Sebastian Andrzej Siewior

Quoted 9 lines · Sebastian Andrzej Siewior

Quoted 7 lines · Kees Cook

drivers/firmware/efi/efi-pstore.c +0 -0

index cfe87b465819..0f7d97917197 100644
@@ -259,8 +259,7 @@ static int efi_pstore_write(struct pstore_record *record)
              efi_name[i] = name[i];

Quoted 9 lines

drivers/firmware/efi/efi-pstore.c static int efi_pstore_write(struct pstore_record *record) · line 262
Quoted 4 lines
drivers/firmware/efi/efi-pstore.c +1 -2
      ret = efivar_entry_set_safe(efi_name, vendor, PSTORE_EFI_ATTRIBUTES,
-                           !pstore_cannot_block_path(record->reason),
-                           record->size, record->psi->buf);
+                           preemptible(), record->size, record->psi->buf);
Well. Better I think.
might_sleep() / preempt_count_equals() checks for preemptible() + rcu_preempt_depth(). kmsg_dump() starts with rcu_read_lock() which means with this patch applied I got:

Okay, so, if kmsg_dump() uses rcu_read_lock(), that means efi-pstore can _never_ sleep, and it's nothing to do with pstore internals. :( I guess we just hard-code it, then? And efi-pstore should probably only attach to pstore if it has a nonblock implementation (and warn if one isn't available).

-Kees

Signature · 1 line

Kees Cook

Sebastian Andrzej Siewior

2018-12-04 18:06 UTC #10 lore raw

Quoted 5 lines · Kees Cook

Okay, so, if kmsg_dump() uses rcu_read_lock(), that means efi-pstore can _never_ sleep, and it's nothing to do with pstore internals. :( I guess we just hard-code it, then? And efi-pstore should probably only attach to pstore if it has a nonblock implementation (and warn if one isn't available).

I was about to suggest that. I am not aware if anything else could use efi_pstore_write() use that but otherwise you could hardcode it.

Quoted 2 lines

-Kees

Sebastian

Ard Biesheuvel

2018-12-21 15:37 UTC #11 lore raw

Quoted 11 lines · Sebastian Andrzej Siewior

Quoted 5 lines · Kees Cook
Okay, so, if kmsg_dump() uses rcu_read_lock(), that means efi-pstore can _never_ sleep, and it's nothing to do with pstore internals. :( I guess we just hard-code it, then? And efi-pstore should probably only attach to pstore if it has a nonblock implementation (and warn if one isn't available).
I was about to suggest that. I am not aware if anything else could use efi_pstore_write() use that but otherwise you could hardcode it.

efivar_entry_set_safe() will only use the default backend if no non-blocking variant is provided, in which case it assumes that the default one is non-blocking. Perhaps we should just assign both function pointers to the same function in this case.