block: fix mis-synchronisation in blkdev_issue_zeroout()

PATCH In review Lukas Czerner 3 participants 5 messages Mar 11, 2011

Subsystems (2)

VFS Block layer

lore

Share & tools

lore

Lukas Czerner

author

2011-03-11 08:06 UTC #0 lore raw

PATCH v2 1/1 block: fix mis-synchronisation in blkdev_issue_zeroout()

BZ29402
https://bugzilla.kernel.org/show_bug.cgi?id=29402

We can hit serious mis-synchronization in bio completion path of blkdev_issue_zeroout() leading to a panic.

The problem is that when we are going to wait_for_completion() in blkdev_issue_zeroout() we check if the bb.done equals issued (number of submitted bios). If it does, we can skip the wait_for_completition() and just out of the function since there is nothing to wait for. However, there is a ordering problem because bio_batch_end_io() is calling atomic_inc(&bb->done) before complete(), hence it might seem to blkdev_issue_zeroout() that all bios has been completed and exit. At this point when bio_batch_end_io() is going to call complete(bb->wait), bb and wait does not longer exist since it was allocated on stack in blkdev_issue_zeroout() ==> panic!

(thread 1)                      (thread 2)
bio_batch_end_io()              blkdev_issue_zeroout()
  if(bb) {                      ...
    if (bb->end_io)             ...
      bb->end_io(bio, err);     ...
    atomic_inc(&bb->done);      ...
    ...                         while (issued != atomic_read(&bb.done))
    ...                         (let issued == bb.done)
    ...                         (do the rest of the function)
    ...                         return ret;
    complete(bb->wait);
    ^^^^^^^^
    panic

We can fix this easily by simplifying bio_batch and completion counting.

Also remove bio_end_io_t *end_io since it is not used.

Signed-off-by Lukas Czerner <[email protected]>

Reported-by Eric Whitney <[email protected]>

Tested-by Eric Whitney <[email protected]>

CC Jens Axboe <[email protected]>

CC Dmitry Monakhov <[email protected]>

CC Jeff Moyer <[email protected]>

---
 block/blk-lib.c |   19 +++++++------------
 1 files changed, 7 insertions(+), 12 deletions(-)

block/blk-lib.c +7 ~~-12~~

index eec78be..bd3e8df 100644
@@ -109,7 +109,6 @@ struct bio_batch
 	atomic_t 		done;
 	unsigned long 		flags;
 	struct completion 	*wait;
-	bio_end_io_t		*end_io;
 };
 
 static void bio_batch_end_io(struct bio *bio, int err)

@@ -122,12 +121,9 @@ static void bio_batch_end_io(struct bio *bio, int err)
 		else
 			clear_bit(BIO_UPTODATE, &bb->flags);
 	}
-	if (bb) {
-		if (bb->end_io)
-			bb->end_io(bio, err);
-		atomic_inc(&bb->done);
-		complete(bb->wait);
-	}
+	if (bb)
+		if (atomic_dec_and_test(&bb->done))
+			complete(bb->wait);
 	bio_put(bio);
 }

@@ -150,13 +146,12 @@ int blkdev_issue_zeroout(struct block_device *bdev, sector_t sector,
 	int ret;
 	struct bio *bio;
 	struct bio_batch bb;
-	unsigned int sz, issued = 0;
+	unsigned int sz;
 	DECLARE_COMPLETION_ONSTACK(wait);
 
-	atomic_set(&bb.done, 0);
+	atomic_set(&bb.done, 1);
 	bb.flags = 1 << BIO_UPTODATE;
 	bb.wait = &wait;
-	bb.end_io = NULL;
 
 submit:
 	ret = 0;

@@ -185,12 +180,12 @@ submit:
 				break;
 		}
 		ret = 0;
-		issued++;
+		atomic_inc(&bb.done);
 		submit_bio(WRITE, bio);
 	}
 
 	/* Wait for bios in-flight */
