linux-scsi.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* spinlock recursion in aio_complete()
@ 2023-05-21  5:43 Helge Deller
  2023-05-22 19:28 ` Bart Van Assche
  0 siblings, 1 reply; 11+ messages in thread
From: Helge Deller @ 2023-05-21  5:43 UTC (permalink / raw)
  To: Linux SCSI List, linux-aio, linux-parisc

Hi,

On a single-CPU parisc64 machine I face the spinlock recursion below.
Happens reproduceably directly at bootup since kernel 6.2 (and ~ 6.1.5).
Kernel is built for SMP. Same kernel binary works nicely on machines with more than
one CPU, but stops on UP machines.
Any idea or patch I could try?

Helge

[   26.116568] fuse: init (API version 7.38)
[   26.338231] loop: module loaded
[   26.357390] BUG: spinlock recursion on CPU#0, systemd/1
[   26.357504]  lock: 0x4ad42618, .magic: dead4ead, .owner: systemd/1, .owner_cpu: 0
[   26.357884] CPU: 0 PID: 1 Comm: systemd Not tainted 6.3.3+ #180
[   26.357976] Hardware name: 9000/800/rp3410
[   26.361055] Backtrace:
[   26.361055]  [<000000004030c4d0>] show_stack+0x74/0xb0
[   26.361055]  [<00000000412ec758>] dump_stack_lvl+0xd8/0x128
[   26.361055]  [<00000000412ec7dc>] dump_stack+0x34/0x48
[   26.361055]  [<00000000412d6c8c>] spin_dump+0xe0/0x120
[   26.361055]  [<00000000403f026c>] do_raw_spin_lock+0x138/0x1a0
[   26.361055]  [<00000000412efcb0>] _raw_spin_lock_irqsave+0x3c/0x78
[   26.361055]  [<0000000040808b9c>] aio_complete+0x68/0x470
[   26.361055]  [<000000004080bd54>] aio_complete_rw+0x200/0x400
[   26.361055]  [<0000000040aac314>] blkdev_bio_end_io_async+0x60/0x140
[   26.361055]  [<0000000040ab0fb0>] bio_endio+0x274/0x318
[   26.361055]  [<0000000040ad0b38>] blk_update_request+0x2bc/0x600
[   26.361055]  [<0000000040e6daa4>] scsi_end_request+0x60/0x370
[   26.361055]  [<0000000040e6ef68>] scsi_io_completion+0x9c/0x8e8
[   26.361055]  [<0000000040e5d5c8>] scsi_finish_command+0x10c/0x290
[   26.361055]  [<0000000040e6e7c4>] scsi_complete+0x118/0x308
[   26.361055]  [<0000000040acd3fc>] blk_mq_complete_request+0x60/0x98
[   26.361055]  [<0000000040e6eaf0>] scsi_done_internal+0x13c/0x1c8
[   26.361055]  [<0000000040e6ebac>] scsi_done+0x30/0x60
[   26.361055]  [<0000000040e8b140>] sym_xpt_done+0x94/0xc8
[   26.361055]  [<0000000040e91588>] sym_interrupt+0x5bc/0x28e8
[   26.361055]  [<0000000040e88c1c>] sym53c8xx_intr+0x98/0x170
[   26.361055]  [<00000000403fe380>] __handle_irq_event_percpu+0xdc/0x338
[   26.361055]  [<00000000403fe618>] handle_irq_event_percpu+0x3c/0xf8
[   26.361055]  [<000000004040925c>] handle_percpu_irq+0xb8/0x110
[   26.361055]  [<00000000403fcc74>] generic_handle_irq+0x60/0xb0
[   26.361055]  [<000000004030f930>] do_cpu_irq_mask+0x304/0x508
[   26.361055]  [<0000000040303070>] intr_return+0x0/0xc

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: spinlock recursion in aio_complete()
  2023-05-21  5:43 spinlock recursion in aio_complete() Helge Deller
@ 2023-05-22 19:28 ` Bart Van Assche
  2023-05-22 20:51   ` Helge Deller
  0 siblings, 1 reply; 11+ messages in thread
From: Bart Van Assche @ 2023-05-22 19:28 UTC (permalink / raw)
  To: Helge Deller, Linux SCSI List, linux-aio, linux-parisc

On 5/20/23 22:43, Helge Deller wrote:
> On a single-CPU parisc64 machine I face the spinlock recursion below.
> Happens reproduceably directly at bootup since kernel 6.2 (and ~ 6.1.5).
> Kernel is built for SMP. Same kernel binary works nicely on machines with more than
> one CPU, but stops on UP machines.
> Any idea or patch I could try?

How about performing one or more of the following actions?
* Translating aio_complete+0x68 into a line number.
* Repeating the test with lockdep enabled.
* Bisecting this issue.

Thanks,

Bart.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: spinlock recursion in aio_complete()
  2023-05-22 19:28 ` Bart Van Assche
