| Message ID | 20180410203035.26167-1-bart.vanassche@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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'.
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.
On Tue, 2018-04-10 at 20:10 -0600, Jens Axboe wrote: > 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. But how could the request pointer be NULL? Which code clears that pointer? Thanks, Bart.
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);
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(-)