Linux kernel mirror (for testing) git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
kernel os linux

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 <bigeasy@linutronix.de>
Fixes: 21b3ddd39fee ("efi: Don't use spinlocks for efi vars")
Signed-off-by: Kees Cook <keescook@chromium.org>

+27 -32
-2
arch/powerpc/kernel/nvram_64.c
··· 563 563 nvram_pstore_info.buf = oops_data; 564 564 nvram_pstore_info.bufsize = oops_data_sz; 565 565 566 - spin_lock_init(&nvram_pstore_info.buf_lock); 567 - 568 566 rc = pstore_register(&nvram_pstore_info); 569 567 if (rc && (rc != -EPERM)) 570 568 /* Print error only when pstore.backend == nvram */
-1
drivers/acpi/apei/erst.c
··· 1176 1176 "Error Record Serialization Table (ERST) support is initialized.\n"); 1177 1177 1178 1178 buf = kmalloc(erst_erange.size, GFP_KERNEL); 1179 - spin_lock_init(&erst_info.buf_lock); 1180 1179 if (buf) { 1181 1180 erst_info.buf = buf + sizeof(struct cper_pstore_record); 1182 1181 erst_info.bufsize = erst_erange.size -
+1 -3
drivers/firmware/efi/efi-pstore.c
··· 259 259 efi_name[i] = name[i]; 260 260 261 261 ret = efivar_entry_set_safe(efi_name, vendor, PSTORE_EFI_ATTRIBUTES, 262 - !pstore_cannot_block_path(record->reason), 263 - record->size, record->psi->buf); 262 + preemptible(), record->size, record->psi->buf); 264 263 265 264 if (record->reason == KMSG_DUMP_OOPS) 266 265 efivar_run_worker(); ··· 368 369 return -ENOMEM; 369 370 370 371 efi_pstore_info.bufsize = 1024; 371 - spin_lock_init(&efi_pstore_info.buf_lock); 372 372 373 373 if (pstore_register(&efi_pstore_info)) { 374 374 kfree(efi_pstore_info.buf);
+23 -21
fs/pstore/platform.c
··· 161 161 } 162 162 } 163 163 164 - bool pstore_cannot_block_path(enum kmsg_dump_reason reason) 164 + /* 165 + * Should pstore_dump() wait for a concurrent pstore_dump()? If 166 + * not, the current pstore_dump() will report a failure to dump 167 + * and return. 168 + */ 169 + static bool pstore_cannot_wait(enum kmsg_dump_reason reason) 165 170 { 166 - /* 167 - * In case of NMI path, pstore shouldn't be blocked 168 - * regardless of reason. 169 - */ 171 + /* In NMI path, pstore shouldn't block regardless of reason. */ 170 172 if (in_nmi()) 171 173 return true; 172 174 173 175 switch (reason) { 174 176 /* In panic case, other cpus are stopped by smp_send_stop(). */ 175 177 case KMSG_DUMP_PANIC: 176 - /* Emergency restart shouldn't be blocked by spin lock. */ 178 + /* Emergency restart shouldn't be blocked. */ 177 179 case KMSG_DUMP_EMERG: 178 180 return true; 179 181 default: 180 182 return false; 181 183 } 182 184 } 183 - EXPORT_SYMBOL_GPL(pstore_cannot_block_path); 184 185 185 186 #if IS_ENABLED(CONFIG_PSTORE_DEFLATE_COMPRESS) 186 187 static int zbufsize_deflate(size_t size) ··· 401 400 unsigned long total = 0; 402 401 const char *why; 403 402 unsigned int part = 1; 404 - unsigned long flags = 0; 405 - int is_locked; 406 403 int ret; 407 404 408 405 why = get_reason_str(reason); 409 406 410 - if (pstore_cannot_block_path(reason)) { 411 - is_locked = spin_trylock_irqsave(&psinfo->buf_lock, flags); 412 - if (!is_locked) { 413 - pr_err("pstore dump routine blocked in %s path, may corrupt error record\n" 414 - , in_nmi() ? "NMI" : why); 407 + if (down_trylock(&psinfo->buf_lock)) { 408 + /* Failed to acquire lock: give up if we cannot wait. */ 409 + if (pstore_cannot_wait(reason)) { 410 + pr_err("dump skipped in %s path: may corrupt error record\n", 411 + in_nmi() ? "NMI" : why); 415 412 return; 416 413 } 417 - } else { 418 - spin_lock_irqsave(&psinfo->buf_lock, flags); 419 - is_locked = 1; 414 + if (down_interruptible(&psinfo->buf_lock)) { 415 + pr_err("could not grab semaphore?!\n"); 416 + return; 417 + } 420 418 } 419 + 421 420 oopscount++; 422 421 while (total < kmsg_bytes) { 423 422 char *dst; ··· 434 433 record.part = part; 435 434 record.buf = psinfo->buf; 436 435 437 - if (big_oops_buf && is_locked) { 436 + if (big_oops_buf) { 438 437 dst = big_oops_buf; 439 438 dst_size = big_oops_buf_sz; 440 439 } else { ··· 452 451 dst_size, &dump_size)) 453 452 break; 454 453 455 - if (big_oops_buf && is_locked) { 454 + if (big_oops_buf) { 456 455 zipped_len = pstore_compress(dst, psinfo->buf, 457 456 header_size + dump_size, 458 457 psinfo->bufsize); ··· 475 474 total += record.size; 476 475 part++; 477 476 } 478 - if (is_locked) 479 - spin_unlock_irqrestore(&psinfo->buf_lock, flags); 477 + 478 + up(&psinfo->buf_lock); 480 479 } 481 480 482 481 static struct kmsg_dumper pstore_dumper = { ··· 595 594 psi->write_user = pstore_write_user_compat; 596 595 psinfo = psi; 597 596 mutex_init(&psinfo->read_mutex); 597 + sema_init(&psinfo->buf_lock, 1); 598 598 spin_unlock(&pstore_lock); 599 599 600 600 if (owner && !try_module_get(owner)) {
-1
fs/pstore/ram.c
··· 815 815 err = -ENOMEM; 816 816 goto fail_clear; 817 817 } 818 - spin_lock_init(&cxt->pstore.buf_lock); 819 818 820 819 cxt->pstore.flags = PSTORE_FLAGS_DMESG; 821 820 if (cxt->console_size)
+3 -4
include/linux/pstore.h
··· 26 26 #include <linux/errno.h> 27 27 #include <linux/kmsg_dump.h> 28 28 #include <linux/mutex.h> 29 - #include <linux/spinlock.h> 29 + #include <linux/semaphore.h> 30 30 #include <linux/time.h> 31 31 #include <linux/types.h> 32 32 ··· 99 99 * @owner: module which is responsible for this backend driver 100 100 * @name: name of the backend driver 101 101 * 102 - * @buf_lock: spinlock to serialize access to @buf 102 + * @buf_lock: semaphore to serialize access to @buf 103 103 * @buf: preallocated crash dump buffer 104 104 * @bufsize: size of @buf available for crash dump bytes (must match 105 105 * smallest number of bytes available for writing to a ··· 184 184 struct module *owner; 185 185 char *name; 186 186 187 - spinlock_t buf_lock; 187 + struct semaphore buf_lock; 188 188 char *buf; 189 189 size_t bufsize; 190 190 ··· 210 210 211 211 extern int pstore_register(struct pstore_info *); 212 212 extern void pstore_unregister(struct pstore_info *); 213 - extern bool pstore_cannot_block_path(enum kmsg_dump_reason reason); 214 213 215 214 struct pstore_ftrace_record { 216 215 unsigned long ip;