Skip to contentSkip to conversation

2.4.21-pre7 ide request races

PATCH In review Jens Axboe 3 participants 8 messages Apr 14, 2003 Block layer

lore
Share & tools
lore

Jens Axboe

author
#0 lore raw

PATCH 1/1 2.4.21-pre7 ide request races

Hi,

We've had some problems with request corruption on IDE in the past, IBM traced these to stack corruption. In various places, the IDE code does something ala:

submission:

	struct request rq;

	...
	ide_do_drive_cmd(drive, &rq, ide_wait);

ide_end_request:

	...
	blkdev_release_request()

which works fine, as long as the stack persists for the blkdev_release_request() call, but it may not if the task has already exited (CPU0 may be waiting in ide_do_drive_cmd(), CPU1 gets the completion interrupt, task is woken, and exits, CPU0 now calls blkdev_release_request()). The result is random stack corruption (or request list corruption, rq->q may have been scrippled!), not good.

The solution is to allocate the requests from the ordinary block pool, which not alone fixes the problem but also is a lot cleaner IMHO. I've added blk_get_request() which is identical to what I added in 2.5, so we can easily carry this fix forward to the 2.5 IDE base as well.

drivers/block/ll_rw_blk.c renamed from opt/kernel/linux-2.4.21-pre7/drivers/block/ll_rw_blk.c +35 -6
diff -ur -X /home/axboe/cdrom/exclude /opt/kernel/linux-2.4.21-pre7/drivers/block/ll_rw_blk.c linux-2.4.21-pre7/drivers/block/ll_rw_blk.c
@@ -427,7 +427,9 @@
 	INIT_LIST_HEAD(&q->rq[READ].free);
 	INIT_LIST_HEAD(&q->rq[WRITE].free);
 	q->rq[READ].count = 0;
+	q->rq[READ].wait = &q->wait_for_requests[READ];
 	q->rq[WRITE].count = 0;
+	q->rq[WRITE].wait = &q->wait_for_requests[WRITE];
 	q->nr_requests = 0;
 
 	si_meminfo(&si);

@@ -521,6 +523,7 @@
 		rq->cmd = rw;
 		rq->special = NULL;
 		rq->q = q;
+		rq->rl = rl;
 	}
 
 	return rq;

@@ -605,6 +608,30 @@
 	return rq;
 }
 
+struct request *blk_get_request(request_queue_t *q, int rw, int gfp_mask)
+{
+	struct request *rq;
+	unsigned long flags;
+
+	BUG_ON(rw != READ && rw != WRITE);
+
+	spin_lock_irqsave(&io_request_lock, flags);
+	rq = get_request(q, rw);
+	spin_unlock_irqrestore(&io_request_lock, flags);
+
+	if (!rq && (gfp_mask & __GFP_WAIT))
+		rq = __get_request_wait(q, rw);
+
+	if (rq) {
+		rq->buffer = NULL;
+		rq->bh = rq->bhtail = NULL;
+		rq->waiting = NULL;
+		rq->special = NULL;
+	}
+
+	return rq;
+}
+
 /* RO fail safe mechanism */
 
 static long ro_bits[MAX_BLKDEV][8];