@ 2023-05-22 20:51   ` Helge Deller
  2023-05-22 20:58     ` Bart Van Assche
  0 siblings, 1 reply; 11+ messages in thread
From: Helge Deller @ 2023-05-22 20:51 UTC (permalink / raw)
  To: Bart Van Assche, Linux SCSI List, linux-aio, linux-parisc

On 5/22/23 21:28, Bart Van Assche wrote:
> On 5/20/23 22:43, Helge Deller wrote:
>> On a single-CPU parisc64 machine I face the spinlock recursion below.
>> Happens reproduceably directly at bootup since kernel 6.2 (and ~ 6.1.5).
>> Kernel is built for SMP. Same kernel binary works nicely on machines with more than
>> one CPU, but stops on UP machines.
>> Any idea or patch I could try?
>
> How about performing one or more of the following actions?
> * Translating aio_complete+0x68 into a line number.

It hangs in fs/aio.c:1128, function aio_complete(), in this call:
	spin_lock_irqsave(&ctx->completion_lock, flags);

> * Repeating the test with lockdep enabled.

Hmm... We don't have LOCKDEP yet on parisc :-(

> * Bisecting this issue.

Will try, but this process is *very* slow as it happens on one machine only,
it's only accessible remotely, uses endless time to reboot and I have no
easy way to netboot kernels...

Helge

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: spinlock recursion in aio_complete()
  2023-05-22 20:51   ` Helge Deller
@ 2023-05-22 20:58     ` Bart Van Assche
  2023-05-22 21:22       ` Helge Deller
  0 siblings, 1 reply; 11+ messages in thread
From: Bart Van Assche @ 2023-05-22 20:58 UTC (permalink / raw)
  To: Helge Deller, Linux SCSI List, linux-aio, linux-parisc

On 5/22/23 13:51, Helge Deller wrote:
> On 5/22/23 21:28, Bart Van Assche wrote:
>> On 5/20/23 22:43, Helge Deller wrote:
>>> On a single-CPU parisc64 machine I face the spinlock recursion below.
>>> Happens reproduceably directly at bootup since kernel 6.2 (and ~ 6.1.5).
>>> Kernel is built for SMP. Same kernel binary works nicely on machines with more than
>>> one CPU, but stops on UP machines.
>>> Any idea or patch I could try?
>>
>> How about performing one or more of the following actions?
>> * Translating aio_complete+0x68 into a line number.
> 
> It hangs in fs/aio.c:1128, function aio_complete(), in this call:
>      spin_lock_irqsave(&ctx->completion_lock, flags);

All code that I found and that obtains ctx->completion_lock disables IRQs.
It is not clear to me how this spinlock can be locked recursively? Is it
sure that the "spinlock recursion" report is correct?

Thanks,

Bart.


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: spinlock recursion in aio_complete()
  2023-05-22 20:58     ` Bart Van Assche
@ 2023-05-22 21:22       ` Helge Deller
  2023-05-23 10:24         ` Helge Deller
  0 siblings, 1 reply; 11+ messages in thread
