[PATCH] pstore: replace spinlock_t by raw_spinlock_t

[PATCH] pstore: replace spinlock_t by raw_spinlock_t

[Wen Yang]

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(-)

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;
 
-- 
2.25.1

Re: [PATCH] pstore: replace spinlock_t by raw_spinlock_t

[Greg KH]

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

Re: [PATCH] pstore: replace spinlock_t by raw_spinlock_t

[Wen Yang]

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

Re: [PATCH] pstore: replace spinlock_t by raw_spinlock_t

[Kees Cook]

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

-- 
Kees Cook

Re: [PATCH] pstore: replace spinlock_t by raw_spinlock_t

[Kees Cook]

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,

-- 
Kees Cook