-	while (issued != atomic_read(&bb.done))
+	if (!atomic_dec_and_test(&bb.done))
 		wait_for_completion(&wait);
 
 	if (!test_bit(BIO_UPTODATE, &bb.flags))

Signature · 1 line

1.7.4

Jens Axboe

maintainer

2011-03-11 09:29 UTC #1 lore raw

Quoted 31 lines · Lukas Czerner

BZ29402
https://bugzilla.kernel.org/show_bug.cgi?id=29402
We can hit serious mis-synchronization in bio completion path of blkdev_issue_zeroout() leading to a panic.
The problem is that when we are going to wait_for_completion() in blkdev_issue_zeroout() we check if the bb.done equals issued (number of submitted bios). If it does, we can skip the wait_for_completition() and just out of the function since there is nothing to wait for. However, there is a ordering problem because bio_batch_end_io() is calling atomic_inc(&bb->done) before complete(), hence it might seem to blkdev_issue_zeroout() that all bios has been completed and exit. At this point when bio_batch_end_io() is going to call complete(bb->wait), bb and wait does not longer exist since it was allocated on stack in blkdev_issue_zeroout() ==> panic!
(thread 1)                      (thread 2)
bio_batch_end_io()              blkdev_issue_zeroout()
  if(bb) {                      ...
    if (bb->end_io)             ...
      bb->end_io(bio, err);     ...
    atomic_inc(&bb->done);      ...
    ...                         while (issued != atomic_read(&bb.done))
    ...                         (let issued == bb.done)
    ...                         (do the rest of the function)
    ...                         return ret;
    complete(bb->wait);
    ^^^^^^^^
    panic

Quoted 3 lines

We can fix this easily by simplifying bio_batch and completion counting.
Also remove bio_end_io_t *end_io since it is not used.

Thanks Lukas, I'll expedite this for 2.6.38!

Signature · 1 line

Jens Axboe

Jeff Moyer

Reviewed-by on v2 1/1

2011-03-11 14:23 UTC #2 lore raw

In reply to Lukas Czerner

Quoted 60 lines · Lukas Czerner

BZ29402
https://bugzilla.kernel.org/show_bug.cgi?id=29402
We can hit serious mis-synchronization in bio completion path of blkdev_issue_zeroout() leading to a panic.
The problem is that when we are going to wait_for_completion() in blkdev_issue_zeroout() we check if the bb.done equals issued (number of submitted bios). If it does, we can skip the wait_for_completition() and just out of the function since there is nothing to wait for. However, there is a ordering problem because bio_batch_end_io() is calling atomic_inc(&bb->done) before complete(), hence it might seem to blkdev_issue_zeroout() that all bios has been completed and exit. At this point when bio_batch_end_io() is going to call complete(bb->wait), bb and wait does not longer exist since it was allocated on stack in blkdev_issue_zeroout() ==> panic!
(thread 1)                      (thread 2)
bio_batch_end_io()              blkdev_issue_zeroout()
  if(bb) {                      ...
    if (bb->end_io)             ...
      bb->end_io(bio, err);     ...
    atomic_inc(&bb->done);      ...
    ...                         while (issued != atomic_read(&bb.done))
    ...                         (let issued == bb.done)
    ...                         (do the rest of the function)
    ...                         return ret;
    complete(bb->wait);
    ^^^^^^^^
    panic
We can fix this easily by simplifying bio_batch and completion counting.
Also remove bio_end_io_t *end_io since it is not used.
Signed-off-by Lukas Czerner <[email protected]>
Reported-by Eric Whitney <[email protected]>
Tested-by Eric Whitney <[email protected]>
CC Jens Axboe <[email protected]>
CC Dmitry Monakhov <[email protected]>
CC Jeff Moyer <[email protected]>
---
 block/blk-lib.c |   19 +++++++------------
 1 files changed, 7 insertions(+), 12 deletions(-)
block/blk-lib.c +0 -1
index eec78be..bd3e8df 100644
@@ -109,7 +109,6 @@ struct bio_batch
 	atomic_t 		done;
 	unsigned long 		flags;
 	struct completion 	*wait;
-	bio_end_io_t		*end_io;
 };
 
 static void bio_batch_end_io(struct bio *bio, int err)
@@ -122,12 +121,9 @@ static void bio_batch_end_io(struct bio *bio, int err)
 		else
 			clear_bit(BIO_UPTODATE, &bb->flags);

block/blk-lib.c static void bio_batch_end_io(struct bio *bio, int err) · line 126

Quoted 10 lines

block/blk-lib.c +3 -6

 	}