From: Helge Deller @ 2023-05-22 21:22 UTC (permalink / raw)
  To: Bart Van Assche, Linux SCSI List, linux-aio, linux-parisc

On 5/22/23 22:58, Bart Van Assche wrote:
> On 5/22/23 13:51, Helge Deller wrote:
>> On 5/22/23 21:28, Bart Van Assche wrote:
>>> On 5/20/23 22:43, Helge Deller wrote:
>>>> On a single-CPU parisc64 machine I face the spinlock recursion below.
>>>> Happens reproduceably directly at bootup since kernel 6.2 (and ~ 6.1.5).
>>>> Kernel is built for SMP. Same kernel binary works nicely on machines with more than
>>>> one CPU, but stops on UP machines.
>>>> Any idea or patch I could try?
>>>
>>> How about performing one or more of the following actions?
>>> * Translating aio_complete+0x68 into a line number.
>>
>> It hangs in fs/aio.c:1128, function aio_complete(), in this call:
>>      spin_lock_irqsave(&ctx->completion_lock, flags);
>
> All code that I found and that obtains ctx->completion_lock disables IRQs.
> It is not clear to me how this spinlock can be locked recursively? Is it
> sure that the "spinlock recursion" report is correct?

Yes, it seems correct.
I can see the machine hanging in exactly the same lock when I build without
the CONFIG_DEBUG_SPINLOCK option and check the processor registers with the TOC
command (TOC on parisc means: stop CPU instruction processing, save all CPU registers
to an area which can be analyzed at a later point in time, then reboot the machine).

So, the spinlock recursion from kernel and TOC both agree.

Again, the strange thing is, that I can only reproduce it on that specific machine,
which has only one CPU but runs the SMP kernel.

Helge

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: spinlock recursion in aio_complete()
  2023-05-22 21:22       ` Helge Deller
@ 2023-05-23 10:24         ` Helge Deller
  2023-05-23 10:51           ` Russell King (Oracle)
  0 siblings, 1 reply; 11+ messages in thread
From: Helge Deller @ 2023-05-23 10:24 UTC (permalink / raw)
  To: Bart Van Assche, Linux SCSI List, linux-aio, linux-parisc,
	Russell King, linux-arm-kernel@lists.infradead.org

On 5/22/23 23:22, Helge Deller wrote:
>>> It hangs in fs/aio.c:1128, function aio_complete(), in this call:
>>>      spin_lock_irqsave(&ctx->completion_lock, flags);
>>
>> All code that I found and that obtains ctx->completion_lock disables IRQs.
>> It is not clear to me how this spinlock can be locked recursively? Is it
>> sure that the "spinlock recursion" report is correct?
>
> Yes, it seems correct.
> [...]

Bart, thanks to your suggestions I was able to narrow down the problem!

I got LOCKDEP working on parisc, which then reports:
	raw_local_irq_restore() called with IRQs enabled
for the spin_unlock_irqrestore() in function aio_complete(), which shouldn't happen.

Finally, I found that parisc's flush_dcache_page() re-enables the IRQs
which leads to the spinlock hang in aio_complete().

So, this is NOT a bug in aio or scsci, but we need fix in the the arch code.


While checking flush_dcache_page() re-enables IRQs, I see on parisc and ARM(32):
flush_dcache_page()  calls:
   -> flush_dcache_mmap_lock()   /  flush_dcache_mmap_unlock()
which uses: xa_lock_irq()	/  xa_unlock_irq()

So, the call to xa_unlock_irq() re-enables the IRQs unconditionally
and triggers the hang in aio_complete().

I temporarily #defined flush_dcache_mmap_lock() to NOP and the kernel booted nicely.

Not sure yet what the best fix is...

Helge

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: spinlock recursion in aio_complete()
  2023-05-23 10:24         ` Helge Deller
@ 2023-05-23 10:51           ` Russell King (Oracle)
  2023-05-23 20:01             ` Helge Deller
  0 siblings, 1 reply; 11+ messages in thread