@@ -818,20 +845,21 @@
 void blkdev_release_request(struct request *req)
 {
 	request_queue_t *q = req->q;
-	int rw = req->cmd;
+	struct request_list *rl = req->rl;
 
 	req->rq_status = RQ_INACTIVE;
 	req->q = NULL;
+	req->rl = NULL;
 
 	/*
 	 * Request may not have originated from ll_rw_blk. if not,
 	 * assume it has free buffers and check waiters
 	 */
-	if (q) {
-		list_add(&req->queue, &q->rq[rw].free);
-		if (++q->rq[rw].count >= q->batch_requests &&
-				waitqueue_active(&q->wait_for_requests[rw]))
-			wake_up(&q->wait_for_requests[rw]);
+	if (rl) {
+		list_add(&req->queue, &rl->free);
+		if (++rl->count >= q->batch_requests &&
+		    waitqueue_active(rl->wait))
+			wake_up(rl->wait);
 	}
 }
 

@@ -1518,3 +1546,4 @@
 EXPORT_SYMBOL(blk_max_pfn);
 EXPORT_SYMBOL(blk_seg_merge_ok);
 EXPORT_SYMBOL(blk_nohighio);
+EXPORT_SYMBOL(blk_get_request);
drivers/ide/ide-cd.c renamed from opt/kernel/linux-2.4.21-pre7/drivers/ide/ide-cd.c +10 -9
diff -ur -X /home/axboe/cdrom/exclude /opt/kernel/linux-2.4.21-pre7/drivers/ide/ide-cd.c linux-2.4.21-pre7/drivers/ide/ide-cd.c
@@ -1601,7 +1601,7 @@
 int cdrom_queue_packet_command(ide_drive_t *drive, struct packet_command *pc)
 {
 	struct request_sense sense;
-	struct request req;
+	struct request *req;
 	int retries = 10;
 
 	if (pc->sense == NULL)

@@ -1609,10 +1609,11 @@
 
 	/* Start of retry loop. */
 	do {
-		ide_init_drive_cmd (&req);
-		req.cmd = PACKET_COMMAND;
-		req.buffer = (char *)pc;
-		ide_do_drive_cmd(drive, &req, ide_wait);
+		req = blk_get_request(&drive->queue, WRITE, GFP_KERNEL);
+		ide_init_drive_cmd (req);
+		req->cmd = PACKET_COMMAND;
+		req->buffer = (char *)pc;
+		ide_do_drive_cmd(drive, req, ide_wait);
 		/* FIXME: we should probably abort/retry or something 
 		 * in case of failure */
 		if (pc->stat != 0) {

@@ -2487,12 +2488,12 @@
 {
 	ide_drive_t *drive = (ide_drive_t*) cdi->handle;
 	struct request_sense sense;
-	struct request req;
+	struct request *req = blk_get_request(&drive->queue, WRITE, GFP_KERNEL);
 	int ret;
 
-	ide_init_drive_cmd (&req);
-	req.cmd = RESET_DRIVE_COMMAND;
-	ret = ide_do_drive_cmd(drive, &req, ide_wait);
+	ide_init_drive_cmd (req);
+	req->cmd = RESET_DRIVE_COMMAND;
+	ret = ide_do_drive_cmd(drive, req, ide_wait);
 
 	/*
 	 * A reset will unlock the door. If it was previously locked,
drivers/ide/ide-disk.c renamed from opt/kernel/linux-2.4.21-pre7/drivers/ide/ide-disk.c +5 -4
diff -ur -X /home/axboe/cdrom/exclude /opt/kernel/linux-2.4.21-pre7/drivers/ide/ide-disk.c linux-2.4.21-pre7/drivers/ide/ide-disk.c
@@ -1414,15 +1414,16 @@
  */
 static int set_multcount(ide_drive_t *drive, int arg)
 {
-	struct request rq;
+	struct request *rq;
 
 	if (drive->special.b.set_multmode)
 		return -EBUSY;
-	ide_init_drive_cmd (&rq);
-	rq.cmd = IDE_DRIVE_CMD;
+	rq = blk_get_request(&drive->queue, WRITE, GFP_KERNEL);
+	ide_init_drive_cmd (rq);
+	rq->cmd = IDE_DRIVE_CMD;
 	drive->mult_req = arg;
 	drive->special.b.set_multmode = 1;
-	(void) ide_do_drive_cmd (drive, &rq, ide_wait);
+	(void) ide_do_drive_cmd (drive, rq, ide_wait);
 	return (drive->mult_count == arg) ? 0 : -EIO;
 }
drivers/ide/ide-io.c renamed from opt/kernel/linux-2.4.21-pre7/drivers/ide/ide-io.c +3 -1
diff -ur -X /home/axboe/cdrom/exclude /opt/kernel/linux-2.4.21-pre7/drivers/ide/ide-io.c linux-2.4.21-pre7/drivers/ide/ide-io.c
@@ -1281,8 +1281,10 @@
 
 void ide_init_drive_cmd (struct request *rq)
 {
-	memset(rq, 0, sizeof(*rq));
+	INIT_LIST_HEAD(&rq->queue);
 	rq->cmd = IDE_DRIVE_CMD;
+	rq->special = rq->buffer = NULL;
+	rq->bh = rq->bhtail = NULL;
 }
 
 EXPORT_SYMBOL(ide_init_drive_cmd);
drivers/ide/ide-taskfile.c renamed from opt/kernel/linux-2.4.21-pre7/drivers/ide/ide-taskfile.c +26 -26

112 lines not on this page · raw message

drivers/ide/ide.c renamed from opt/kernel/linux-2.4.21-pre7/drivers/ide/ide.c +4 -5
diff -ur -X /home/axboe/cdrom/exclude /opt/kernel/linux-2.4.21-pre7/drivers/ide/ide.c linux-2.4.21-pre7/drivers/ide/ide.c
@@ -1359,16 +1359,17 @@
 
 static int set_pio_mode (ide_drive_t *drive, int arg)
 {
-	struct request rq;
+	struct request *rq;
 
 	if (!HWIF(drive)->tuneproc)
 		return -ENOSYS;
 	if (drive->special.b.set_tune)
 		return -EBUSY;
-	ide_init_drive_cmd(&rq);
+	rq = blk_get_request(&drive->queue, WRITE, GFP_KERNEL);
+	ide_init_drive_cmd(rq);
 	drive->tune_req = (u8) arg;
 	drive->special.b.set_tune = 1;
-	(void) ide_do_drive_cmd(drive, &rq, ide_wait);
+	(void) ide_do_drive_cmd(drive, rq, ide_wait);
 	return 0;
 }
 

@@ -1533,7 +1534,6 @@
 {
 	int err = 0, major, minor;
 	ide_drive_t *drive;
-	struct request rq;
 	kdev_t dev;
 	ide_settings_t *setting;
 

@@ -1560,7 +1560,6 @@
 	}
 	up(&ide_setting_sem);
 
-	ide_init_drive_cmd (&rq);
 	switch (cmd) {
 		case HDIO_GETGEO:
 		{
include/linux/blkdev.h renamed from opt/kernel/linux-2.4.21-pre7/include/linux/blkdev.h +8 -5
diff -ur -X /home/axboe/cdrom/exclude /opt/kernel/linux-2.4.21-pre7/include/linux/blkdev.h linux-2.4.21-pre7/include/linux/blkdev.h
@@ -15,6 +15,12 @@
 struct elevator_s;
 typedef struct elevator_s elevator_t;
 
+struct request_list {
+	unsigned int count;
+	wait_queue_head_t *wait;
+	struct list_head free;
+};
+
 /*
  * Ok, this is an expanded form so that we can use the same
  * request for paging requests.

@@ -46,6 +52,7 @@
 	struct buffer_head * bh;
 	struct buffer_head * bhtail;
 	request_queue_t *q;
+	struct request_list *rl;
 };
 
 #include <linux/elevator.h>

@@ -70,11 +77,6 @@
  */
 #define QUEUE_NR_REQUESTS	8192
 
-struct request_list {
-	unsigned int count;
-	struct list_head free;
-};
-
 struct request_queue
 {
 	/*

@@ -217,6 +219,7 @@
 extern void generic_make_request(int rw, struct buffer_head * bh);
 extern inline request_queue_t *blk_get_queue(kdev_t dev);
 extern void blkdev_release_request(struct request *);
+extern struct request *blk_get_request(request_queue_t *, int, int);
 
 /*
  * Access functions for manipulating queue properties
Signature · 1 line
Jens Axboe
#1 lore raw
Quoted 15 lines · Jens Axboe

We've had some problems with request corruption on IDE in the past, IBM traced these to stack corruption. In various places, the IDE code does something ala:

submission:

	struct request rq;

	...
	ide_do_drive_cmd(drive, &rq, ide_wait);

ide_end_request:

	...
	blkdev_release_request()
Quoted 7 lines

which works fine, as long as the stack persists for the blkdev_release_request() call, but it may not if the task has already exited (CPU0 may be waiting in ide_do_drive_cmd(), CPU1 gets the completion interrupt, task is woken, and exits, CPU0 now calls blkdev_release_request()). The result is random stack corruption (or request list corruption, rq->q may have been scrippled!), not good.

Those locally allocated requests are foul, and the patch is a good cleanup, but is a simpler fix more appropriate?

The bug is in end_that_request_last(), yes?

void end_that_request_last(struct request *req)

{
	if (req->waiting != NULL)
		complete(req->waiting);
	req_finished_io(req);

	blkdev_release_request(req);
}

Wouldn't it be simpler to just do:

void end_that_request_last(struct request *req)

{
	struct completion *c = req->waiting;

	req_finished_io(req);
	blkdev_release_request(req);
	if (c)
		complete(c);
}

I vaguely seem to remember being told months ago why this wasn't right ;)

author
#2 lore raw
Quoted 51 lines · Andrew Morton
Quoted 15 lines · Jens Axboe

We've had some problems with request corruption on IDE in the past, IBM traced these to stack corruption. In various places, the IDE code does something ala:

submission:

	struct request rq;

	...
	ide_do_drive_cmd(drive, &rq, ide_wait);

ide_end_request:

	...
	blkdev_release_request()
Quoted 7 lines

which works fine, as long as the stack persists for the blkdev_release_request() call, but it may not if the task has already exited (CPU0 may be waiting in ide_do_drive_cmd(), CPU1 gets the completion interrupt, task is woken, and exits, CPU0 now calls blkdev_release_request()). The result is random stack corruption (or request list corruption, rq->q may have been scrippled!), not good.

Those locally allocated requests are foul, and the patch is a good cleanup, but is a simpler fix more appropriate?

The bug is in end_that_request_last(), yes?

void end_that_request_last(struct request *req)

{
	if (req->waiting != NULL)
		complete(req->waiting);
	req_finished_io(req);

	blkdev_release_request(req);
}

Wouldn't it be simpler to just do:

void end_that_request_last(struct request *req)

{
	struct completion *c = req->waiting;

	req_finished_io(req);
	blkdev_release_request(req);
	if (c)
		complete(c);
}
Quoted 1 line

I vaguely seem to remember being told months ago why this wasn't right ;)

How would that solve the problem? The request could be gone even before end_that_request_last() is run, that is the issue.

Signature · 1 line
Jens Axboe
#3 lore raw
Quoted 3 lines · Jens Axboe

How would that solve the problem? The request could be gone even before end_that_request_last() is run, that is the issue.

In that case I didn't understand your description of the bug even the tiniest little bit.

That request is sitting in the kernel stack of some process which is sleeping in wait_for_completion(). Hence it is safe memory until someone runs complete() against the completion struct.

author
#4 lore raw
Quoted 11 lines · Andrew Morton
Quoted 3 lines · Jens Axboe

How would that solve the problem? The request could be gone even before end_that_request_last() is run, that is the issue.

In that case I didn't understand your description of the bug even the tiniest little bit.

That request is sitting in the kernel stack of some process which is sleeping in wait_for_completion(). Hence it is safe memory until someone runs complete() against the completion struct.

Sorry you are right, that should fix the problem as well! Your fix is probably the better one for 2.4, less intrusive. I'll kill the stack requests in 2.5 then.

Signature · 1 line
Jens Axboe
author
#5 lore raw
Quoted 9 lines · Jens Axboe
Quoted 8 lines · Andrew Morton
Quoted 3 lines · Jens Axboe

How would that solve the problem? The request could be gone even before end_that_request_last() is run, that is the issue.

In that case I didn't understand your description of the bug even the tiniest little bit.

Quoted 7 lines
Quoted 3 lines

That request is sitting in the kernel stack of some process which is sleeping in wait_for_completion(). Hence it is safe memory until someone runs complete() against the completion struct.

Sorry you are right, that should fix the problem as well! Your fix is probably the better one for 2.4, less intrusive. I'll kill the stack requests in 2.5 then.

Marcelo,

Please apply this simpler variant. Thanks!

block/ll_rw_blk.c renamed from opt/kernel/linux-2.4.21-pre7/drivers/block/ll_rw_blk.c +4 -3
@@ -1375,11 +1403,12 @@
 
 void end_that_request_last(struct request *req)
 {
-	if (req->waiting != NULL)
-		complete(req->waiting);
-	req_finished_io(req);
+	struct completion *waiting = req->waiting;
 
+	req_finished_io(req);
 	blkdev_release_request(req);
+	if (waiting)
+		complete(waiting);
 }
 
 int __init blk_dev_init(void)
Signature · 1 line
Jens Axboe
#6 lore raw
In reply to Jens Axboe
Quoted 9 lines · Jens Axboe [[email protected]] wrote:
Quoted 8 lines · Andrew Morton
Quoted 3 lines · Jens Axboe

How would that solve the problem? The request could be gone even before end_that_request_last() is run, that is the issue.

In that case I didn't understand your description of the bug even the tiniest little bit.

Quoted 7 lines
Quoted 3 lines

That request is sitting in the kernel stack of some process which is sleeping in wait_for_completion(). Hence it is safe memory until someone runs complete() against the completion struct.

Sorry you are right, that should fix the problem as well! Your fix is probably the better one for 2.4, less intrusive. I'll kill the stack requests in 2.5 then.

In 2.5 will you include the 2.4 end_that_request_last fix proposed in this thread along with removal of requests on the stack?

-andmike
--
Michael Anderson
[email protected]

author
#7 lore raw
Quoted 14 lines · Mike Anderson
Quoted 13 lines · Jens Axboe [[email protected]] wrote:
Quoted 11 lines · Andrew Morton
Quoted 3 lines · Jens Axboe

How would that solve the problem? The request could be gone even before end_that_request_last() is run, that is the issue.

In that case I didn't understand your description of the bug even the tiniest little bit.

That request is sitting in the kernel stack of some process which is sleeping in wait_for_completion(). Hence it is safe memory until someone runs complete() against the completion struct.

Quoted 6 lines
Quoted 3 lines

Sorry you are right, that should fix the problem as well! Your fix is probably the better one for 2.4, less intrusive. I'll kill the stack requests in 2.5 then.

In 2.5 will you include the 2.4 end_that_request_last fix proposed in this thread along with removal of requests on the stack?

Yes of course. One is a good cleanup, the other prevents similar problems from other drivers.

Signature · 1 line
Jens Axboe