Skip to content

Commit 2d8fcff

Browse files
Yufen YuSomasundaram Krishnasamy
authored andcommitted
block: fix null pointer dereference in blk_mq_rq_timed_out()
We got a null pointer deference BUG_ON in blk_mq_rq_timed_out() as following: [ 108.825472] BUG: kernel NULL pointer dereference, address: 0000000000000040 [ 108.827059] PGD 0 P4D 0 [ 108.827313] Oops: 0000 [#1] SMP PTI [ 108.827657] CPU: 6 PID: 198 Comm: kworker/6:1H Not tainted 5.3.0-rc8+ #431 [ 108.829503] Workqueue: kblockd blk_mq_timeout_work [ 108.829913] RIP: 0010:blk_mq_check_expired+0x258/0x330 [ 108.838191] Call Trace: [ 108.838406] bt_iter+0x74/0x80 [ 108.838665] blk_mq_queue_tag_busy_iter+0x204/0x450 [ 108.839074] ? __switch_to_asm+0x34/0x70 [ 108.839405] ? blk_mq_stop_hw_queue+0x40/0x40 [ 108.839823] ? blk_mq_stop_hw_queue+0x40/0x40 [ 108.840273] ? syscall_return_via_sysret+0xf/0x7f [ 108.840732] blk_mq_timeout_work+0x74/0x200 [ 108.841151] process_one_work+0x297/0x680 [ 108.841550] worker_thread+0x29c/0x6f0 [ 108.841926] ? rescuer_thread+0x580/0x580 [ 108.842344] kthread+0x16a/0x1a0 [ 108.842666] ? kthread_flush_work+0x170/0x170 [ 108.843100] ret_from_fork+0x35/0x40 The bug is caused by the race between timeout handle and completion for flush request. When timeout handle function blk_mq_rq_timed_out() try to read 'req->q->mq_ops', the 'req' have completed and reinitiated by next flush request, which would call blk_rq_init() to clear 'req' as 0. After commit 12f5b93 ("blk-mq: Remove generation seqeunce"), normal requests lifetime are protected by refcount. Until 'rq->ref' drop to zero, the request can really be free. Thus, these requests cannot been reused before timeout handle finish. However, flush request has defined .end_io and rq->end_io() is still called even if 'rq->ref' doesn't drop to zero. After that, the 'flush_rq' can be reused by the next flush request handle, resulting in null pointer deference BUG ON. We fix this problem by covering flush request with 'rq->ref'. If the refcount is not zero, flush_end_io() return and wait the last holder recall it. To record the request status, we add a new entry 'rq_status', which will be used in flush_end_io(). Orabug: 30671963 Cc: Christoph Hellwig <[email protected]> Cc: Keith Busch <[email protected]> Cc: Bart Van Assche <[email protected]> Cc: [email protected] # v4.18+ Reviewed-by: Ming Lei <[email protected]> Reviewed-by: Bob Liu <[email protected]> Signed-off-by: Yufen Yu <[email protected]> ------- v2: - move rq_status from struct request to struct blk_flush_queue v3: - remove unnecessary '{}' pair. v4: - let spinlock to protect 'fq->rq_status' v5: - move rq_status after flush_running_idx member of struct blk_flush_queue Signed-off-by: Jens Axboe <[email protected]> (cherry picked from commit 8d69966) Signed-off-by: Ritika Srivastava <[email protected]> Reviewed-by: Junxiao Bi <[email protected]> Conflicts: block/blk-flush.c block/blk-mq.c Signed-off-by: Somasundaram Krishnasamy <[email protected]>
1 parent b3ad8c0 commit 2d8fcff

File tree

3 files changed

+22
-2
lines changed

3 files changed

+22
-2
lines changed

block/blk-flush.c

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -225,11 +225,22 @@ static void flush_end_io(struct request *flush_rq, blk_status_t error)
225225
unsigned long flags = 0;
226226
struct blk_flush_queue *fq = blk_get_flush_queue(q, flush_rq->mq_ctx);
227227

228+
if (q->mq_ops)
229+
spin_lock_irqsave(&fq->mq_flush_lock, flags);
230+
231+
if (!refcount_dec_and_test(&flush_rq->ref)) {
232+
fq->rq_status = error;
233+
if (q->mq_ops)
234+
spin_unlock_irqrestore(&fq->mq_flush_lock, flags);
235+
return;
236+
}
237+
238+
if (fq->rq_status != BLK_STS_OK)
239+
error = fq->rq_status;
228240
if (q->mq_ops) {
229241
struct blk_mq_hw_ctx *hctx;
230242

231243
/* release the tag's ownership to the req cloned from */
232-
spin_lock_irqsave(&fq->mq_flush_lock, flags);
233244
hctx = blk_mq_map_queue(q, flush_rq->mq_ctx->cpu);
234245
if (!q->elevator) {
235246
blk_mq_tag_set_rq(hctx, flush_rq->tag, fq->orig_rq);

block/blk-mq.c

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -836,7 +836,9 @@ static void blk_mq_check_expired(struct blk_mq_hw_ctx *hctx,
836836
return;
837837
if (!blk_mark_rq_complete(rq))
838838
blk_mq_rq_timed_out(rq, reserved);
839-
if (refcount_dec_and_test(&rq->ref))
839+
if (is_flush_rq(rq, hctx))
840+
rq->end_io(rq, 0);
841+
else if (refcount_dec_and_test(&rq->ref))
840842
__blk_mq_free_request(rq);
841843
} else if (!data->next_set || time_after(data->next, rq->deadline)) {
842844
data->next = rq->deadline;

block/blk.h

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ struct blk_flush_queue {
2323
unsigned int flush_queue_delayed:1;
2424
unsigned int flush_pending_idx:1;
2525
unsigned int flush_running_idx:1;
26+
UEK_KABI_FILL_HOLE(blk_status_t rq_status)
2627
unsigned long flush_pending_since;
2728
struct list_head flush_queue[2];
2829
struct list_head flush_data_in_flight;
@@ -54,6 +55,12 @@ static inline void __blk_get_queue(struct request_queue *q)
5455
kobject_get(&q->kobj);
5556
}
5657

58+
static inline bool
59+
is_flush_rq(struct request *req, struct blk_mq_hw_ctx *hctx)
60+
{
61+
return hctx->fq->flush_rq == req;
62+
}
63+
5764
struct blk_flush_queue *blk_alloc_flush_queue(struct request_queue *q,
5865
int node, int cmd_size);
5966
void blk_free_flush_queue(struct blk_flush_queue *q);

0 commit comments

Comments
 (0)