From: Russell King (Oracle) @ 2023-05-23 10:51 UTC (permalink / raw)
  To: Helge Deller
  Cc: Bart Van Assche, Linux SCSI List, linux-aio, linux-parisc,
	linux-arm-kernel@lists.infradead.org

On Tue, May 23, 2023 at 12:24:04PM +0200, Helge Deller wrote:
> On 5/22/23 23:22, Helge Deller wrote:
> > > > It hangs in fs/aio.c:1128, function aio_complete(), in this call:
> > > >      spin_lock_irqsave(&ctx->completion_lock, flags);
> > > 
> > > All code that I found and that obtains ctx->completion_lock disables IRQs.
> > > It is not clear to me how this spinlock can be locked recursively? Is it
> > > sure that the "spinlock recursion" report is correct?
> > 
> > Yes, it seems correct.
> > [...]
> 
> Bart, thanks to your suggestions I was able to narrow down the problem!
> 
> I got LOCKDEP working on parisc, which then reports:
> 	raw_local_irq_restore() called with IRQs enabled
> for the spin_unlock_irqrestore() in function aio_complete(), which shouldn't happen.
> 
> Finally, I found that parisc's flush_dcache_page() re-enables the IRQs
> which leads to the spinlock hang in aio_complete().
> 
> So, this is NOT a bug in aio or scsci, but we need fix in the the arch code.

You can find some of the background to this at:

https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git/commit/?id=16ceff2d5dc9f0347ab5a08abff3f4647c2fee04

which introduced flush_dcache_mmap_lock(). It looks like Hugh had
questions over whether this should be _irqsave() rather than _irq()
but I guess at the time all callers had interrupts enabled, and
it's only recently that someone came up with the idea of calling
flush_dcache_page() with interrupts disabled.

Adding another arg to flush_dcache_mmap_lock() to save the flags
may be doable, but requires a patch that touches not only architectures
that have a private implementation, but also various code in mm/.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: spinlock recursion in aio_complete()
  2023-05-23 10:51           ` Russell King (Oracle)
@ 2023-05-23 20:01             ` Helge Deller
  2023-05-23 20:06               ` Bart Van Assche
  2023-05-23 20:24               ` Helge Deller
  0 siblings, 2 replies; 11+ messages in thread
From: Helge Deller @ 2023-05-23 20:01 UTC (permalink / raw)
  To: Russell King (Oracle), Dinh Nguyen
  Cc: Helge Deller, Bart Van Assche, Linux SCSI List, linux-aio,
	linux-parisc, linux-arm-kernel@lists.infradead.org

* Russell King (Oracle) <linux@armlinux.org.uk>:
> On Tue, May 23, 2023 at 12:24:04PM +0200, Helge Deller wrote:
> > On 5/22/23 23:22, Helge Deller wrote:
> > > > > It hangs in fs/aio.c:1128, function aio_complete(), in this call:
> > > > >      spin_lock_irqsave(&ctx->completion_lock, flags);
> > > >
> > > > All code that I found and that obtains ctx->completion_lock disables IRQs.
> > > > It is not clear to me how this spinlock can be locked recursively? Is it
> > > > sure that the "spinlock recursion" report is correct?
> > >
> > > Yes, it seems correct.
> > > [...]
> >
> > Bart, thanks to your suggestions I was able to narrow down the problem!
> >
> > I got LOCKDEP working on parisc, which then reports:
> > 	raw_local_irq_restore() called with IRQs enabled
> > for the spin_unlock_irqrestore() in function aio_complete(), which shouldn't happen.
> >
> > Finally, I found that parisc's flush_dcache_page() re-enables the IRQs
> > which leads to the spinlock hang in aio_complete().
> >
> > So, this is NOT a bug in aio or scsci, but we need fix in the the arch code.
>
> You can find some of the background to this at:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git/commit/?id=16ceff2d5dc9f0347ab5a08abff3f4647c2fee04
>
> which introduced flush_dcache_mmap_lock(). It looks like Hugh had
> questions over whether this should be _irqsave() rather than _irq()
> but I guess at the time all callers had interrupts enabled, and
> it's only recently that someone came up with the idea of calling
> flush_dcache_page() with interrupts disabled.
>
> Adding another arg to flush_dcache_mmap_lock() to save the flags
> may be doable, but requires a patch that touches not only architectures
> that have a private implementation, but also various code in mm/.