-	if (bb) {
-		if (bb->end_io)
-			bb->end_io(bio, err);
-		atomic_inc(&bb->done);
-		complete(bb->wait);
-	}
+	if (bb)
+		if (atomic_dec_and_test(&bb->done))
+			complete(bb->wait);

I think bb will always be set here, no real need to check.

Anyway, I though I already added my:

Reviewed-by Jeff Moyer <[email protected]>

to this. No?

Cheers,
Jeff

Lukas Czerner

author

2011-03-11 14:31 UTC #3 lore raw

Quoted 75 lines · Jeff Moyer

Quoted 60 lines · Lukas Czerner
BZ29402
https://bugzilla.kernel.org/show_bug.cgi?id=29402
We can hit serious mis-synchronization in bio completion path of blkdev_issue_zeroout() leading to a panic.
The problem is that when we are going to wait_for_completion() in blkdev_issue_zeroout() we check if the bb.done equals issued (number of submitted bios). If it does, we can skip the wait_for_completition() and just out of the function since there is nothing to wait for. However, there is a ordering problem because bio_batch_end_io() is calling atomic_inc(&bb->done) before complete(), hence it might seem to blkdev_issue_zeroout() that all bios has been completed and exit. At this point when bio_batch_end_io() is going to call complete(bb->wait), bb and wait does not longer exist since it was allocated on stack in blkdev_issue_zeroout() ==> panic!
(thread 1)                      (thread 2)
bio_batch_end_io()              blkdev_issue_zeroout()
  if(bb) {                      ...
    if (bb->end_io)             ...
      bb->end_io(bio, err);     ...
    atomic_inc(&bb->done);      ...
    ...                         while (issued != atomic_read(&bb.done))
    ...                         (let issued == bb.done)
    ...                         (do the rest of the function)
    ...                         return ret;
    complete(bb->wait);
    ^^^^^^^^
    panic
We can fix this easily by simplifying bio_batch and completion counting.
Also remove bio_end_io_t *end_io since it is not used.
Signed-off-by Lukas Czerner <[email protected]>
Reported-by Eric Whitney <[email protected]>
Tested-by Eric Whitney <[email protected]>
CC Jens Axboe <[email protected]>
CC Dmitry Monakhov <[email protected]>
CC Jeff Moyer <[email protected]>
---
 block/blk-lib.c |   19 +++++++------------
 1 files changed, 7 insertions(+), 12 deletions(-)
block/blk-lib.c +0 -1
index eec78be..bd3e8df 100644
@@ -109,7 +109,6 @@ struct bio_batch
 	atomic_t 		done;
 	unsigned long 		flags;
 	struct completion 	*wait;
-	bio_end_io_t		*end_io;
 };
 
 static void bio_batch_end_io(struct bio *bio, int err)
@@ -122,12 +121,9 @@ static void bio_batch_end_io(struct bio *bio, int err)
 		else
 			clear_bit(BIO_UPTODATE, &bb->flags);
block/blk-lib.c static void bio_batch_end_io(struct bio *bio, int err) · line 126
Quoted 10 lines
block/blk-lib.c +3 -6
 	}
