Re: spinlock recursion in aio_complete()

[Helge Deller <deller@gmx.de>]

* 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,