I've tested the attached patch on parisc, and it solves the issue.
I've not compile-tested it on arm and nios2, both seem to be
the only other affected platforms.

Thoughts?

Helge


From 25a96a4211975d46e6f4dac06e144d0fb9f5ed53 Mon Sep 17 00:00:00 2001
From: Helge Deller <deller@gmx.de>
Date: Tue, 23 May 2023 21:48:33 +0200
Subject: [PATCH] Fix flush_dcache_page() for usage in irq context

flush_dcache_page() can be called with IRQs disabled, e.g. from
aio_complete().

Fix flush_dcache_page() on the arm, parisc and nios2 architectures
to not unintentionally re-enable IRQs by using xa_lock_irqsave() instead
of xa_lock_irq() for the flush_dcache_mmap_*lock() functions.

Cc: Russell King (Oracle) <linux@armlinux.org.uk>
Cc: Dinh Nguyen <dinguyen@kernel.org>
Signed-off-by: Helge Deller <deller@gmx.de>

diff --git a/arch/arm/include/asm/cacheflush.h b/arch/arm/include/asm/cacheflush.h
index a094f964c869..5b8a1ef0dc50 100644
--- a/arch/arm/include/asm/cacheflush.h
+++ b/arch/arm/include/asm/cacheflush.h
@@ -315,6 +315,10 @@ static inline void flush_anon_page(struct vm_area_struct *vma,

 #define flush_dcache_mmap_lock(mapping)		xa_lock_irq(&mapping->i_pages)
 #define flush_dcache_mmap_unlock(mapping)	xa_unlock_irq(&mapping->i_pages)
+#define flush_dcache_mmap_lock_irqsave(mapping, flags)		\
+		xa_lock_irqsave(&mapping->i_pages, flags)
+#define flush_dcache_mmap_unlock_irqrestore(mapping, flags)	\
+		xa_unlock_irqrestore(&mapping->i_pages, flags)

 /*
  * We don't appear to need to do anything here.  In fact, if we did, we'd
diff --git a/arch/arm/mm/flush.c b/arch/arm/mm/flush.c
index 7ff9feea13a6..d57ec9165520 100644
--- a/arch/arm/mm/flush.c
+++ b/arch/arm/mm/flush.c
@@ -238,6 +238,7 @@ static void __flush_dcache_aliases(struct address_space *mapping, struct page *p
 {
 	struct mm_struct *mm = current->active_mm;
 	struct vm_area_struct *mpnt;
+	unsigned long flags;
 	pgoff_t pgoff;

 	/*
@@ -248,7 +249,7 @@ static void __flush_dcache_aliases(struct address_space *mapping, struct page *p
 	 */
 	pgoff = page->index;

-	flush_dcache_mmap_lock(mapping);
+	flush_dcache_mmap_lock_irqsave(mapping, flags);
 	vma_interval_tree_foreach(mpnt, &mapping->i_mmap, pgoff, pgoff) {
 		unsigned long offset;

@@ -262,7 +263,7 @@ static void __flush_dcache_aliases(struct address_space *mapping, struct page *p
 		offset = (pgoff - mpnt->vm_pgoff) << PAGE_SHIFT;
 		flush_cache_page(mpnt, mpnt->vm_start + offset, page_to_pfn(page));
 	}
-	flush_dcache_mmap_unlock(mapping);
+	flush_dcache_mmap_unlock_irqrestore(mapping, flags);
 }

 #if __LINUX_ARM_ARCH__ >= 6
diff --git a/arch/nios2/include/asm/cacheflush.h b/arch/nios2/include/asm/cacheflush.h
index d0b71dd71287..a37242662809 100644
--- a/arch/nios2/include/asm/cacheflush.h
+++ b/arch/nios2/include/asm/cacheflush.h
@@ -48,5 +48,9 @@ extern void invalidate_dcache_range(unsigned long start, unsigned long end);

 #define flush_dcache_mmap_lock(mapping)		xa_lock_irq(&mapping->i_pages)
 #define flush_dcache_mmap_unlock(mapping)	xa_unlock_irq(&mapping->i_pages)
+#define flush_dcache_mmap_lock_irqsave(mapping, flags)		\
+		xa_lock_irqsave(&mapping->i_pages, flags)
+#define flush_dcache_mmap_unlock_irqrestore(mapping, flags)	\
+		xa_unlock_irqrestore(&mapping->i_pages, flags)

 #endif /* _ASM_NIOS2_CACHEFLUSH_H */
diff --git a/arch/nios2/mm/cacheflush.c b/arch/nios2/mm/cacheflush.c
index 6aa9257c3ede..35f3b599187f 100644
--- a/arch/nios2/mm/cacheflush.c
+++ b/arch/nios2/mm/cacheflush.c
@@ -75,11 +75,12 @@ static void flush_aliases(struct address_space *mapping, struct page *page)
 {
 	struct mm_struct *mm = current->active_mm;
 	struct vm_area_struct *mpnt;
+	unsigned long flags;
 	pgoff_t pgoff;

 	pgoff = page->index;

-	flush_dcache_mmap_lock(mapping);
+	flush_dcache_mmap_lock_irqsave(mapping, flags);
 	vma_interval_tree_foreach(mpnt, &mapping->i_mmap, pgoff, pgoff) {
 		unsigned long offset;

@@ -92,7 +93,7 @@ static void flush_aliases(struct address_space *mapping, struct page *page)
 		flush_cache_page(mpnt, mpnt->vm_start + offset,
 			page_to_pfn(page));
 	}
-	flush_dcache_mmap_unlock(mapping);
+	flush_dcache_mmap_unlock_irqrestore(mapping, flags);
 }

 void flush_cache_all(void)
diff --git a/arch/parisc/include/asm/cacheflush.h b/arch/parisc/include/asm/cacheflush.h
index 0bdee6724132..c8b6928cee1e 100644
--- a/arch/parisc/include/asm/cacheflush.h
+++ b/arch/parisc/include/asm/cacheflush.h
@@ -48,6 +48,10 @@ void flush_dcache_page(struct page *page);

 #define flush_dcache_mmap_lock(mapping)		xa_lock_irq(&mapping->i_pages)
 #define flush_dcache_mmap_unlock(mapping)	xa_unlock_irq(&mapping->i_pages)
+#define flush_dcache_mmap_lock_irqsave(mapping, flags)		\
+		xa_lock_irqsave(&mapping->i_pages, flags)
+#define flush_dcache_mmap_unlock_irqrestore(mapping, flags)	\
+		xa_unlock_irqrestore(&mapping->i_pages, flags)

 #define flush_icache_page(vma,page)	do { 		\
 	flush_kernel_dcache_page_addr(page_address(page)); \
diff --git a/arch/parisc/kernel/cache.c b/arch/parisc/kernel/cache.c
index 1d3b8bc8a623..ca4a302d4365 100644
--- a/arch/parisc/kernel/cache.c
+++ b/arch/parisc/kernel/cache.c
@@ -399,6 +399,7 @@ void flush_dcache_page(struct page *page)
 	unsigned long offset;
 	unsigned long addr, old_addr = 0;
 	unsigned long count = 0;
+	unsigned long flags;
 	pgoff_t pgoff;

 	if (mapping && !mapping_mapped(mapping)) {
@@ -420,7 +421,7 @@ void flush_dcache_page(struct page *page)
 	 * to flush one address here for them all to become coherent
 	 * on machines that support equivalent aliasing
 	 */
-	flush_dcache_mmap_lock(mapping);
+	flush_dcache_mmap_lock_irqsave(mapping, flags);
 	vma_interval_tree_foreach(mpnt, &mapping->i_mmap, pgoff, pgoff) {
 		offset = (pgoff - mpnt->vm_pgoff) << PAGE_SHIFT;
 		addr = mpnt->vm_start + offset;
@@ -460,7 +461,7 @@ void flush_dcache_page(struct page *page)
 		}
 		WARN_ON(++count == 4096);
 	}
-	flush_dcache_mmap_unlock(mapping);
+	flush_dcache_mmap_unlock_irqrestore(mapping, flags);
 }
 EXPORT_SYMBOL(flush_dcache_page);


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: spinlock recursion in aio_complete()
  2023-05-23 20:01             ` Helge Deller
@ 2023-05-23 20:06               ` Bart Van Assche
  2023-05-23 20:12                 ` Helge Deller
  2023-05-23 20:24               ` Helge Deller
  1 sibling, 1 reply; 11+ messages in thread
