[PATCH] blk: fix dangling pointer access in __elv_add_request

[PATCH] blk: fix dangling pointer access in __elv_add_request

[Tejun Heo]

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.

Signed-off-by: Tejun Heo <htejun@gmail.com>
---

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.

diff --git a/drivers/block/elevator.c b/drivers/block/elevator.c
--- a/drivers/block/elevator.c
+++ b/drivers/block/elevator.c
@@ -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:

Re: [PATCH] blk: fix dangling pointer access in __elv_add_request

[Jens Axboe]

On Tue, Nov 01 2005, Tejun Heo wrote:
> 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.

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.

> Jens, does generalizing queue kicking functions and disallowing
> ioscheds from directly calling q->request_fn sound like a good idea?

Yes certainly.

-- 
Jens Axboe

Re: [PATCH] blk: fix dangling pointer access in __elv_add_request

[Tejun Heo]

Jens Axboe wrote:
> On Tue, Nov 01 2005, Tejun Heo wrote:
> 
>>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.
> 
> 
> 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.
 >
 >
 >>Jens, does generalizing queue kicking functions and disallowing
 >>ioscheds from directly calling q->request_fn sound 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.

-- 
tejun

Re: [PATCH] blk: fix dangling pointer access in __elv_add_request

[Jens Axboe]

On Tue, Nov 01 2005, Tejun Heo wrote:
> Jens Axboe wrote:
> >On Tue, Nov 01 2005, Tejun Heo wrote:
> >
> >>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.
> >
> >
> >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.
> >
> >
> >>Jens, does generalizing queue kicking functions and disallowing
> >>ioscheds from directly calling q->request_fn sound 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?

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.

> 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

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 :-)

-- 
Jens Axboe