Skip to contentSkip to conversation

Exiting queue and task might race to free cic

PATCH In review Nikanth Karthikesan 3 participants 5 messages Nov 19, 2008 – Nov 22, 2008 Block layer

lore
Share & tools
lore

Nikanth Karthikesan

author
#0 lore raw

PATCH 1/1 Exiting queue and task might race to free cic

Hi Jens

Looking at the bug reported here
http://thread.gmane.org/gmane.linux.kernel/722539
it looks like an exiting queue can race with an exiting task.

When a queue exits the queue lock is taken and cfq_exit_queue() would free all the cic's associated with the queue.

But when a task exits, cfq_exit_io_context() gets cic one by one and then locks the associated queue to call __cfq_exit_single_io_context. It looks like between getting a cic from the ioc and locking the queue, the queue might have exited on another cpu. Isn't this possible?

If possible, either verifying whether cic->key is still not null or q->flags does not have QUEUE_FLAG_DEAD set would fix this.

Thanks
Nikanth Karthikesan

Signed-off-by Nikanth Karthikesan <[email protected]>

---

block/cfq-iosched.c +0 -0
index 6a062ee..b9b627a 100644
@@ -1318,7 +1318,12 @@ static void cfq_exit_single_io_context(struct

io_context *ioc,
unsigned long flags;

spin_lock_irqsave(q->queue_lock, flags);
- __cfq_exit_single_io_context(cfqd, cic);
+ /*
+ * cic might have been already exited when an exiting task
+ * races with an exiting queue.
+ */
+ if (likely(cic->key))
+ __cfq_exit_single_io_context(cfqd, cic);
spin_unlock_irqrestore(q->queue_lock, flags);
}
}

Or this would also work

block/cfq-iosched.c +0 -0
index 6a062ee..7a068bd 100644
@@ -1318,7 +1318,11 @@ static void cfq_exit_single_io_context(struct

io_context *ioc,
unsigned long flags;

spin_lock_irqsave(q->queue_lock, flags);
- __cfq_exit_single_io_context(cfqd, cic);
+ /*
+ * Make sure the queue is not dead.
+ */
+ if (likely(!test_bit(QUEUE_FLAG_DEAD, &q->queue_flags)))
+ __cfq_exit_single_io_context(cfqd, cic);
spin_unlock_irqrestore(q->queue_lock, flags);
}
}

#1 lore raw
Quoted 41 lines · Nikanth Karthikesan

Hi Jens

Looking at the bug reported here
http://thread.gmane.org/gmane.linux.kernel/722539
it looks like an exiting queue can race with an exiting task.

When a queue exits the queue lock is taken and cfq_exit_queue() would free all the cic's associated with the queue.

But when a task exits, cfq_exit_io_context() gets cic one by one and then locks the associated queue to call __cfq_exit_single_io_context. It looks like between getting a cic from the ioc and locking the queue, the queue might have exited on another cpu. Isn't this possible?

If possible, either verifying whether cic->key is still not null or q->flags does not have QUEUE_FLAG_DEAD set would fix this.

Thanks
Nikanth Karthikesan

Signed-off-by Nikanth Karthikesan <[email protected]>

---

block/cfq-iosched.c +6 -1
index 6a062ee..b9b627a 100644
@@ -1318,7 +1318,12 @@ static void cfq_exit_single_io_context(struct 
 io_context *ioc,
 		unsigned long flags;
 
 		spin_lock_irqsave(q->queue_lock, flags);
-		__cfq_exit_single_io_context(cfqd, cic);
+		/*
+		 * cic might have been already exited when an exiting task
+		 * races with an exiting queue.
+		 */
+		if (likely(cic->key))
+			__cfq_exit_single_io_context(cfqd, cic);
 		spin_unlock_irqrestore(q->queue_lock, flags);
 	}
Quoted 1 line

}

Not sure this is enough, we probably need to copy the key to ensure that we get a fresh value. How does this look?

Did you actually trigger this, or is it just from code inspection?

block/cfq-iosched.c +8 -1
index 6a062ee..560cd1c 100644
@@ -1318,7 +1318,14 @@ static void cfq_exit_single_io_context(struct io_context *ioc,
 		unsigned long flags;
 
 		spin_lock_irqsave(q->queue_lock, flags);
-		__cfq_exit_single_io_context(cfqd, cic);
+
+		/*
+		 * Ensure we get a fresh copy of the ->key to prevent
+		 * race between exiting task and queue
+		 */
+		smp_read_barrier_depends();
+		if (cic->key)
+			__cfq_exit_single_io_context(cfqd, cic);
 		spin_unlock_irqrestore(q->queue_lock, flags);
 	}
 }
Signature · 1 line
Jens Axboe
#2 lore raw
Quoted 63 lines

From: Jens Axboe <[email protected]>
Date: Wed, Nov 19, 2008 03:15:31PM +0100

Quoted 41 lines · Nikanth Karthikesan

Hi Jens

Looking at the bug reported here
http://thread.gmane.org/gmane.linux.kernel/722539
it looks like an exiting queue can race with an exiting task.

When a queue exits the queue lock is taken and cfq_exit_queue() would free all the cic's associated with the queue.

But when a task exits, cfq_exit_io_context() gets cic one by one and then locks the associated queue to call __cfq_exit_single_io_context. It looks like between getting a cic from the ioc and locking the queue, the queue might have exited on another cpu. Isn't this possible?

