[PATCH] blk-mq: Avoid that submitting a bio concurrently with device removal triggers a crash

[PATCH] blk-mq: Avoid that submitting a bio concurrently with device removal triggers a crash

[Bart Van Assche]

Because blkcg_exit_queue() is now called from inside blk_cleanup_queue()
it is no longer safe to access cgroup information during or after the
blk_cleanup_queue() call. Hence protect the generic_make_request_checks()
call with blk_queue_enter() / blk_queue_exit().

Reported-by: Ming Lei <ming.lei@redhat.com>
Fixes: a063057d7c73 ("block: Fix a race between request queue removal and the block cgroup controller")
Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
Cc: Ming Lei <ming.lei@redhat.com>
Cc: Joseph Qi <joseph.qi@linux.alibaba.com>
---
 block/blk-core.c | 32 ++++++++++++++++++++++++++------
 1 file changed, 26 insertions(+), 6 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 34e2f2227fd9..a330cd2829e1 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -2386,8 +2386,19 @@ blk_qc_t generic_make_request(struct bio *bio)
 	 * yet.
 	 */
 	struct bio_list bio_list_on_stack[2];
+	blk_mq_req_flags_t flags = bio->bi_opf & REQ_NOWAIT ?
+		BLK_MQ_REQ_NOWAIT : 0;
+	struct request_queue *q = bio->bi_disk->queue;
 	blk_qc_t ret = BLK_QC_T_NONE;
 
+	if (blk_queue_enter(q, flags) < 0) {
+		if (!blk_queue_dying(q) && (bio->bi_opf & REQ_NOWAIT))
+			bio_wouldblock_error(bio);
+		else
+			bio_io_error(bio);
+		return ret;
+	}
+
 	if (!generic_make_request_checks(bio))
 		goto out;
 
