| Message ID | 20240819145945.61274-1-wen.yang@linux.dev (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 1bf8012fc6997f2117f6919369cde16659db60e0 |
| Headers | show |
| Series | pstore: replace spinlock_t by raw_spinlock_t | expand |
On Mon, Aug 19, 2024 at 10:59:45PM +0800, Wen Yang wrote: > pstore_dump() is called when both preemption and local IRQ are disabled, > and a spinlock is obtained, which is problematic for the RT kernel because > in this configuration, spinlocks are sleep locks. > > Replace the spinlock_t with raw_spinlock_t to avoid sleeping in atomic context. This feels odd, is it only an out-of-tree RT thing? Or does this affect in-kernel code as well? What prevents any normal spinlock from sleeping in your system configuration as well? thanks, greg k-h
On 2024/8/20 01:45, Greg KH wrote: > On Mon, Aug 19, 2024 at 10:59:45PM +0800, Wen Yang wrote: >> pstore_dump() is called when both preemption and local IRQ are disabled, >> and a spinlock is obtained, which is problematic for the RT kernel because >> in this configuration, spinlocks are sleep locks. >> >> Replace the spinlock_t with raw_spinlock_t to avoid sleeping in atomic context. > > This feels odd, is it only an out-of-tree RT thing? Or does this affect > in-kernel code as well? What prevents any normal spinlock from sleeping > in your system configuration as well? > Thank you for your comment. If we enable PREEMPT_RT, it will also affect in-kernel code, such as in the following scenario: echo b > /proc/sysrq-trigger - sysrq_handle_reboot - emergency_restart - kmsg_dump - pstore_dump Obtained psinfo->buf_lock, if there is a lot of error log in the kernel, it will last for a long time If the system unexpectedly crashes at this time: - panic() - kmsg_dump - pstore_dump Attempting to obtain psinfo->buf_lock while disabling interrupts and preemption, but since this lock is already held by the above process, it will result in illegal sleep. -- Best wishes, Wen
On Sat, Aug 24, 2024 at 03:25:04PM +0800, Wen Yang wrote: > > > On 2024/8/20 01:45, Greg KH wrote: > > On Mon, Aug 19, 2024 at 10:59:45PM +0800, Wen Yang wrote: > > > pstore_dump() is called when both preemption and local IRQ are disabled, > > > and a spinlock is obtained, which is problematic for the RT kernel because > > > in this configuration, spinlocks are sleep locks. > > > > > > Replace the spinlock_t with raw_spinlock_t to avoid sleeping in atomic context. > > > > This feels odd, is it only an out-of-tree RT thing? Or does this affect > > in-kernel code as well? What prevents any normal spinlock from sleeping > > in your system configuration as well? > > > > Thank you for your comment. > > If we enable PREEMPT_RT, it will also affect in-kernel code, such as in the > following scenario: > > echo b > /proc/sysrq-trigger > - sysrq_handle_reboot > - emergency_restart > - kmsg_dump > - pstore_dump > Obtained psinfo->buf_lock, if there is a lot of error log in the kernel, it > will last for a long time > > If the system unexpectedly crashes at this time: > - panic() > - kmsg_dump > - pstore_dump > Attempting to obtain psinfo->buf_lock while disabling interrupts and > preemption, but since this lock is already held by the above process, it > will result in illegal sleep. Reading Documentation/locking/locktypes.rst seems to suggest pstore does want the raw version. I'm surprised there aren't many more cases where this is a problem. :P
On Mon, 19 Aug 2024 22:59:45 +0800, Wen Yang wrote: > pstore_dump() is called when both preemption and local IRQ are disabled, > and a spinlock is obtained, which is problematic for the RT kernel because > in this configuration, spinlocks are sleep locks. > > Replace the spinlock_t with raw_spinlock_t to avoid sleeping in atomic context. > > > [...] Applied to for-next/pstore, thanks! [1/1] pstore: replace spinlock_t by raw_spinlock_t https://git.kernel.org/kees/c/1bf8012fc699 Take care,
diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c index 3497ede88aa0..84719e2bcbc4 100644 --- a/fs/pstore/platform.c +++ b/fs/pstore/platform.c @@ -288,13 +288,13 @@ static void pstore_dump(struct kmsg_dumper *dumper, why = kmsg_dump_reason_str(reason); if (pstore_cannot_block_path(reason)) { - if (!spin_trylock_irqsave(&psinfo->buf_lock, flags)) { + if (!raw_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); + raw_spin_lock_irqsave(&psinfo->buf_lock, flags); } kmsg_dump_rewind(&iter); @@ -364,7 +364,7 @@ static void pstore_dump(struct kmsg_dumper *dumper, total += record.size; part++; } - spin_unlock_irqrestore(&psinfo->buf_lock, flags); + raw_spin_unlock_irqrestore(&psinfo->buf_lock, flags); if (saved_ret) { pr_err_once("backend (%s) writing error (%d)\n", psinfo->name, @@ -503,7 +503,7 @@ int pstore_register(struct pstore_info *psi) psi->write_user = pstore_write_user_compat; psinfo = psi; mutex_init(&psinfo->read_mutex); - spin_lock_init(&psinfo->buf_lock); + raw_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 638507a3c8ff..fed601053c51 100644 --- a/include/linux/pstore.h +++ b/include/linux/pstore.h @@ -182,7 +182,7 @@ struct pstore_info { struct module *owner; const char *name; - spinlock_t buf_lock; + raw_spinlock_t buf_lock; char *buf; size_t bufsize;
pstore_dump() is called when both preemption and local IRQ are disabled, and a spinlock is obtained, which is problematic for the RT kernel because in this configuration, spinlocks are sleep locks. Replace the spinlock_t with raw_spinlock_t to avoid sleeping in atomic context. Signed-off-by: Wen Yang <wen.yang@linux.dev> Cc: Kees Cook <kees@kernel.org> Cc: Tony Luck <tony.luck@intel.com> Cc: "Guilherme G. Piccoli" <gpiccoli@igalia.com> Cc: linux-hardening@vger.kernel.org Cc: linux-kernel@vger.kernel.org --- fs/pstore/platform.c | 8 ++++---- include/linux/pstore.h | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-)