* 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).