@@ -2424,11 +2435,20 @@ blk_qc_t generic_make_request(struct bio *bio)
 	bio_list_init(&bio_list_on_stack[0]);
 	current->bio_list = bio_list_on_stack;
 	do {
-		struct request_queue *q = bio->bi_disk->queue;
-		blk_mq_req_flags_t flags = bio->bi_opf & REQ_NOWAIT ?
-			BLK_MQ_REQ_NOWAIT : 0;
+		bool enter_succeeded = true;
 
-		if (likely(blk_queue_enter(q, flags) == 0)) {
+		if (unlikely(q != bio->bi_disk->queue)) {
+			flags = bio->bi_opf & REQ_NOWAIT ? BLK_MQ_REQ_NOWAIT :
+				0;
+			blk_queue_exit(q);
+			q = bio->bi_disk->queue;
+			if (blk_queue_enter(q, flags) < 0) {
+				enter_succeeded = false;
+				q = NULL;
+			}
+		}
+
+		if (enter_succeeded) {
 			struct bio_list lower, same;
 
 			/* Create a fresh bio_list for all subordinate requests */
@@ -2436,8 +2456,6 @@ blk_qc_t generic_make_request(struct bio *bio)
 			bio_list_init(&bio_list_on_stack[0]);
 			ret = q->make_request_fn(q, bio);
 
-			blk_queue_exit(q);
-
 			/* sort new bios into those for a lower level
 			 * and those for the same level
 			 */
@@ -2464,6 +2482,8 @@ blk_qc_t generic_make_request(struct bio *bio)
 	current->bio_list = NULL; /* deactivate */
 
 out:
+	if (q)
+		blk_queue_exit(q);
 	return ret;
 }
 EXPORT_SYMBOL(generic_make_request);
-- 
2.16.2

Re: [PATCH] blk-mq: Avoid that submitting a bio concurrently with device removal triggers a crash

[Ming Lei]

On Tue, Apr 10, 2018 at 02:30:35PM -0600, Bart Van Assche wrote:
> Because blkcg_exit_queue() is now called from inside blk_cleanup_queue()
> it is no longer safe to access cgroup information during or after the
> blk_cleanup_queue() call. Hence protect the generic_make_request_checks()
> call with blk_queue_enter() / blk_queue_exit().
> 
> Reported-by: Ming Lei <ming.lei@redhat.com>
> Fixes: a063057d7c73 ("block: Fix a race between request queue removal and the block cgroup controller")
> Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
> Cc: Ming Lei <ming.lei@redhat.com>
> Cc: Joseph Qi <joseph.qi@linux.alibaba.com>
> ---
>  block/blk-core.c | 32 ++++++++++++++++++++++++++------
>  1 file changed, 26 insertions(+), 6 deletions(-)
> 
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 34e2f2227fd9..a330cd2829e1 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -2386,8 +2386,19 @@ blk_qc_t generic_make_request(struct bio *bio)
>  	 * yet.
>  	 */
>  	struct bio_list bio_list_on_stack[2];
> +	blk_mq_req_flags_t flags = bio->bi_opf & REQ_NOWAIT ?
> +		BLK_MQ_REQ_NOWAIT : 0;
> +	struct request_queue *q = bio->bi_disk->queue;
>  	blk_qc_t ret = BLK_QC_T_NONE;
>  
> +	if (blk_queue_enter(q, flags) < 0) {

As I mentioned last time, the queue pointer has to be checked before
calling blk_queue_enter(), since it isn't difficult to trigger the
failure log of 'generic_make_request: Trying to access nonexistent
block-device'.

-- 
Ming

Re: [PATCH] blk-mq: Avoid that submitting a bio concurrently with device removal triggers a crash

[Jens Axboe]

On 4/10/18 8:05 PM, Ming Lei wrote:
> On Tue, Apr 10, 2018 at 02:30:35PM -0600, Bart Van Assche wrote:
>> Because blkcg_exit_queue() is now called from inside blk_cleanup_queue()
>> it is no longer safe to access cgroup information during or after the
>> blk_cleanup_queue() call. Hence protect the generic_make_request_checks()
>> call with blk_queue_enter() / blk_queue_exit().
>>
>> Reported-by: Ming Lei <ming.lei@redhat.com>
>> Fixes: a063057d7c73 ("block: Fix a race between request queue removal and the block cgroup controller")
>> Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
>> Cc: Ming Lei <ming.lei@redhat.com>
>> Cc: Joseph Qi <joseph.qi@linux.alibaba.com>
>> ---
>>  block/blk-core.c | 32 ++++++++++++++++++++++++++------
>>  1 file changed, 26 insertions(+), 6 deletions(-)
>>
>> diff --git a/block/blk-core.c b/block/blk-core.c
>> index 34e2f2227fd9..a330cd2829e1 100644
>> --- a/block/blk-core.c
>> +++ b/block/blk-core.c
>> @@ -2386,8 +2386,19 @@ blk_qc_t generic_make_request(struct bio *bio)
>>  	 * yet.
>>  	 */
>>  	struct bio_list bio_list_on_stack[2];
>> +	blk_mq_req_flags_t flags = bio->bi_opf & REQ_NOWAIT ?
>> +		BLK_MQ_REQ_NOWAIT : 0;
>> +	struct request_queue *q = bio->bi_disk->queue;
>>  	blk_qc_t ret = BLK_QC_T_NONE;
>>  
>> +	if (blk_queue_enter(q, flags) < 0) {
> 
> As I mentioned last time, the queue pointer has to be checked before
> calling blk_queue_enter(), since it isn't difficult to trigger the
> failure log of 'generic_make_request: Trying to access nonexistent
> block-device'.

That's a good point, the NULL check needs to be included first. Bart,
do you want to send an update? I can just rebase the for-linus branch.

-- 
Jens Axboe

Re: [PATCH] blk-mq: Avoid that submitting a bio concurrently with device removal triggers a crash

[Bart Van Assche]

T24gVHVlLCAyMDE4LTA0LTEwIGF0IDIwOjEwIC0wNjAwLCBKZW5zIEF4Ym9lIHdyb3RlOg0KPiBP
biA0LzEwLzE4IDg6MDUgUE0sIE1pbmcgTGVpIHdyb3RlOg0KPiA+IE9uIFR1ZSwgQXByIDEwLCAy
MDE4IGF0IDAyOjMwOjM1UE0gLTA2MDAsIEJhcnQgVmFuIEFzc2NoZSB3cm90ZToNCj4gPiA+IEJl
Y2F1c2UgYmxrY2dfZXhpdF9xdWV1ZSgpIGlzIG5vdyBjYWxsZWQgZnJvbSBpbnNpZGUgYmxrX2Ns
ZWFudXBfcXVldWUoKQ0KPiA+ID4gaXQgaXMgbm8gbG9uZ2VyIHNhZmUgdG8gYWNjZXNzIGNncm91
cCBpbmZvcm1hdGlvbiBkdXJpbmcgb3IgYWZ0ZXIgdGhlDQo+ID4gPiBibGtfY2xlYW51cF9xdWV1
ZSgpIGNhbGwuIEhlbmNlIHByb3RlY3QgdGhlIGdlbmVyaWNfbWFrZV9yZXF1ZXN0X2NoZWNrcygp
DQo+ID4gPiBjYWxsIHdpdGggYmxrX3F1ZXVlX2VudGVyKCkgLyBibGtfcXVldWVfZXhpdCgpLg0K
PiA+ID4gDQo+ID4gPiBSZXBvcnRlZC1ieTogTWluZyBMZWkgPG1pbmcubGVpQHJlZGhhdC5jb20+
DQo+ID4gPiBGaXhlczogYTA2MzA1N2Q3YzczICgiYmxvY2s6IEZpeCBhIHJhY2UgYmV0d2VlbiBy
ZXF1ZXN0IHF1ZXVlIHJlbW92YWwgYW5kIHRoZSBibG9jayBjZ3JvdXAgY29udHJvbGxlciIpDQo+
ID4gPiBTaWduZWQtb2ZmLWJ5OiBCYXJ0IFZhbiBBc3NjaGUgPGJhcnQudmFuYXNzY2hlQHdkYy5j
b20+DQo+ID4gPiBDYzogTWluZyBMZWkgPG1pbmcubGVpQHJlZGhhdC5jb20+DQo+ID4gPiBDYzog
Sm9zZXBoIFFpIDxqb3NlcGgucWlAbGludXguYWxpYmFiYS5jb20+DQo+ID4gPiAtLS0NCj4gPiA+
ICBibG9jay9ibGstY29yZS5jIHwgMzIgKysrKysrKysrKysrKysrKysrKysrKysrKystLS0tLS0N
Cj4gPiA+ICAxIGZpbGUgY2hhbmdlZCwgMjYgaW5zZXJ0aW9ucygrKSwgNiBkZWxldGlvbnMoLSkN
Cj4gPiA+IA0KPiA+ID4gZGlmZiAtLWdpdCBhL2Jsb2NrL2Jsay1jb3JlLmMgYi9ibG9jay9ibGst
Y29yZS5jDQo+ID4gPiBpbmRleCAzNGUyZjIyMjdmZDkuLmEzMzBjZDI4MjllMSAxMDA2NDQNCj4g
PiA+IC0tLSBhL2Jsb2NrL2Jsay1jb3JlLmMNCj4gPiA+ICsrKyBiL2Jsb2NrL2Jsay1jb3JlLmMN
Cj4gPiA+IEBAIC0yMzg2LDggKzIzODYsMTkgQEAgYmxrX3FjX3QgZ2VuZXJpY19tYWtlX3JlcXVl
c3Qoc3RydWN0IGJpbyAqYmlvKQ0KPiA+ID4gIAkgKiB5ZXQuDQo+ID4gPiAgCSAqLw0KPiA+ID4g
IAlzdHJ1Y3QgYmlvX2xpc3QgYmlvX2xpc3Rfb25fc3RhY2tbMl07DQo+ID4gPiArCWJsa19tcV9y
ZXFfZmxhZ3NfdCBmbGFncyA9IGJpby0+Ymlfb3BmICYgUkVRX05PV0FJVCA/DQo+ID4gPiArCQlC
TEtfTVFfUkVRX05PV0FJVCA6IDA7DQo+ID4gPiArCXN0cnVjdCByZXF1ZXN0X3F1ZXVlICpxID0g
YmlvLT5iaV9kaXNrLT5xdWV1ZTsNCj4gPiA+ICAJYmxrX3FjX3QgcmV0ID0gQkxLX1FDX1RfTk9O
RTsNCj4gPiA+ICANCj4gPiA+ICsJaWYgKGJsa19xdWV1ZV9lbnRlcihxLCBmbGFncykgPCAwKSB7
DQo+ID4gDQo+ID4gQXMgSSBtZW50aW9uZWQgbGFzdCB0aW1lLCB0aGUgcXVldWUgcG9pbnRlciBo
YXMgdG8gYmUgY2hlY2tlZCBiZWZvcmUNCj4gPiBjYWxsaW5nIGJsa19xdWV1ZV9lbnRlcigpLCBz
aW5jZSBpdCBpc24ndCBkaWZmaWN1bHQgdG8gdHJpZ2dlciB0aGUNCj4gPiBmYWlsdXJlIGxvZyBv
ZiAnZ2VuZXJpY19tYWtlX3JlcXVlc3Q6IFRyeWluZyB0byBhY2Nlc3Mgbm9uZXhpc3RlbnQNCj4g
PiBibG9jay1kZXZpY2UnLg0KPiANCj4gVGhhdCdzIGEgZ29vZCBwb2ludCwgdGhlIE5VTEwgY2hl
Y2sgbmVlZHMgdG8gYmUgaW5jbHVkZWQgZmlyc3QuIEJhcnQsDQo+IGRvIHlvdSB3YW50IHRvIHNl
bmQgYW4gdXBkYXRlPyBJIGNhbiBqdXN0IHJlYmFzZSB0aGUgZm9yLWxpbnVzIGJyYW5jaC4NCg0K
QnV0IGhvdyBjb3VsZCB0aGUgcmVxdWVzdCBwb2ludGVyIGJlIE5VTEw/IFdoaWNoIGNvZGUgY2xl
YXJzIHRoYXQgcG9pbnRlcj8NCg0KVGhhbmtzLA0KDQpCYXJ0Lg0KDQoNCg0K