If possible, either verifying whether cic->key is still not null or q->flags does not have QUEUE_FLAG_DEAD set would fix this.

Thanks
Nikanth Karthikesan

Signed-off-by Nikanth Karthikesan <[email protected]>

---

block/cfq-iosched.c +6 -1
index 6a062ee..b9b627a 100644
@@ -1318,7 +1318,12 @@ static void cfq_exit_single_io_context(struct 
 io_context *ioc,
 		unsigned long flags;
 
 		spin_lock_irqsave(q->queue_lock, flags);
-		__cfq_exit_single_io_context(cfqd, cic);
+		/*
+		 * cic might have been already exited when an exiting task
+		 * races with an exiting queue.
+		 */
+		if (likely(cic->key))
+			__cfq_exit_single_io_context(cfqd, cic);
 		spin_unlock_irqrestore(q->queue_lock, flags);
 	}
Quoted 1 line

}

Not sure this is enough, we probably need to copy the key to ensure that we get a fresh value. How does this look?

Did you actually trigger this, or is it just from code inspection?

block/cfq-iosched.c +1 -1
index 6a062ee..560cd1c 100644
@@ -1318,7 +1318,14 @@ static void cfq_exit_single_io_context(struct io_context *ioc,
 		unsigned long flags;
 
 		spin_lock_irqsave(q->queue_lock, flags);
-		__cfq_exit_single_io_context(cfqd, cic);
+

block/cfq-iosched.c static void cfq_exit_single_io_context(struct io_context *ioc, · line 1331

Quoted 10 lines
block/cfq-iosched.c +7 -0
+		/*
+		 * Ensure we get a fresh copy of the ->key to prevent
+		 * race between exiting task and queue
+		 */
+		smp_read_barrier_depends();
+		if (cic->key)
+			__cfq_exit_single_io_context(cfqd, cic);
 		spin_unlock_irqrestore(q->queue_lock, flags);
 	}
 }

I've seen once the oops reported (the BUG() now @ line 1247), but I've never been able to reproduce it afterwards. I think that there still is a window open for a race here:

1314 struct cfq_data *cfqd = cic->key;
1315

=====> here cfq_exit_queue() can free cfqd and assign cic->key = NULL,

       and accessing cfqd->queue is not safe.  [ If I'm not wrong :) ]

1316 if (cfqd) {
1317 struct request_queue *q = cfqd->queue;

author
#3 lore raw
In reply to Jens Axboe
Quoted 5 lines · On Wednesday 19 November 2008 19:45:31 Jens Axboe wrote:

On Wed, Nov 19 2008, Nikanth Karthikesan wrote:

Not sure this is enough, we probably need to copy the key to ensure that we get a fresh value. How does this look?

Agreed. Read barrier required. But the compiler hint, "likely" can stay?

block/cfq-iosched.c +0 -0
index 6a062ee..4504b94 100644
@@ -1318,7 +1318,15 @@ static void cfq_exit_single_io_context(struct

io_context *ioc,
unsigned long flags;

spin_lock_irqsave(q->queue_lock, flags);
- __cfq_exit_single_io_context(cfqd, cic);
+
+ /*
+ * Ensure we get a fresh copy of the ->key to prevent
+ * race between exiting task and queue
+ */
+ smp_read_barrier_depends();
+ if (likely(cic->key))
+ __cfq_exit_single_io_context(cfqd, cic);
+
spin_unlock_irqrestore(q->queue_lock, flags);
}
}

Quoted 2 lines

Did you actually trigger this, or is it just from code inspection?

No. But I am looking at another bug report on Suse Kernel where the bug is triggered during reboot when the kernel thread usb_stor_scan_thread exits.

Thanks
Nikanth Karthikesan

author
#4 lore raw

Hi Jens

Quoted 31 lines · On Thursday 20 November 2008 10:27:06 Nikanth Karthikesan wrote:
Quoted 4 lines · On Wednesday 19 November 2008 19:45:31 Jens Axboe wrote:

On Wed, Nov 19 2008, Nikanth Karthikesan wrote:

Not sure this is enough, we probably need to copy the key to ensure that we get a fresh value. How does this look?

Agreed. Read barrier required. But the compiler hint, "likely" can stay?

block/cfq-iosched.c +9 -1
index 6a062ee..4504b94 100644
@@ -1318,7 +1318,15 @@ static void cfq_exit_single_io_context(struct
 io_context *ioc,
 		unsigned long flags;
 
 		spin_lock_irqsave(q->queue_lock, flags);
-		__cfq_exit_single_io_context(cfqd, cic);
+
+		/*
+		 * Ensure we get a fresh copy of the ->key to prevent
+		 * race between exiting task and queue
+		 */
+		smp_read_barrier_depends();
+		if (likely(cic->key))
+			__cfq_exit_single_io_context(cfqd, cic);
+
 		spin_unlock_irqrestore(q->queue_lock, flags);
 	}

}

Quoted 5 lines
Quoted 1 line

Did you actually trigger this, or is it just from code inspection?

No. But I am looking at another bug report on Suse Kernel where the bug is triggered during reboot when the kernel thread usb_stor_scan_thread exits.

This patch seems to solve the above said bug report on Suse kernel. So, yes it is reproducible! Can this be merged?

Thanks
Nikanth