Skip to contentSkip to conversation

Fix lock inversion aio_kick_handler()

RFC PATCH In review Zach Brown 3 participants 6 messages Jul 29, 2006 – Nov 9, 2006

Subsystems (2)
VFS Aio
lore
Share & tools
lore

Zach Brown

author
#0 lore raw

RFC PATCH 1/1 Fix lock inversion aio_kick_handler()

Fix lock inversion aio_kick_handler()

lockdep found a AB BC CA lock inversion in retry-based AIO:

1) The task struct's alloc_lock (A) is acquired in process context with interrupts enabled. An interrupt might arrive and call wake_up() which grabs the wait queue's q->lock (B).

2) When performing retry-based AIO the AIO core registers aio_wake_function() as the wake funtion for iocb->ki_wait. It is called with the wait queue's q->lock (B) held and then tries to add the iocb to the run list after acquiring the ctx_lock (C).

3) aio_kick_handler() holds the ctx_lock (C) while acquiring the alloc_lock (A) via lock_task() and unuse_mm(). Lockdep emits a warning saying that we're trying to connect the irq-safe q->lock to the irq-unsafe alloc_lock via ctx_lock.

This fixes the inversion by calling unuse_mm() in the AIO kick handing path after we've released the ctx_lock.

Signed-off-by Zach Brown <[email protected]>

---

Ben, can you remember why we put unuse_mm() inside the ctx_lock? Is that intentional and still needed?

fs/aio.c +1 -4
Index: 2.6.18-rc2-mm1-cmdepoll/fs/aio.c
===================================================================
@@ -598,9 +598,6 @@ static void use_mm(struct mm_struct *mm)
  *	by the calling kernel thread
  *	(Note: this routine is intended to be called only
  *	from a kernel thread context)
- *
- * Comments: Called with ctx->ctx_lock held. This nests
- * task_lock instead ctx_lock.
  */
 static void unuse_mm(struct mm_struct *mm)
 {

@@ -866,8 +863,8 @@ static void aio_kick_handler(void *data)
 	use_mm(ctx->mm);
 	spin_lock_irq(&ctx->ctx_lock);
 	requeue =__aio_run_iocbs(ctx);
- 	unuse_mm(ctx->mm);
 	spin_unlock_irq(&ctx->ctx_lock);
+ 	unuse_mm(ctx->mm);
 	set_fs(oldfs);
 	/*
 	 * we're in a worker thread already, don't use queue_delayed_work,
maintainer
#1 lore raw
Quoted 1 line · Zach Brown

Fix lock inversion aio_kick_handler()

Doh. Unfortunately, this patch isn't entirely correct as it could race with __put_ioctx() which sets ioctx->mm = NULL. Something like the following should do the trick:

	struct mm_struct *mm;

...

fs/aio.c static void aio_kick_handler(void *data) · line 869

Quoted 1 line
fs/aio.c +0 -1
- 	unuse_mm(ctx->mm);

+ mm = ctx->mm;

Quoted 1 line

spin_unlock_irq(&ctx->ctx_lock);

+ unuse_mm(mm);

fs/aio.c static void aio_kick_handler(void *data) · line 869

Quoted 2 lines
fs/aio.c +0 -0
 	set_fs(oldfs);
 	/*

...

-ben

Signature · 2 lines
"Time is of no importance, Mr. President, only life is important."
Don't Email: <[email protected]>.
author
#2 lore raw
Quoted 5 lines · Benjamin LaHaise
Quoted 1 line · Zach Brown

Fix lock inversion aio_kick_handler()

Doh. Unfortunately, this patch isn't entirely correct as it could race with __put_ioctx() which sets ioctx->mm = NULL.

Aha, yeah, that's what I was missing. Thanks.

Quoted 1 line

Something like the following should do the trick:

Cool, I'll respin and send it out.

- z

#3 lore raw

==> On Fri, 28 Jul 2006 19:02:23 -0700, Zach Brown <[email protected]> said:

Zach> Benjamin LaHaise wrote:
Zach> > On Fri, Jul 28, 2006 at 05:10:32PM -0700, Zach Brown wrote:
Zach> >> Fix lock inversion aio_kick_handler()
Zach> >
Zach> > Doh. Unfortunately, this patch isn't entirely correct as it Zach> could race with > __put_ioctx() which sets ioctx->mm = NULL.

Zach> Aha, yeah, that's what I was missing. Thanks.

Zach> > Something like the following should do the trick:

Zach> Cool, I'll respin and send it out.

Did you ever resend this patch, Zach? It doesn't appear to be in current kernels. I'm still running into the lockdep warnings.

-Jeff

fs/aio.c renamed from fs/aio.c.orig +3 -1
@@ -864,13 +864,15 @@ static void aio_kick_handler(void *data)
 	struct kioctx *ctx = data;
 	mm_segment_t oldfs = get_fs();
 	int requeue;
+	struct mm_struct *mm;
 
 	set_fs(USER_DS);
 	use_mm(ctx->mm);
 	spin_lock_irq(&ctx->ctx_lock);
 	requeue =__aio_run_iocbs(ctx);
- 	unuse_mm(ctx->mm);
+	mm = ctx->mm;
 	spin_unlock_irq(&ctx->ctx_lock);
+ 	unuse_mm(mm);
 	set_fs(oldfs);
 	/*
 	 * we're in a worker thread already, don't use queue_delayed_work,
author
#4 lore raw
Quoted 11 lines

Zach> > Doh. Unfortunately, this patch isn't entirely correct as it Zach> could race with > __put_ioctx() which sets ioctx->mm = NULL.

Zach> Aha, yeah, that's what I was missing. Thanks.

Zach> > Something like the following should do the trick:

Zach> Cool, I'll respin and send it out.

Did you ever resend this patch, Zach?

Sadly, no. I vaguely remember thinking that the refcounting was pretty messed up in these paths and that more than just this patch was needed.
I don't remember the details.

Maybe I should take a look again :/.

Quoted 1 line

current kernels. I'm still running into the lockdep warnings.

When doing what? Working with that IO_CMD_EPOLL_WAIT patch?

- z

#5 lore raw

==> On Thu, 09 Nov 2006 14:51:59 -0800, Zach Brown <[email protected]> said:

Zach> > Zach> > Doh. Unfortunately, this patch isn't entirely correct Zach> as it > Zach> could race with > __put_ioctx() which sets Zach> ioctx->mm = NULL.
Zach> >
Zach> > Zach> Aha, yeah, that's what I was missing. Thanks. Zach> >
Zach> > Zach> > Something like the following should do the trick:
Zach> >
Zach> > Zach> Cool, I'll respin and send it out.
Zach> >
Zach> > Did you ever resend this patch, Zach?

Zach> Sadly, no. I vaguely remember thinking that the refcounting was Zach> pretty messed up in these paths and that more than just this Zach> patch was needed. I don't remember the details.

Zach> Maybe I should take a look again :/.

Zach> > current kernels. I'm still running into the lockdep warnings.

Zach> When doing what? Working with that IO_CMD_EPOLL_WAIT patch?

Yep. =)

-Jeff