From: Bart Van Assche @ 2023-05-23 20:06 UTC (permalink / raw)
  To: Helge Deller, Russell King (Oracle), Dinh Nguyen
  Cc: Linux SCSI List, linux-aio, linux-parisc,
	linux-arm-kernel@lists.infradead.org

On 5/23/23 13:01, Helge Deller wrote:
> Subject: [PATCH] Fix flush_dcache_page() for usage in irq context
> 
> flush_dcache_page() can be called with IRQs disabled, e.g. from
> aio_complete().
> 
> Fix flush_dcache_page() on the arm, parisc and nios2 architectures
> to not unintentionally re-enable IRQs by using xa_lock_irqsave() instead
> of xa_lock_irq() for the flush_dcache_mmap_*lock() functions.

Please consider adding a Fixes: tag such that this patch is picked up 
automatically by the stable tree maintainers.

Thanks,

Bart.


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: spinlock recursion in aio_complete()
  2023-05-23 20:06               ` Bart Van Assche
@ 2023-05-23 20:12                 ` Helge Deller
  0 siblings, 0 replies; 11+ messages in thread
From: Helge Deller @ 2023-05-23 20:12 UTC (permalink / raw)
  To: Bart Van Assche, Russell King (Oracle), Dinh Nguyen
  Cc: Linux SCSI List, linux-aio, linux-parisc,
	linux-arm-kernel@lists.infradead.org

