[PATCH] block: fix oops with block tag queueing

[PATCH] block: fix oops with block tag queueing

[James Bottomley]

commit e8939a50466fd963eb1ba9118c34b9ffb7ff6aa6
Author: Tejun Heo <tj@kernel.org>
Date:   Fri May 8 11:54:16 2009 +0900

    block: implement and enforce request peek/start/fetch

Added a BUG_ON(blk_queued_rq(req)) to the top of blk_finish_req().
Unfortunately, this checks whether req->queuelist is empty.  This list
is doing double duty both as the queue list and the tag list, so tagged
requests come in here with this not empty and boom (the tag list is
emptied by blk_queue_end_tag() lower down).

Fix this by moving the BUG_ON to below the end tag we also seem
vulnerable to this in blk_requeue_request() as well.  I think all uses
of blk_queued_rq() need auditing because the check is clearly wrong in
the tagged case.

Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>

---
You might like to combine this with the above commit.  Otherwise we'll
have a bisect panic with adaptec and other drivers in the interval
between the two commits.

diff --git a/block/blk-core.c b/block/blk-core.c
index 05c2520..3ea7242 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -956,8 +956,6 @@ EXPORT_SYMBOL(blk_make_request);
  */
 void blk_requeue_request(struct request_queue *q, struct request *rq)
 {
-	BUG_ON(blk_queued_rq(rq));
-
 	blk_delete_timer(rq);
 	blk_clear_rq_complete(rq);
 	trace_block_rq_requeue(q, rq);
@@ -965,6 +963,8 @@ void blk_requeue_request(struct request_queue *q, struct request *rq)
 	if (blk_rq_tagged(rq))
 		blk_queue_end_tag(q, rq);
 
+	BUG_ON(blk_queued_rq(rq));
+
 	elv_requeue_request(q, rq);
 }
 EXPORT_SYMBOL(blk_requeue_request);