-	if (bb) {
-		if (bb->end_io)
-			bb->end_io(bio, err);
-		atomic_inc(&bb->done);
-		complete(bb->wait);
-	}
+	if (bb)
+		if (atomic_dec_and_test(&bb->done))
+			complete(bb->wait);
I think bb will always be set here, no real need to check.

Quoted 8 lines

Anyway, I though I already added my:
Reviewed-by Jeff Moyer <[email protected]>
to this. No?
Cheers,
Jeff

Yes, you did and I forgot to add it into the patch. Sorry about that.

Thanks!
-Lukas

Jens Axboe

maintainer

2011-03-11 14:36 UTC #4 lore raw

Quoted 79 lines · Lukas Czerner

Quoted 77 lines · Jeff Moyer
Quoted 60 lines · Lukas Czerner
BZ29402
https://bugzilla.kernel.org/show_bug.cgi?id=29402
We can hit serious mis-synchronization in bio completion path of blkdev_issue_zeroout() leading to a panic.
The problem is that when we are going to wait_for_completion() in blkdev_issue_zeroout() we check if the bb.done equals issued (number of submitted bios). If it does, we can skip the wait_for_completition() and just out of the function since there is nothing to wait for. However, there is a ordering problem because bio_batch_end_io() is calling atomic_inc(&bb->done) before complete(), hence it might seem to blkdev_issue_zeroout() that all bios has been completed and exit. At this point when bio_batch_end_io() is going to call complete(bb->wait), bb and wait does not longer exist since it was allocated on stack in blkdev_issue_zeroout() ==> panic!
(thread 1)                      (thread 2)
bio_batch_end_io()              blkdev_issue_zeroout()
  if(bb) {                      ...
    if (bb->end_io)             ...
      bb->end_io(bio, err);     ...
    atomic_inc(&bb->done);      ...
    ...                         while (issued != atomic_read(&bb.done))
    ...                         (let issued == bb.done)
    ...                         (do the rest of the function)
    ...                         return ret;
    complete(bb->wait);
    ^^^^^^^^
    panic
We can fix this easily by simplifying bio_batch and completion counting.
Also remove bio_end_io_t *end_io since it is not used.
Signed-off-by Lukas Czerner <[email protected]>
Reported-by Eric Whitney <[email protected]>
Tested-by Eric Whitney <[email protected]>
CC Jens Axboe <[email protected]>
CC Dmitry Monakhov <[email protected]>
CC Jeff Moyer <[email protected]>
---
 block/blk-lib.c |   19 +++++++------------
 1 files changed, 7 insertions(+), 12 deletions(-)
block/blk-lib.c +0 -1
index eec78be..bd3e8df 100644
@@ -109,7 +109,6 @@ struct bio_batch
 	atomic_t 		done;
 	unsigned long 		flags;
 	struct completion 	*wait;
-	bio_end_io_t		*end_io;
 };
 
 static void bio_batch_end_io(struct bio *bio, int err)
@@ -122,12 +121,9 @@ static void bio_batch_end_io(struct bio *bio, int err)
 		else
 			clear_bit(BIO_UPTODATE, &bb->flags);
block/blk-lib.c static void bio_batch_end_io(struct bio *bio, int err) · line 126
Quoted 10 lines
block/blk-lib.c +3 -6
 	}
-	if (bb) {
-		if (bb->end_io)
-			bb->end_io(bio, err);
-		atomic_inc(&bb->done);
-		complete(bb->wait);
-	}
+	if (bb)
+		if (atomic_dec_and_test(&bb->done))
+			complete(bb->wait);
I think bb will always be set here, no real need to check.
Anyway, I though I already added my:

Quoted 8 lines

Quoted 6 lines
Reviewed-by Jeff Moyer <[email protected]>
to this. No?
Cheers,
Jeff
Yes, you did and I forgot to add it into the patch. Sorry about that.

No worries, I added it now.

Signature · 1 line

Jens Axboe