From: Shida Zhang Don't call bio->bi_end_io() directly. Use the bio_endio() helper function instead, which handles completion more safely and uniformly. Suggested-by: Christoph Hellwig Reviewed-by: Christoph Hellwig Signed-off-by: Shida Zhang --- drivers/md/bcache/request.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/md/bcache/request.c b/drivers/md/bcache/request.c index af345dc6fde..82fdea7dea7 100644 --- a/drivers/md/bcache/request.c +++ b/drivers/md/bcache/request.c @@ -1104,7 +1104,7 @@ static void detached_dev_end_io(struct bio *bio) } kfree(ddip); - bio->bi_end_io(bio); + bio_endio(bio); } static void detached_dev_do_request(struct bcache_device *d, struct bio *bio, @@ -1121,7 +1121,7 @@ static void detached_dev_do_request(struct bcache_device *d, struct bio *bio, ddip = kzalloc(sizeof(struct detached_dev_io_private), GFP_NOIO); if (!ddip) { bio->bi_status = BLK_STS_RESOURCE; - bio->bi_end_io(bio); + bio_endio(bio); return; } @@ -1136,7 +1136,7 @@ static void detached_dev_do_request(struct bcache_device *d, struct bio *bio, if ((bio_op(bio) == REQ_OP_DISCARD) && !bdev_max_discard_sectors(dc->bdev)) - bio->bi_end_io(bio); + detached_dev_end_io(bio); else submit_bio_noacct(bio); } -- 2.34.1 From: Shida Zhang Now that all potential callers of bio_chain_endio have been eliminated, completely prohibit any future calls to this function. Suggested-by: Ming Lei Suggested-by: Andreas Gruenbacher Suggested-by: Christoph Hellwig Reviewed-by: Christoph Hellwig Signed-off-by: Shida Zhang --- block/bio.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/block/bio.c b/block/bio.c index b3a79285c27..d236ca35271 100644 --- a/block/bio.c +++ b/block/bio.c @@ -320,9 +320,13 @@ static struct bio *__bio_chain_endio(struct bio *bio) return parent; } +/* + * This function should only be used as a flag and must never be called. + * If execution reaches here, it indicates a serious programming error. + */ static void bio_chain_endio(struct bio *bio) { - bio_endio(__bio_chain_endio(bio)); + BUG(); } /** -- 2.34.1 From: Shida Zhang Andreas point out that multiple completions can race setting bi_status. If __bio_chain_endio() is called concurrently from multiple threads accessing the same parent bio, it should use WRITE_ONCE()/READ_ONCE() to access parent->bi_status and avoid data races. On x86 and ARM, these macros compile to the same instruction as a normal write, but they may be required on other architectures to prevent tearing, and to ensure the compiler does not add or remove memory accesses under the assumption that the values are not accessed concurrently. Adopting a cmpxchg approach, as used in other code paths, resolves all these issues, as suggested by Christoph. Suggested-by: Andreas Gruenbacher Suggested-by: Christoph Hellwig Suggested-by: Caleb Sander Mateos Reviewed-by: Christoph Hellwig Signed-off-by: Shida Zhang --- block/bio.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/block/bio.c b/block/bio.c index d236ca35271..8b4b6b4e210 100644 --- a/block/bio.c +++ b/block/bio.c @@ -314,8 +314,9 @@ static struct bio *__bio_chain_endio(struct bio *bio) { struct bio *parent = bio->bi_private; - if (bio->bi_status && !parent->bi_status) - parent->bi_status = bio->bi_status; + if (bio->bi_status) + cmpxchg(&parent->bi_status, 0, bio->bi_status); + bio_put(bio); return parent; } -- 2.34.1