[RFC PATCH] dm: don't assign zero to ->bi_status of an active bio.

[NeilBrown]

Between the moment when generic_make_request() is first
called on a bio, and when bio_endio() finally gets past
bio_remaining_done(), a bio might have chained children, and might
get ->bi_status set asynchronously.

So during this time it is not safe to set it to zero.
It *is* safe to set it to any other error status as for most
purposes, one error is as good as another.

This patch is untested and RFC only.  It identifies a number of
possible problem areas in dm and often suggests fixes.

Please don't apply without careful review.

Reviewed-by: Nobody yet.
Signed-off-by: NeilBrown <neilb@xxxxxxxx>
---
 drivers/md/dm-bio-prison-v1.c | 3 ++-
 drivers/md/dm-bufio.c         | 6 ++++--
 drivers/md/dm-crypt.c         | 3 ++-
 drivers/md/dm-mpath.c         | 3 +++
 drivers/md/dm-thin.c          | 3 ++-
 drivers/md/dm-verity-target.c | 3 ++-
 drivers/md/dm-zoned-target.c  | 3 ++-
 7 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/drivers/md/dm-bio-prison-v1.c b/drivers/md/dm-bio-prison-v1.c
index 874841f0fc83..68cb4fe98e9f 100644
--- a/drivers/md/dm-bio-prison-v1.c
+++ b/drivers/md/dm-bio-prison-v1.c
@@ -238,7 +238,8 @@ void dm_cell_error(struct dm_bio_prison *prison,
 	dm_cell_release(prison, cell, &bios);
 
 	while ((bio = bio_list_pop(&bios))) {
-		bio->bi_status = error;
+		if (error)
+			bio->bi_status = error;
 		bio_endio(bio);
 	}
 }
diff --git a/drivers/md/dm-bufio.c b/drivers/md/dm-bufio.c
index 414c9af54ded..80131e098650 100644
--- a/drivers/md/dm-bufio.c
+++ b/drivers/md/dm-bufio.c
@@ -565,7 +565,8 @@ static void dmio_complete(unsigned long error, void *context)
 {
 	struct dm_buffer *b = context;
 
-	b->bio.bi_status = error ? BLK_STS_IOERR : 0;
+	if (error)
+		b->bio.bi_status = BLK_STS_IOERR;
 	b->bio.bi_end_io(&b->bio);
 }
 
@@ -614,7 +615,8 @@ static void inline_endio(struct bio *bio)
 	 */
 	bio_reset(bio);
 
-	bio->bi_status = status;
+	if (status)
+		bio->bi_status = status;
 	end_fn(bio);
 }
 
diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index 8168f737590e..3e7db4a1093e 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -1488,7 +1488,8 @@ static void crypt_dec_pending(struct dm_crypt_io *io)
 	else
 		kfree(io->integrity_metadata);
 
-	base_bio->bi_status = error;
+	if (error)
+		base_bio->bi_status = error;
 	bio_endio(base_bio);
 }
 
diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
index 7d3e572072f5..6f793d7d29f0 100644
--- a/drivers/md/dm-mpath.c
+++ b/drivers/md/dm-mpath.c
@@ -651,6 +651,9 @@ static int __multipath_map_bio(struct multipath *m, struct bio *bio,
 
 	mpio->pgpath = pgpath;
 
+	/* FIXME If the bio was chained, this could
+	 * over-write an error... is that possible?
+	 */
 	bio->bi_status = 0;
 	bio_set_dev(bio, pgpath->path.dev->bdev);
 	bio->bi_opf |= REQ_FAILFAST_TRANSPORT;
diff --git a/drivers/md/dm-thin.c b/drivers/md/dm-thin.c
index 629c555890c1..16d4c386cc56 100644
--- a/drivers/md/dm-thin.c
+++ b/drivers/md/dm-thin.c
@@ -563,7 +563,8 @@ static void error_bio_list(struct bio_list *bios, blk_status_t error)
 	struct bio *bio;
 
 	while ((bio = bio_list_pop(bios))) {
-		bio->bi_status = error;
+		if (error)
+			bio->bi_status = error;
 		bio_endio(bio);
 	}
 }
diff --git a/drivers/md/dm-verity-target.c b/drivers/md/dm-verity-target.c
index aedb8222836b..86a1d501d915 100644
--- a/drivers/md/dm-verity-target.c
+++ b/drivers/md/dm-verity-target.c
@@ -503,7 +503,8 @@ static void verity_finish_io(struct dm_verity_io *io, blk_status_t status)
 	struct bio *bio = dm_bio_from_per_bio_data(io, v->ti->per_io_data_size);
 
 	bio->bi_end_io = io->orig_bi_end_io;
-	bio->bi_status = status;
+	if (status)
+		bio->bi_status = status;
 
 	verity_fec_finish_io(io);
 
diff --git a/drivers/md/dm-zoned-target.c b/drivers/md/dm-zoned-target.c
index caff02caf083..cfdacaf79492 100644
--- a/drivers/md/dm-zoned-target.c
+++ b/drivers/md/dm-zoned-target.c
@@ -637,7 +637,8 @@ static int dmz_end_io(struct dm_target *ti, struct bio *bio, blk_status_t *error
 		return DM_ENDIO_INCOMPLETE;
 
 	/* Done */
-	bio->bi_status = bioctx->status;
+	if (bioctx->status)
+		bio->bi_status = bioctx->status;
 
 	if (bioctx->zone) {
 		struct dm_zone *zone = bioctx->zone;
-- 
2.14.0.rc0.dirty

Attachment: signature.asc
Description: PGP signature