On 5/23/23 22:06, Bart Van Assche wrote:
> On 5/23/23 13:01, Helge Deller wrote:
>> Subject: [PATCH] Fix flush_dcache_page() for usage in irq context
>>
>> flush_dcache_page() can be called with IRQs disabled, e.g. from
>> aio_complete().
>>
>> Fix flush_dcache_page() on the arm, parisc and nios2 architectures
>> to not unintentionally re-enable IRQs by using xa_lock_irqsave() instead
>> of xa_lock_irq() for the flush_dcache_mmap_*lock() functions.
>
> Please consider adding a Fixes: tag such that this patch is picked up
> automatically by the stable tree maintainers.
Sure. I'll probably split it up as per-arch patch as well. Just wanted
to get some feedback first.

Helge

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: spinlock recursion in aio_complete()
  2023-05-23 20:01             ` Helge Deller
  2023-05-23 20:06               ` Bart Van Assche
@ 2023-05-23 20:24               ` Helge Deller
  1 sibling, 0 replies; 11+ messages in thread
From: Helge Deller @ 2023-05-23 20:24 UTC (permalink / raw)
  To: Helge Deller
  Cc: Russell King (Oracle), Dinh Nguyen, Bart Van Assche,
	Linux SCSI List, linux-aio, linux-parisc,
	linux-arm-kernel@lists.infradead.org

* Helge Deller <deller@gmx.de>:
> * Russell King (Oracle) <linux@armlinux.org.uk>:
> > On Tue, May 23, 2023 at 12:24:04PM +0200, Helge Deller wrote:
> > > On 5/22/23 23:22, Helge Deller wrote:
> > > > > > It hangs in fs/aio.c:1128, function aio_complete(), in this call:
> > > > > >      spin_lock_irqsave(&ctx->completion_lock, flags);
> > > > >
> > > > > All code that I found and that obtains ctx->completion_lock disables IRQs.
> > > > > It is not clear to me how this spinlock can be locked recursively? Is it
> > > > > sure that the "spinlock recursion" report is correct?
> > > >
> > > > Yes, it seems correct.
> > > > [...]
> > >
> > > Bart, thanks to your suggestions I was able to narrow down the problem!
> > >
> > > I got LOCKDEP working on parisc, which then reports:
> > > 	raw_local_irq_restore() called with IRQs enabled
> > > for the spin_unlock_irqrestore() in function aio_complete(), which shouldn't happen.
> > >
> > > Finally, I found that parisc's flush_dcache_page() re-enables the IRQs
> > > which leads to the spinlock hang in aio_complete().
> > >
> > > So, this is NOT a bug in aio or scsci, but we need fix in the the arch code.
> >
> > You can find some of the background to this at:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git/commit/?id=16ceff2d5dc9f0347ab5a08abff3f4647c2fee04
> >
> > which introduced flush_dcache_mmap_lock(). It looks like Hugh had
> > questions over whether this should be _irqsave() rather than _irq()
> > but I guess at the time all callers had interrupts enabled, and
> > it's only recently that someone came up with the idea of calling
> > flush_dcache_page() with interrupts disabled.
> >
> > Adding another arg to flush_dcache_mmap_lock() to save the flags
> > may be doable, but requires a patch that touches not only architectures
> > that have a private implementation, but also various code in mm/.
>
> I've tested the attached patch on parisc, and it solves the issue.
> I've not compile-tested it on arm and nios2, both seem to be
> the only other affected platforms.

For your convenience, here is the hunk I used to trigger the bug.
It triggers immediately at bootup when starting userspace.

Helge

diff --git a/fs/aio.c b/fs/aio.c
index b0b17bd098bb..6076b0ab5580 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -1127,6 +1127,7 @@ static void aio_complete(struct aio_kiocb *iocb)
 	 */
 	spin_lock_irqsave(&ctx->completion_lock, flags);

+	BUG_ON(!arch_irqs_disabled());
 	tail = ctx->tail;
 	pos = tail + AIO_EVENTS_OFFSET;

@@ -1139,7 +1140,10 @@ static void aio_complete(struct aio_kiocb *iocb)
 	*event = iocb->ki_res;

 	kunmap_atomic(ev_page);
+	BUG_ON(!arch_irqs_disabled());
+	/* the next flush_dcache_page() should keep IRQs disabled */
 	flush_dcache_page(ctx->ring_pages[pos / AIO_EVENTS_PER_PAGE]);
+	BUG_ON(!arch_irqs_disabled());

 	pr_debug("%p[%u]: %p: %p %Lx %Lx %Lx\n", ctx, tail, iocb,
 		 (void __user *)(unsigned long)iocb->ki_res.obj,



^ permalink raw reply related	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2023-05-23 20:24 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-05-21  5:43 spinlock recursion in aio_complete() Helge Deller
2023-05-22 19:28 ` Bart Van Assche
2023-05-22 20:51   ` Helge Deller
2023-05-22 20:58     ` Bart Van Assche
2023-05-22 21:22       ` Helge Deller
2023-05-23 10:24         ` Helge Deller
2023-05-23 10:51           ` Russell King (Oracle)
2023-05-23 20:01             ` Helge Deller
2023-05-23 20:06               ` Bart Van Assche
2023-05-23 20:12                 ` Helge Deller
2023-05-23 20:24               ` Helge Deller

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).