* [PATCH v2 0/2] fix request uaf in nbd_read_stat() @ 2021-08-09 3:09 Yu Kuai 2021-08-09 3:09 ` [PATCH v2 1/2] blk-mq: add a new interface to get request by tag Yu Kuai 2021-08-09 3:09 ` [PATCH v2 2/2] nbd: convert to use blk_mq_get_rq_by_tag() Yu Kuai 0 siblings, 2 replies; 8+ messages in thread From: Yu Kuai @ 2021-08-09 3:09 UTC (permalink / raw) To: axboe, josef, bvanassche, ming.lei Cc: linux-block, linux-kernel, nbd, yukuai3, yi.zhang This patchset fix request uaf in nbd, other drivers might have the same problem. Will fix them later if this patchset is accepted. Changes in v2: - as Bart suggested, add a new helper function for drivers to get request by tag. Yu Kuai (2): blk-mq: add a new interface to get request by tag nbd: convert to use blk_mq_get_rq_by_tag() block/blk-mq-tag.c | 33 +++++++++++++++++++++++++++++++++ block/blk-mq.c | 1 + block/blk-mq.h | 1 - drivers/block/nbd.c | 11 ++++++----- include/linux/blk-mq.h | 3 +++ 5 files changed, 43 insertions(+), 6 deletions(-) -- 2.31.1 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 1/2] blk-mq: add a new interface to get request by tag 2021-08-09 3:09 [PATCH v2 0/2] fix request uaf in nbd_read_stat() Yu Kuai @ 2021-08-09 3:09 ` Yu Kuai 2021-08-09 3:09 ` [PATCH v2 2/2] nbd: convert to use blk_mq_get_rq_by_tag() Yu Kuai 1 sibling, 0 replies; 8+ messages in thread From: Yu Kuai @ 2021-08-09 3:09 UTC (permalink / raw) To: axboe, josef, bvanassche, ming.lei Cc: linux-block, linux-kernel, nbd, yukuai3, yi.zhang Ming Lei had fixed the request uaf while iterating tags in commit bd63141d585b ("blk-mq: clear stale request in tags->rq[] before freeing one request pool"). However, some drivers are still using blk_mq_tag_to_rq(), which access request by tag directly. Thus add a new interface for such drivers to avoid request uaf. Signed-off-by: Yu Kuai <yukuai3@huawei.com> --- block/blk-mq-tag.c | 33 +++++++++++++++++++++++++++++++++ block/blk-mq.c | 1 + block/blk-mq.h | 1 - include/linux/blk-mq.h | 3 +++ 4 files changed, 37 insertions(+), 1 deletion(-) diff --git a/block/blk-mq-tag.c b/block/blk-mq-tag.c index 86f87346232a..7921bd2019df 100644 --- a/block/blk-mq-tag.c +++ b/block/blk-mq-tag.c @@ -652,3 +652,36 @@ u32 blk_mq_unique_tag(struct request *rq) (rq->tag & BLK_MQ_UNIQUE_TAG_MASK); } EXPORT_SYMBOL(blk_mq_unique_tag); + +/** + * blk_mq_get_rq_by_tag - if the request that is represented by the tag is + * not idle, increment it's reference and then return it. Otherwise return + * NULL. + */ +struct request *blk_mq_get_rq_by_tag(struct blk_mq_tags *tags, + unsigned int tag) +{ + unsigned long flags; + struct request *rq; + + /* hold lock to prevent accessing freed request by tag */ + spin_lock_irqsave(&tags->lock, flags); + rq = blk_mq_tag_to_rq(tags, tag); + if (!rq) + goto out_unlock; + + if (!refcount_inc_not_zero(&rq->ref)) { + rq = NULL; + goto out_unlock; + } + + if (!blk_mq_request_started(rq)) { + blk_mq_put_rq_ref(rq); + rq = NULL; + } + +out_unlock: + spin_unlock_irqrestore(&tags->lock, flags); + return rq; +} +EXPORT_SYMBOL(blk_mq_get_rq_by_tag); diff --git a/block/blk-mq.c b/block/blk-mq.c index 495f508c6300..7d0bd3809336 100644 --- a/block/blk-mq.c +++ b/block/blk-mq.c @@ -916,6 +916,7 @@ void blk_mq_put_rq_ref(struct request *rq) else if (refcount_dec_and_test(&rq->ref)) __blk_mq_free_request(rq); } +EXPORT_SYMBOL_GPL(blk_mq_put_rq_ref); static bool blk_mq_check_expired(struct blk_mq_hw_ctx *hctx, struct request *rq, void *priv, bool reserved) diff --git a/block/blk-mq.h b/block/blk-mq.h index d08779f77a26..20ef743a3ff6 100644 --- a/block/blk-mq.h +++ b/block/blk-mq.h @@ -47,7 +47,6 @@ void blk_mq_add_to_requeue_list(struct request *rq, bool at_head, void blk_mq_flush_busy_ctxs(struct blk_mq_hw_ctx *hctx, struct list_head *list); struct request *blk_mq_dequeue_from_ctx(struct blk_mq_hw_ctx *hctx, struct blk_mq_ctx *start); -void blk_mq_put_rq_ref(struct request *rq); /* * Internal helpers for allocating/freeing the request map diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h index 1d18447ebebc..d460e5e2c07a 100644 --- a/include/linux/blk-mq.h +++ b/include/linux/blk-mq.h @@ -635,4 +635,7 @@ blk_qc_t blk_mq_submit_bio(struct bio *bio); void blk_mq_hctx_set_fq_lock_class(struct blk_mq_hw_ctx *hctx, struct lock_class_key *key); +struct request *blk_mq_get_rq_by_tag(struct blk_mq_tags *tags, + unsigned int tag); +void blk_mq_put_rq_ref(struct request *rq); #endif -- 2.31.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v2 2/2] nbd: convert to use blk_mq_get_rq_by_tag() 2021-08-09 3:09 [PATCH v2 0/2] fix request uaf in nbd_read_stat() Yu Kuai 2021-08-09 3:09 ` [PATCH v2 1/2] blk-mq: add a new interface to get request by tag Yu Kuai @ 2021-08-09 3:09 ` Yu Kuai 2021-08-09 6:28 ` Ming Lei 1 sibling, 1 reply; 8+ messages in thread From: Yu Kuai @ 2021-08-09 3:09 UTC (permalink / raw) To: axboe, josef, bvanassche, ming.lei Cc: linux-block, linux-kernel, nbd, yukuai3, yi.zhang blk_mq_tag_to_rq() might return freed request, use blk_mq_get_rq_by_tag() instead. Signed-off-by: Yu Kuai <yukuai3@huawei.com> --- drivers/block/nbd.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c index c38317979f74..9e56975a8eee 100644 --- a/drivers/block/nbd.c +++ b/drivers/block/nbd.c @@ -713,11 +713,10 @@ static struct nbd_cmd *nbd_read_stat(struct nbd_device *nbd, int index) tag = nbd_handle_to_tag(handle); hwq = blk_mq_unique_tag_to_hwq(tag); if (hwq < nbd->tag_set.nr_hw_queues) - req = blk_mq_tag_to_rq(nbd->tag_set.tags[hwq], - blk_mq_unique_tag_to_tag(tag)); - if (!req || !blk_mq_request_started(req)) { - dev_err(disk_to_dev(nbd->disk), "Unexpected reply (%d) %p\n", - tag, req); + req = blk_mq_get_rq_by_tag(nbd->tag_set.tags[hwq], + blk_mq_unique_tag_to_tag(tag)); + if (!req) { + dev_err(disk_to_dev(nbd->disk), "Unexpected reply %d\n", tag); return ERR_PTR(-ENOENT); } trace_nbd_header_received(req, handle); @@ -779,6 +778,8 @@ static struct nbd_cmd *nbd_read_stat(struct nbd_device *nbd, int index) } out: trace_nbd_payload_received(req, handle); + if (req) + blk_mq_put_rq_ref(req); mutex_unlock(&cmd->lock); return ret ? ERR_PTR(ret) : cmd; } -- 2.31.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] nbd: convert to use blk_mq_get_rq_by_tag() 2021-08-09 3:09 ` [PATCH v2 2/2] nbd: convert to use blk_mq_get_rq_by_tag() Yu Kuai @ 2021-08-09 6:28 ` Ming Lei 2021-08-09 7:08 ` yukuai (C) 0 siblings, 1 reply; 8+ messages in thread From: Ming Lei @ 2021-08-09 6:28 UTC (permalink / raw) To: Yu Kuai Cc: axboe, josef, bvanassche, linux-block, linux-kernel, nbd, yi.zhang On Mon, Aug 09, 2021 at 11:09:27AM +0800, Yu Kuai wrote: > blk_mq_tag_to_rq() might return freed request, use > blk_mq_get_rq_by_tag() instead. > > Signed-off-by: Yu Kuai <yukuai3@huawei.com> > --- > drivers/block/nbd.c | 11 ++++++----- > 1 file changed, 6 insertions(+), 5 deletions(-) > > diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c > index c38317979f74..9e56975a8eee 100644 > --- a/drivers/block/nbd.c > +++ b/drivers/block/nbd.c > @@ -713,11 +713,10 @@ static struct nbd_cmd *nbd_read_stat(struct nbd_device *nbd, int index) > tag = nbd_handle_to_tag(handle); > hwq = blk_mq_unique_tag_to_hwq(tag); > if (hwq < nbd->tag_set.nr_hw_queues) > - req = blk_mq_tag_to_rq(nbd->tag_set.tags[hwq], > - blk_mq_unique_tag_to_tag(tag)); > - if (!req || !blk_mq_request_started(req)) { > - dev_err(disk_to_dev(nbd->disk), "Unexpected reply (%d) %p\n", > - tag, req); > + req = blk_mq_get_rq_by_tag(nbd->tag_set.tags[hwq], > + blk_mq_unique_tag_to_tag(tag)); > + if (!req) { > + dev_err(disk_to_dev(nbd->disk), "Unexpected reply %d\n", tag); > return ERR_PTR(-ENOENT); > } > trace_nbd_header_received(req, handle); > @@ -779,6 +778,8 @@ static struct nbd_cmd *nbd_read_stat(struct nbd_device *nbd, int index) > } > out: > trace_nbd_payload_received(req, handle); > + if (req) > + blk_mq_put_rq_ref(req); > mutex_unlock(&cmd->lock); > return ret ? ERR_PTR(ret) : cmd; After blk_mq_put_rq_ref() returns, this request may have been freed, so the returned 'cmd' may have been freed too. As I replied in your another thread, it is driver's responsibility to cover race between normal completion and timeout/error handling, that means the caller of blk_mq_tag_to_rq need to make sure that the request represented by the passed 'tag' can't be freed. I'd suggest to understand why nbd_read_stat()/blk_mq_tag_to_rq() may return one freed request first, who frees the request and how when calling blk_mq_tag_to_rq() before figuring out solutions. Thanks, Ming ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] nbd: convert to use blk_mq_get_rq_by_tag() 2021-08-09 6:28 ` Ming Lei @ 2021-08-09 7:08 ` yukuai (C) 2021-08-09 9:46 ` Ming Lei 0 siblings, 1 reply; 8+ messages in thread From: yukuai (C) @ 2021-08-09 7:08 UTC (permalink / raw) To: Ming Lei Cc: axboe, josef, bvanassche, linux-block, linux-kernel, nbd, yi.zhang On 2021/08/09 14:28, Ming Lei wrote: > On Mon, Aug 09, 2021 at 11:09:27AM +0800, Yu Kuai wrote: >> blk_mq_tag_to_rq() might return freed request, use >> blk_mq_get_rq_by_tag() instead. >> >> Signed-off-by: Yu Kuai <yukuai3@huawei.com> >> --- >> drivers/block/nbd.c | 11 ++++++----- >> 1 file changed, 6 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c >> index c38317979f74..9e56975a8eee 100644 >> --- a/drivers/block/nbd.c >> +++ b/drivers/block/nbd.c >> @@ -713,11 +713,10 @@ static struct nbd_cmd *nbd_read_stat(struct nbd_device *nbd, int index) >> tag = nbd_handle_to_tag(handle); >> hwq = blk_mq_unique_tag_to_hwq(tag); >> if (hwq < nbd->tag_set.nr_hw_queues) >> - req = blk_mq_tag_to_rq(nbd->tag_set.tags[hwq], >> - blk_mq_unique_tag_to_tag(tag)); >> - if (!req || !blk_mq_request_started(req)) { >> - dev_err(disk_to_dev(nbd->disk), "Unexpected reply (%d) %p\n", >> - tag, req); >> + req = blk_mq_get_rq_by_tag(nbd->tag_set.tags[hwq], >> + blk_mq_unique_tag_to_tag(tag)); >> + if (!req) { >> + dev_err(disk_to_dev(nbd->disk), "Unexpected reply %d\n", tag); >> return ERR_PTR(-ENOENT); >> } >> trace_nbd_header_received(req, handle); >> @@ -779,6 +778,8 @@ static struct nbd_cmd *nbd_read_stat(struct nbd_device *nbd, int index) >> } >> out: >> trace_nbd_payload_received(req, handle); >> + if (req) >> + blk_mq_put_rq_ref(req); >> mutex_unlock(&cmd->lock); >> return ret ? ERR_PTR(ret) : cmd; > > After blk_mq_put_rq_ref() returns, this request may have been freed, > so the returned 'cmd' may have been freed too. > > As I replied in your another thread, it is driver's responsibility to > cover race between normal completion and timeout/error handling, that > means the caller of blk_mq_tag_to_rq need to make sure that the request > represented by the passed 'tag' can't be freed. Hi, Ming There are two problems here in nbd, both reported by our syzkaller. The first is that blk_mq_tag_to_rq() returned a freed request, which is because tags->static_rq[] is freed without clearing tags->rq[]. Syzkaller log shows that a reply package is sent to client without the client's request package. And this patch is trying to solve this problem. The second is that flush_end_io() decrement it's refcount to -1. I guess this is because nbd_clear_que concurrent with normal completion / timeout / error handling, and somehow trigger the problem. However I'm still trying to understand the logic in nbd. I tried to add a check in flush_end_io() to fix this, as you replied, it's driver's responsibility to fix the problem. It seems that the two problems are not related. And this patch is ok to fix the first problem. Any suggestions? Thanks Kuai > > I'd suggest to understand why nbd_read_stat()/blk_mq_tag_to_rq() may return > one freed request first, who frees the request and how when calling > blk_mq_tag_to_rq() before figuring out solutions. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] nbd: convert to use blk_mq_get_rq_by_tag() 2021-08-09 7:08 ` yukuai (C) @ 2021-08-09 9:46 ` Ming Lei 2021-08-09 14:04 ` yukuai (C) 0 siblings, 1 reply; 8+ messages in thread From: Ming Lei @ 2021-08-09 9:46 UTC (permalink / raw) To: yukuai (C) Cc: axboe, josef, bvanassche, linux-block, linux-kernel, nbd, yi.zhang On Mon, Aug 09, 2021 at 03:08:26PM +0800, yukuai (C) wrote: > On 2021/08/09 14:28, Ming Lei wrote: > > On Mon, Aug 09, 2021 at 11:09:27AM +0800, Yu Kuai wrote: > > > blk_mq_tag_to_rq() might return freed request, use > > > blk_mq_get_rq_by_tag() instead. > > > > > > Signed-off-by: Yu Kuai <yukuai3@huawei.com> > > > --- > > > drivers/block/nbd.c | 11 ++++++----- > > > 1 file changed, 6 insertions(+), 5 deletions(-) > > > > > > diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c > > > index c38317979f74..9e56975a8eee 100644 > > > --- a/drivers/block/nbd.c > > > +++ b/drivers/block/nbd.c > > > @@ -713,11 +713,10 @@ static struct nbd_cmd *nbd_read_stat(struct nbd_device *nbd, int index) > > > tag = nbd_handle_to_tag(handle); > > > hwq = blk_mq_unique_tag_to_hwq(tag); > > > if (hwq < nbd->tag_set.nr_hw_queues) > > > - req = blk_mq_tag_to_rq(nbd->tag_set.tags[hwq], > > > - blk_mq_unique_tag_to_tag(tag)); > > > - if (!req || !blk_mq_request_started(req)) { > > > - dev_err(disk_to_dev(nbd->disk), "Unexpected reply (%d) %p\n", > > > - tag, req); > > > + req = blk_mq_get_rq_by_tag(nbd->tag_set.tags[hwq], > > > + blk_mq_unique_tag_to_tag(tag)); > > > + if (!req) { > > > + dev_err(disk_to_dev(nbd->disk), "Unexpected reply %d\n", tag); > > > return ERR_PTR(-ENOENT); > > > } > > > trace_nbd_header_received(req, handle); > > > @@ -779,6 +778,8 @@ static struct nbd_cmd *nbd_read_stat(struct nbd_device *nbd, int index) > > > } > > > out: > > > trace_nbd_payload_received(req, handle); > > > + if (req) > > > + blk_mq_put_rq_ref(req); > > > mutex_unlock(&cmd->lock); > > > return ret ? ERR_PTR(ret) : cmd; > > > > After blk_mq_put_rq_ref() returns, this request may have been freed, > > so the returned 'cmd' may have been freed too. > > > > As I replied in your another thread, it is driver's responsibility to > > cover race between normal completion and timeout/error handling, that > > means the caller of blk_mq_tag_to_rq need to make sure that the request > > represented by the passed 'tag' can't be freed. > > Hi, Ming > > There are two problems here in nbd, both reported by our syzkaller. > > The first is that blk_mq_tag_to_rq() returned a freed request, which is > because tags->static_rq[] is freed without clearing tags->rq[]. > Syzkaller log shows that a reply package is sent to client without > the client's request package. And this patch is trying to solve this > problem. It is still driver's problem: ->static_rq is freed in blk_mq_free_tag_set() which is called after blk_cleanup_disk() returns. Once blk_cleanup_disk() returns, there shouldn't be any driver activity, including calling blk_mq_tag_to_rq() by passing one invalid tag. Thanks, Ming ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] nbd: convert to use blk_mq_get_rq_by_tag() 2021-08-09 9:46 ` Ming Lei @ 2021-08-09 14:04 ` yukuai (C) 2021-08-10 1:48 ` Ming Lei 0 siblings, 1 reply; 8+ messages in thread From: yukuai (C) @ 2021-08-09 14:04 UTC (permalink / raw) To: Ming Lei Cc: axboe, josef, bvanassche, linux-block, linux-kernel, nbd, yi.zhang On 2021/08/09 17:46, Ming Lei wrote: > On Mon, Aug 09, 2021 at 03:08:26PM +0800, yukuai (C) wrote: >> On 2021/08/09 14:28, Ming Lei wrote: >>> On Mon, Aug 09, 2021 at 11:09:27AM +0800, Yu Kuai wrote: >>>> blk_mq_tag_to_rq() might return freed request, use >>>> blk_mq_get_rq_by_tag() instead. >>>> >>>> Signed-off-by: Yu Kuai <yukuai3@huawei.com> >>>> --- >>>> drivers/block/nbd.c | 11 ++++++----- >>>> 1 file changed, 6 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c >>>> index c38317979f74..9e56975a8eee 100644 >>>> --- a/drivers/block/nbd.c >>>> +++ b/drivers/block/nbd.c >>>> @@ -713,11 +713,10 @@ static struct nbd_cmd *nbd_read_stat(struct nbd_device *nbd, int index) >>>> tag = nbd_handle_to_tag(handle); >>>> hwq = blk_mq_unique_tag_to_hwq(tag); >>>> if (hwq < nbd->tag_set.nr_hw_queues) >>>> - req = blk_mq_tag_to_rq(nbd->tag_set.tags[hwq], >>>> - blk_mq_unique_tag_to_tag(tag)); >>>> - if (!req || !blk_mq_request_started(req)) { >>>> - dev_err(disk_to_dev(nbd->disk), "Unexpected reply (%d) %p\n", >>>> - tag, req); >>>> + req = blk_mq_get_rq_by_tag(nbd->tag_set.tags[hwq], >>>> + blk_mq_unique_tag_to_tag(tag)); >>>> + if (!req) { >>>> + dev_err(disk_to_dev(nbd->disk), "Unexpected reply %d\n", tag); >>>> return ERR_PTR(-ENOENT); >>>> } >>>> trace_nbd_header_received(req, handle); >>>> @@ -779,6 +778,8 @@ static struct nbd_cmd *nbd_read_stat(struct nbd_device *nbd, int index) >>>> } >>>> out: >>>> trace_nbd_payload_received(req, handle); >>>> + if (req) >>>> + blk_mq_put_rq_ref(req); >>>> mutex_unlock(&cmd->lock); >>>> return ret ? ERR_PTR(ret) : cmd; >>> >>> After blk_mq_put_rq_ref() returns, this request may have been freed, >>> so the returned 'cmd' may have been freed too. >>> >>> As I replied in your another thread, it is driver's responsibility to >>> cover race between normal completion and timeout/error handling, that >>> means the caller of blk_mq_tag_to_rq need to make sure that the request >>> represented by the passed 'tag' can't be freed. >> >> Hi, Ming >> >> There are two problems here in nbd, both reported by our syzkaller. >> >> The first is that blk_mq_tag_to_rq() returned a freed request, which is >> because tags->static_rq[] is freed without clearing tags->rq[]. >> Syzkaller log shows that a reply package is sent to client without >> the client's request package. And this patch is trying to solve this >> problem. > > It is still driver's problem: > > ->static_rq is freed in blk_mq_free_tag_set() which is called after > blk_cleanup_disk() returns. Once blk_cleanup_disk() returns, there > shouldn't be any driver activity, including calling blk_mq_tag_to_rq() > by passing one invalid tag. > Hi, Ming I understand if static_rq is freed through blk_mq_free_tag_set(), drivers should not use static_rq anymore. By the way, I was thinking about another path: blk_mq_update_nr_requests if (!hctx->sched_tags) -> if this is true ret = blk_mq_tag_update_depth(hctx, &hctx->tags, nr, false) blk_mq_free_rqs -> static_rq is freed here If this path concurrent with nbd_read_stat(), nbd_read_stat() can get a freed request by blk_mq_tag_to_rq(), since tags->lock is not held. t1: nbd_read_stat t2: blk_mq_update_nr_requests rq = blk_mq_tag_to_rq() blk_mq_free_rqs By holding tags->lock, we can check that rq state is idle, and it's ref is 0. Thanks Kuai ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] nbd: convert to use blk_mq_get_rq_by_tag() 2021-08-09 14:04 ` yukuai (C) @ 2021-08-10 1:48 ` Ming Lei 0 siblings, 0 replies; 8+ messages in thread From: Ming Lei @ 2021-08-10 1:48 UTC (permalink / raw) To: yukuai (C) Cc: axboe, josef, bvanassche, linux-block, linux-kernel, nbd, yi.zhang On Mon, Aug 09, 2021 at 10:04:32PM +0800, yukuai (C) wrote: > On 2021/08/09 17:46, Ming Lei wrote: > > On Mon, Aug 09, 2021 at 03:08:26PM +0800, yukuai (C) wrote: > > > On 2021/08/09 14:28, Ming Lei wrote: > > > > On Mon, Aug 09, 2021 at 11:09:27AM +0800, Yu Kuai wrote: > > > > > blk_mq_tag_to_rq() might return freed request, use > > > > > blk_mq_get_rq_by_tag() instead. > > > > > > > > > > Signed-off-by: Yu Kuai <yukuai3@huawei.com> > > > > > --- > > > > > drivers/block/nbd.c | 11 ++++++----- > > > > > 1 file changed, 6 insertions(+), 5 deletions(-) > > > > > > > > > > diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c > > > > > index c38317979f74..9e56975a8eee 100644 > > > > > --- a/drivers/block/nbd.c > > > > > +++ b/drivers/block/nbd.c > > > > > @@ -713,11 +713,10 @@ static struct nbd_cmd *nbd_read_stat(struct nbd_device *nbd, int index) > > > > > tag = nbd_handle_to_tag(handle); > > > > > hwq = blk_mq_unique_tag_to_hwq(tag); > > > > > if (hwq < nbd->tag_set.nr_hw_queues) > > > > > - req = blk_mq_tag_to_rq(nbd->tag_set.tags[hwq], > > > > > - blk_mq_unique_tag_to_tag(tag)); > > > > > - if (!req || !blk_mq_request_started(req)) { > > > > > - dev_err(disk_to_dev(nbd->disk), "Unexpected reply (%d) %p\n", > > > > > - tag, req); > > > > > + req = blk_mq_get_rq_by_tag(nbd->tag_set.tags[hwq], > > > > > + blk_mq_unique_tag_to_tag(tag)); > > > > > + if (!req) { > > > > > + dev_err(disk_to_dev(nbd->disk), "Unexpected reply %d\n", tag); > > > > > return ERR_PTR(-ENOENT); > > > > > } > > > > > trace_nbd_header_received(req, handle); > > > > > @@ -779,6 +778,8 @@ static struct nbd_cmd *nbd_read_stat(struct nbd_device *nbd, int index) > > > > > } > > > > > out: > > > > > trace_nbd_payload_received(req, handle); > > > > > + if (req) > > > > > + blk_mq_put_rq_ref(req); > > > > > mutex_unlock(&cmd->lock); > > > > > return ret ? ERR_PTR(ret) : cmd; > > > > > > > > After blk_mq_put_rq_ref() returns, this request may have been freed, > > > > so the returned 'cmd' may have been freed too. > > > > > > > > As I replied in your another thread, it is driver's responsibility to > > > > cover race between normal completion and timeout/error handling, that > > > > means the caller of blk_mq_tag_to_rq need to make sure that the request > > > > represented by the passed 'tag' can't be freed. > > > > > > Hi, Ming > > > > > > There are two problems here in nbd, both reported by our syzkaller. > > > > > > The first is that blk_mq_tag_to_rq() returned a freed request, which is > > > because tags->static_rq[] is freed without clearing tags->rq[]. > > > Syzkaller log shows that a reply package is sent to client without > > > the client's request package. And this patch is trying to solve this > > > problem. > > > > It is still driver's problem: > > > > ->static_rq is freed in blk_mq_free_tag_set() which is called after > > blk_cleanup_disk() returns. Once blk_cleanup_disk() returns, there > > shouldn't be any driver activity, including calling blk_mq_tag_to_rq() > > by passing one invalid tag. > > > > Hi, Ming > > I understand if static_rq is freed through blk_mq_free_tag_set(), > drivers should not use static_rq anymore. > > By the way, I was thinking about another path: > > blk_mq_update_nr_requests > if (!hctx->sched_tags) -> if this is true > ret = blk_mq_tag_update_depth(hctx, &hctx->tags, nr, false) > blk_mq_free_rqs -> static_rq is freed here > > If this path concurrent with nbd_read_stat(), nbd_read_stat() can > get a freed request by blk_mq_tag_to_rq(), since tags->lock is not > held. > > t1: nbd_read_stat t2: blk_mq_update_nr_requests > rq = blk_mq_tag_to_rq() > blk_mq_free_rqs t1 isn't supposed to happen when t2 is running. blk_mq_update_nr_requests() is only called by nbd_start_device(). nbd_start_device(): if (nbd->task_recv) return -EBUSY; ... nbd->recv_workq = alloc_workqueue() That means nbd_config_put() has been called and ->config_refs has dropped to zero, so socket has been shutdown, and ->recv_workq has been destroyed, so t1 isn't supposed to happen when t2 is running. > > By holding tags->lock, we can check that rq state is idle, and it's > ref is 0. Firstly tags->lock can't fix the race[1], secondly it should be addressed in driver. [1] https://lore.kernel.org/linux-block/20210809030927.1946162-2-yukuai3@huawei.com/T/#m6651289c5718b45a8ae8a7efc889248f8cb904a3 Thanks, Ming ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-08-10 1:49 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-08-09 3:09 [PATCH v2 0/2] fix request uaf in nbd_read_stat() Yu Kuai 2021-08-09 3:09 ` [PATCH v2 1/2] blk-mq: add a new interface to get request by tag Yu Kuai 2021-08-09 3:09 ` [PATCH v2 2/2] nbd: convert to use blk_mq_get_rq_by_tag() Yu Kuai 2021-08-09 6:28 ` Ming Lei 2021-08-09 7:08 ` yukuai (C) 2021-08-09 9:46 ` Ming Lei 2021-08-09 14:04 ` yukuai (C) 2021-08-10 1:48 ` Ming Lei
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).