blk: fix dangling pointer access in __elv_add_request
PATCH 1/1 blk: fix dangling pointer access in __elv_add_request
cfq's add_req_fn callback may invoke q->request_fn directly and depending on low-level driver used and timing, a queued request may be finished & deallocated before add_req_fn callback returns. So, __elv_add_request must not access rq after it's passed to add_req_fn callback.
This patch moves rq_mergeable test above add_req_fn(). This may result in q->last_merge pointing to REQ_NOMERGE request if add_req_fn callback sets it but as RQ_NOMERGE is checked again when blk layer actually tries to merge requests, this does not cause any problem.
---
Arnaldo, I think this patch should fix the oops you're seeing. Please let me know how it works. And thanks again for detailed reporting.
Jens, does generalizing queue kicking functions and disallowing ioscheds from directly calling q->request_fn sound like a good idea?
Linus, with or without Arnaldo's confirmation, this patch fixes an existing bug. Please apply. Thanks.
drivers/block/elevator.c +6 -1
@@ -369,9 +369,14 @@ void __elv_add_request(request_queue_t *case ELEVATOR_INSERT_SORT :BUG_ON (!blk_fs_request (rq)); rq->flags |=REQ_SORTED ; - q->elevator->ops->elevator_add_req_fn (q, rq);if (q->last_merge ==NULL &&rq_mergeable (rq)) q->last_merge = rq; +/* +* Some ioscheds (cfq) run q->request_fn directly, so +* rq cannot be accessed after calling +* elevator_add_req_fn. +*/ + q->elevator->ops->elevator_add_req_fn (q, rq);break ;default :
Quoted 5 lines · Tejun Heo
cfq's
add_req_fncallback may invokeq->request_fndirectly and depending on low-level driver used and timing, a queued request may be finished & deallocated beforeadd_req_fncallback returns. So,__elv_add_requestmust not access rq after it's passed toadd_req_fncallback.
It's a generel problem, you may get the queue run at any time regardless of what the io scheduler is doing. CFQ does run the queue manully sometimes, but SCSI may do the very same thing for you as well. Given that SCSI also shortly reenables interrupts in the ->request_fn() handling, it's quite possible for the request to be completed.
So, as we don't hold a reference to the request, I'd say your patch looks correct and should be applied right away.
Quoted 2 lines
Jens, does generalizing queue kicking functions and disallowing ioscheds from directly calling
q->request_fnsound like a good idea?
Yes certainly.
Signature · 1 line
Jens Axboe
Quoted 19 lines · Jens Axboe
Quoted 5 lines · Tejun Heo
cfq's
add_req_fncallback may invokeq->request_fndirectly and depending on low-level driver used and timing, a queued request may be finished & deallocated beforeadd_req_fncallback returns. So,__elv_add_requestmust not access rq after it's passed toadd_req_fncallback.It's a generel problem, you may get the queue run at any time regardless of what the io scheduler is doing. CFQ does run the queue manully sometimes, but SCSI may do the very same thing for you as well. Given that SCSI also shortly reenables interrupts in the ->request_fn() handling, it's quite possible for the request to be completed.
So, as we don't hold a reference to the request, I'd say your patch looks correct and should be applied right away.
Quoted 6 lines
Quoted 2 lines
Jens, does generalizing queue kicking functions and disallowing ioscheds from directly calling
q->request_fnsound like a good idea?Yes certainly.
The thing is that we are holding queue_lock before calling add_req_fn callback and also after it finishes giving it an appearance of atomicity. I think q->request_fn semantics is peculiar and a bit prone to bug, so it might be better to make ioscheds always use generic queue kicking function which always uses work queue to run q->request_fn so that we don't have queue_lock releasing and regrabbing inbetween. Do you think there can be any noticieable performance issues?
Hmmmm... One more thing about q->request_fn's locking behavior is that, as I noted while posting the ordered patchset, for SCSI, the behavior can reorder issued requests making it impossible to use ordered tags for flushing. I'm thinking of submitting a patch to make scsi request_fn atomic w.r.t. queue_lock, but there might be some performance issues I'm not aware of. Functions which release and regrab locks underneath the caller are just... hard. :-p
Thanks.
Signature · 1 line
tejun
Quoted 27 lines · Tejun Heo
Quoted 19 lines · Jens Axboe
Quoted 5 lines · Tejun Heo
cfq's
add_req_fncallback may invokeq->request_fndirectly and depending on low-level driver used and timing, a queued request may be finished & deallocated beforeadd_req_fncallback returns. So,__elv_add_requestmust not access rq after it's passed toadd_req_fncallback.It's a generel problem, you may get the queue run at any time regardless of what the io scheduler is doing. CFQ does run the queue manully sometimes, but SCSI may do the very same thing for you as well. Given that SCSI also shortly reenables interrupts in the ->request_fn() handling, it's quite possible for the request to be completed.
So, as we don't hold a reference to the request, I'd say your patch looks correct and should be applied right away.
Quoted 6 lines
Quoted 2 lines
Jens, does generalizing queue kicking functions and disallowing ioscheds from directly calling
q->request_fnsound like a good idea?Yes certainly.
Quoted 7 lines
The thing is that we are holding
queue_lockbefore callingadd_req_fncallback and also after it finishes giving it an appearance of atomicity. I thinkq->request_fnsemantics is peculiar and a bit prone to bug, so it might be better to make ioscheds always use generic queue kicking function which always uses work queue to runq->request_fnso that we don't havequeue_lockreleasing and regrabbing inbetween. Do you think there can be any noticieable performance issues?
Hmm well, I'd rather not push the work off to a workqueue unless I have to. That's basically why I coded it myself so far, since I (usually :-) know when it's safe to do it in the various ways. 'as' basically always pushes off the work to kblockd, but I'd rather not lose this optimization.
Quoted 7 lines
Hmmmm... One more thing about q->request_fn's locking behavior is that, as I noted while posting the ordered patchset, for SCSI, the behavior can reorder issued requests making it impossible to use ordered tags for flushing. I'm thinking of submitting a patch to make scsi
request_fnatomic w.r.t.queue_lock, but there might be some performance issues I'm not aware of. Functions which release and regrab locks underneath the caller are just... hard. :-p
Yeah it's problematic and not exactly the best design. It also tends to screw up cfq/as on requeues, if a requests get requeued and isn't the first returned again.
WRT performance penalty, really hard to guess without looking at the patch :-)
Signature · 1 line
Jens Axboe