On 3/3/20 8:08 AM, Yufen Yu wrote:
Our test robot reported a warning for refcount_dec trying to decrease
value '0'. The reason is that blk_mq_dispatch_rq_list() try to complete
the failed request from nbd driver, while the request have finished in
nbd timeout handle function. The race as following:
CPU1 CPU2
//req->ref = 1
blk_mq_dispatch_rq_list
nbd_queue_rq
nbd_handle_cmd
blk_mq_start_request
blk_mq_check_expired
//req->ref = 2
blk_mq_rq_timed_out
nbd_xmit_timeout
blk_mq_complete_request
//req->ref = 1
refcount_dec_and_test(&req->ref)
refcount_dec_and_test(&req->ref)
//req->ref = 0
__blk_mq_free_request(req)
ret = BLK_STS_IOERR
blk_mq_end_request
// req->ref = 0, req have been free
refcount_dec_and_test(&rq->ref)
In fact, the bug also have been reported by syzbot:
https://lkml.org/lkml/2018/12/5/1308
Since the request have been freed by timeout handle, it can be reused
by others. Then, blk_mq_end_request() may get the re-initialized request
and free it, which is unexpected.
To fix the problem, we move blk_mq_start_request() down until the driver
will handle the request actully. If .queue_rq return something error in
preparation phase, timeout handle may don't need. Thus, moving start
request down may be more reasonable. Then, nbd_queue_rq() will not return
BLK_STS_IOERR after starting request.
This won't work, you have to have the request started if you return an error
because of this in blk_mq_dispatch_rq_list
if (unlikely(ret != BLK_STS_OK)) {
errors++;
blk_mq_end_request(rq, BLK_STS_IOERR);
continue;
}
The request has to be started before we return an error, pushing it down means
we have all of these error cases where we haven't started the request. Thanks,
Josef