@@ -2042,11 +2042,11 @@ static bool blk_update_bidi_request(struct request *rq, int error,
  */
 static void blk_finish_request(struct request *req, int error)
 {
-	BUG_ON(blk_queued_rq(req));
-
 	if (blk_rq_tagged(req))
 		blk_queue_end_tag(req->q, req);
 
+	BUG_ON(blk_queued_rq(req));
+
 	if (unlikely(laptop_mode) && blk_fs_request(req))
 		laptop_io_completion();
 

Re: [PATCH] block: fix oops with block tag queueing

[Tejun Heo]

James Bottomley wrote:
> commit e8939a50466fd963eb1ba9118c34b9ffb7ff6aa6
> Author: Tejun Heo <tj@kernel.org>
> Date:   Fri May 8 11:54:16 2009 +0900
> 
>     block: implement and enforce request peek/start/fetch
> 
> Added a BUG_ON(blk_queued_rq(req)) to the top of blk_finish_req().
> Unfortunately, this checks whether req->queuelist is empty.  This list
> is doing double duty both as the queue list and the tag list, so tagged
> requests come in here with this not empty and boom (the tag list is
> emptied by blk_queue_end_tag() lower down).
> 
> Fix this by moving the BUG_ON to below the end tag we also seem
> vulnerable to this in blk_requeue_request() as well.  I think all uses
> of blk_queued_rq() need auditing because the check is clearly wrong in
> the tagged case.
> 
> Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>

Oops,

Acked-by: Tejun Heo <tj@kernel.org>

There are also some drivers which use queuelist for internal purposes
after dequeueing, which also screws up blk_queued_rq() test in
addition to being questionable practice to begin with.  Maybe we would
be better off with a flag?

Thanks.

-- 
tejun

Re: [PATCH] block: fix oops with block tag queueing

[Boaz Harrosh]

On 05/21/2009 04:55 AM, Tejun Heo wrote:
> James Bottomley wrote:
>> commit e8939a50466fd963eb1ba9118c34b9ffb7ff6aa6
>> Author: Tejun Heo <tj@kernel.org>
>> Date:   Fri May 8 11:54:16 2009 +0900
>>
>>     block: implement and enforce request peek/start/fetch
>>
>> Added a BUG_ON(blk_queued_rq(req)) to the top of blk_finish_req().
>> Unfortunately, this checks whether req->queuelist is empty.  This list
>> is doing double duty both as the queue list and the tag list, so tagged
>> requests come in here with this not empty and boom (the tag list is
>> emptied by blk_queue_end_tag() lower down).
>>
>> Fix this by moving the BUG_ON to below the end tag we also seem
>> vulnerable to this in blk_requeue_request() as well.  I think all uses
>> of blk_queued_rq() need auditing because the check is clearly wrong in
>> the tagged case.
>>
>> Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> 
> Oops,
> 
> Acked-by: Tejun Heo <tj@kernel.org>
> 
> There are also some drivers which use queuelist for internal purposes
> after dequeueing, which also screws up blk_queued_rq() test in
> addition to being questionable practice to begin with.  Maybe we would
> be better off with a flag?
> 

What is the REQ_STARTED flag for?

> Thanks.
> 

Boaz

Re: [PATCH] block: fix oops with block tag queueing

[Tejun Heo]

Boaz Harrosh wrote:
> On 05/21/2009 04:55 AM, Tejun Heo wrote:
>> James Bottomley wrote:
>> There are also some drivers which use queuelist for internal purposes
>> after dequeueing, which also screws up blk_queued_rq() test in
>> addition to being questionable practice to begin with.  Maybe we would
>> be better off with a flag?
>>
> 
> What is the REQ_STARTED flag for?

That is used to pin a request which has been peeked by low level
driver at the top of the queue so that the next peek or fetch returns
the same request.  I'm not sure whether that is necessary at this
point tho.

Thanks.

-- 
tejun

Re: [PATCH] block: fix oops with block tag queueing

[James Bottomley]

On Thu, 2009-05-21 at 10:55 +0900, Tejun Heo wrote:
> James Bottomley wrote:
> > commit e8939a50466fd963eb1ba9118c34b9ffb7ff6aa6
> > Author: Tejun Heo <tj@kernel.org>
> > Date:   Fri May 8 11:54:16 2009 +0900
> > 
> >     block: implement and enforce request peek/start/fetch
> > 
> > Added a BUG_ON(blk_queued_rq(req)) to the top of blk_finish_req().
> > Unfortunately, this checks whether req->queuelist is empty.  This list
> > is doing double duty both as the queue list and the tag list, so tagged
> > requests come in here with this not empty and boom (the tag list is
> > emptied by blk_queue_end_tag() lower down).
> > 
> > Fix this by moving the BUG_ON to below the end tag we also seem
> > vulnerable to this in blk_requeue_request() as well.  I think all uses
> > of blk_queued_rq() need auditing because the check is clearly wrong in
> > the tagged case.
> > 
> > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> 
> Oops,
> 
> Acked-by: Tejun Heo <tj@kernel.org>
> 
> There are also some drivers which use queuelist for internal purposes
> after dequeueing, which also screws up blk_queued_rq() test in
> addition to being questionable practice to begin with.  Maybe we would
> be better off with a flag?

Either is fine by me ... could we get some fix in, please?  I'm
currently carrying this below the merge-base on the SCSI postmerge tree
to prevent my main build server oopsing under SCSI testing ... I'm a bit
surprised we haven't had more reports from linux-oops ... but you can
bet that if Jens moves libata to generic tag use, that will change ...

James

Re: [PATCH] block: fix oops with block tag queueing

[Tejun Heo]

James Bottomley wrote:
> On Thu, 2009-05-21 at 10:55 +0900, Tejun Heo wrote:
>> James Bottomley wrote:
>>> commit e8939a50466fd963eb1ba9118c34b9ffb7ff6aa6
>>> Author: Tejun Heo <tj@kernel.org>
>>> Date:   Fri May 8 11:54:16 2009 +0900
>>>
>>>     block: implement and enforce request peek/start/fetch
>>>
>>> Added a BUG_ON(blk_queued_rq(req)) to the top of blk_finish_req().
>>> Unfortunately, this checks whether req->queuelist is empty.  This list
>>> is doing double duty both as the queue list and the tag list, so tagged
>>> requests come in here with this not empty and boom (the tag list is
>>> emptied by blk_queue_end_tag() lower down).
>>>
>>> Fix this by moving the BUG_ON to below the end tag we also seem
>>> vulnerable to this in blk_requeue_request() as well.  I think all uses
>>> of blk_queued_rq() need auditing because the check is clearly wrong in
>>> the tagged case.
>>>
>>> Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
>> Oops,
>>
>> Acked-by: Tejun Heo <tj@kernel.org>
>>
>> There are also some drivers which use queuelist for internal purposes
>> after dequeueing, which also screws up blk_queued_rq() test in
>> addition to being questionable practice to begin with.  Maybe we would
>> be better off with a flag?
> 
> Either is fine by me ... could we get some fix in, please?  I'm
> currently carrying this below the merge-base on the SCSI postmerge tree
> to prevent my main build server oopsing under SCSI testing ... I'm a bit
> surprised we haven't had more reports from linux-oops ... but you can
> bet that if Jens moves libata to generic tag use, that will change ...

For now, I think your patch is fine and thus the Acked-by.  Jens?

Thanks.

-- 
tejun

Re: [PATCH] block: fix oops with block tag queueing

[Jens Axboe]

On Wed, May 27 2009, Tejun Heo wrote:
> James Bottomley wrote:
> > On Thu, 2009-05-21 at 10:55 +0900, Tejun Heo wrote:
> >> James Bottomley wrote:
> >>> commit e8939a50466fd963eb1ba9118c34b9ffb7ff6aa6
> >>> Author: Tejun Heo <tj@kernel.org>
> >>> Date:   Fri May 8 11:54:16 2009 +0900
> >>>
> >>>     block: implement and enforce request peek/start/fetch
> >>>
> >>> Added a BUG_ON(blk_queued_rq(req)) to the top of blk_finish_req().
> >>> Unfortunately, this checks whether req->queuelist is empty.  This list
> >>> is doing double duty both as the queue list and the tag list, so tagged
> >>> requests come in here with this not empty and boom (the tag list is
> >>> emptied by blk_queue_end_tag() lower down).
> >>>
> >>> Fix this by moving the BUG_ON to below the end tag we also seem
> >>> vulnerable to this in blk_requeue_request() as well.  I think all uses
> >>> of blk_queued_rq() need auditing because the check is clearly wrong in
> >>> the tagged case.
> >>>
> >>> Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> >> Oops,
> >>
> >> Acked-by: Tejun Heo <tj@kernel.org>
> >>
> >> There are also some drivers which use queuelist for internal purposes
> >> after dequeueing, which also screws up blk_queued_rq() test in
> >> addition to being questionable practice to begin with.  Maybe we would
> >> be better off with a flag?
> > 
> > Either is fine by me ... could we get some fix in, please?  I'm
> > currently carrying this below the merge-base on the SCSI postmerge tree
> > to prevent my main build server oopsing under SCSI testing ... I'm a bit
> > surprised we haven't had more reports from linux-oops ... but you can
> > bet that if Jens moves libata to generic tag use, that will change ...
> 
> For now, I think your patch is fine and thus the Acked-by.  Jens?

Yes indeed, sorry for the delay. I'll commit it.

-- 
Jens Axboe