* Re: [PATCH] blk-mq: Avoid race condition with uninitialized requests
2014-09-18 9:04 ` David Hildenbrand
@ 2014-09-19 12:41 ` Christian Borntraeger
2014-09-19 12:41 ` Christian Borntraeger
` (2 subsequent siblings)
3 siblings, 0 replies; 10+ messages in thread
From: Christian Borntraeger @ 2014-09-19 12:41 UTC (permalink / raw)
To: David Hildenbrand, tom.leiming, axboe
Cc: kvm, mst, linux-kernel, stable, virtualization
On 09/18/2014 11:04 AM, David Hildenbrand wrote:
> This patch should fix the bug reported in https://lkml.org/lkml/2014/9/11/249.
>
> We have to initialize at least the atomic_flags and the cmd_flags when
> allocating storage for the requests.
>
> Otherwise blk_mq_timeout_check() might dereference uninitialized pointers when
> racing with the creation of a request.
>
> Also move the reset of cmd_flags for the initializing code to the point where a
> request is freed. So we will never end up with pending flush request indicators
> that might trigger dereferences of invalid pointers in blk_mq_timeout_check().
>
> Cc: stable@vger.kernel.org
> Signed-off-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
So far our testers were not able to reproduce the problem with this patch applied, we will continue testing over weekend. I will give an ACK/NACK on monday then.
Thanks
> ---
> block/blk-mq.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index 383ea0c..eed6340 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -203,7 +203,6 @@ __blk_mq_alloc_request(struct blk_mq_alloc_data *data, int rw)
> if (tag != BLK_MQ_TAG_FAIL) {
> rq = data->hctx->tags->rqs[tag];
>
> - rq->cmd_flags = 0;
> if (blk_mq_tag_busy(data->hctx)) {
> rq->cmd_flags = REQ_MQ_INFLIGHT;
> atomic_inc(&data->hctx->nr_active);
> @@ -258,6 +257,7 @@ static void __blk_mq_free_request(struct blk_mq_hw_ctx *hctx,
>
> if (rq->cmd_flags & REQ_MQ_INFLIGHT)
> atomic_dec(&hctx->nr_active);
> + rq->cmd_flags = 0;
>
> clear_bit(REQ_ATOM_STARTED, &rq->atomic_flags);
> blk_mq_put_tag(hctx, tag, &ctx->last_tag);
> @@ -1404,6 +1404,8 @@ static struct blk_mq_tags *blk_mq_init_rq_map(struct blk_mq_tag_set *set,
> left -= to_do * rq_size;
> for (j = 0; j < to_do; j++) {
> tags->rqs[i] = p;
> + tags->rqs[i]->atomic_flags = 0;
> + tags->rqs[i]->cmd_flags = 0;
> if (set->ops->init_request) {
> if (set->ops->init_request(set->driver_data,
> tags->rqs[i], hctx_idx, i,
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] blk-mq: Avoid race condition with uninitialized requests
2014-09-18 9:04 ` David Hildenbrand
2014-09-19 12:41 ` Christian Borntraeger
@ 2014-09-19 12:41 ` Christian Borntraeger
2014-09-22 14:15 ` Christian Borntraeger
2014-09-22 14:15 ` Christian Borntraeger
3 siblings, 0 replies; 10+ messages in thread
From: Christian Borntraeger @ 2014-09-19 12:41 UTC (permalink / raw)
To: David Hildenbrand, tom.leiming, axboe
Cc: kvm, stable, rusty, virtualization, mst, linux-kernel
On 09/18/2014 11:04 AM, David Hildenbrand wrote:
> This patch should fix the bug reported in https://lkml.org/lkml/2014/9/11/249.
>
> We have to initialize at least the atomic_flags and the cmd_flags when
> allocating storage for the requests.
>
> Otherwise blk_mq_timeout_check() might dereference uninitialized pointers when
> racing with the creation of a request.
>
> Also move the reset of cmd_flags for the initializing code to the point where a
> request is freed. So we will never end up with pending flush request indicators
> that might trigger dereferences of invalid pointers in blk_mq_timeout_check().
>
> Cc: stable@vger.kernel.org
> Signed-off-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
So far our testers were not able to reproduce the problem with this patch applied, we will continue testing over weekend. I will give an ACK/NACK on monday then.
Thanks
> ---
> block/blk-mq.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index 383ea0c..eed6340 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -203,7 +203,6 @@ __blk_mq_alloc_request(struct blk_mq_alloc_data *data, int rw)
> if (tag != BLK_MQ_TAG_FAIL) {
> rq = data->hctx->tags->rqs[tag];
>
> - rq->cmd_flags = 0;
> if (blk_mq_tag_busy(data->hctx)) {
> rq->cmd_flags = REQ_MQ_INFLIGHT;
> atomic_inc(&data->hctx->nr_active);
> @@ -258,6 +257,7 @@ static void __blk_mq_free_request(struct blk_mq_hw_ctx *hctx,
>
> if (rq->cmd_flags & REQ_MQ_INFLIGHT)
> atomic_dec(&hctx->nr_active);
> + rq->cmd_flags = 0;
>
> clear_bit(REQ_ATOM_STARTED, &rq->atomic_flags);
> blk_mq_put_tag(hctx, tag, &ctx->last_tag);
> @@ -1404,6 +1404,8 @@ static struct blk_mq_tags *blk_mq_init_rq_map(struct blk_mq_tag_set *set,
> left -= to_do * rq_size;
> for (j = 0; j < to_do; j++) {
> tags->rqs[i] = p;
> + tags->rqs[i]->atomic_flags = 0;
> + tags->rqs[i]->cmd_flags = 0;
> if (set->ops->init_request) {
> if (set->ops->init_request(set->driver_data,
> tags->rqs[i], hctx_idx, i,
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] blk-mq: Avoid race condition with uninitialized requests
2014-09-18 9:04 ` David Hildenbrand
2014-09-19 12:41 ` Christian Borntraeger
2014-09-19 12:41 ` Christian Borntraeger
@ 2014-09-22 14:15 ` Christian Borntraeger
2014-09-22 14:15 ` Christian Borntraeger
3 siblings, 0 replies; 10+ messages in thread
From: Christian Borntraeger @ 2014-09-22 14:15 UTC (permalink / raw)
To: tom.leiming, axboe
Cc: kvm, mst, ppinatti, linux-kernel, stable, virtualization,
David Hildenbrand
On 09/18/2014 11:04 AM, David Hildenbrand wrote:
> This patch should fix the bug reported in https://lkml.org/lkml/2014/9/11/249.
>
> We have to initialize at least the atomic_flags and the cmd_flags when
> allocating storage for the requests.
>
> Otherwise blk_mq_timeout_check() might dereference uninitialized pointers when
> racing with the creation of a request.
>
> Also move the reset of cmd_flags for the initializing code to the point where a
> request is freed. So we will never end up with pending flush request indicators
> that might trigger dereferences of invalid pointers in blk_mq_timeout_check().
>
> Cc: stable@vger.kernel.org
> Signed-off-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
Can you please add
Reported-by: Paulo De Rezende Pinatti <ppinatti@linux.vnet.ibm.com>
Tested-by: Paulo De Rezende Pinatti <ppinatti@linux.vnet.ibm.com>
as Paulo did the testing work?
We thing this patch is fine and should go upstream.
Christian
> ---
> block/blk-mq.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index 383ea0c..eed6340 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -203,7 +203,6 @@ __blk_mq_alloc_request(struct blk_mq_alloc_data *data, int rw)
> if (tag != BLK_MQ_TAG_FAIL) {
> rq = data->hctx->tags->rqs[tag];
>
> - rq->cmd_flags = 0;
> if (blk_mq_tag_busy(data->hctx)) {
> rq->cmd_flags = REQ_MQ_INFLIGHT;
> atomic_inc(&data->hctx->nr_active);
> @@ -258,6 +257,7 @@ static void __blk_mq_free_request(struct blk_mq_hw_ctx *hctx,
>
> if (rq->cmd_flags & REQ_MQ_INFLIGHT)
> atomic_dec(&hctx->nr_active);
> + rq->cmd_flags = 0;
>
> clear_bit(REQ_ATOM_STARTED, &rq->atomic_flags);
> blk_mq_put_tag(hctx, tag, &ctx->last_tag);
> @@ -1404,6 +1404,8 @@ static struct blk_mq_tags *blk_mq_init_rq_map(struct blk_mq_tag_set *set,
> left -= to_do * rq_size;
> for (j = 0; j < to_do; j++) {
> tags->rqs[i] = p;
> + tags->rqs[i]->atomic_flags = 0;
> + tags->rqs[i]->cmd_flags = 0;
> if (set->ops->init_request) {
> if (set->ops->init_request(set->driver_data,
> tags->rqs[i], hctx_idx, i,
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] blk-mq: Avoid race condition with uninitialized requests
2014-09-18 9:04 ` David Hildenbrand
` (2 preceding siblings ...)
2014-09-22 14:15 ` Christian Borntraeger
@ 2014-09-22 14:15 ` Christian Borntraeger
2014-09-22 14:17 ` Jens Axboe
3 siblings, 1 reply; 10+ messages in thread
From: Christian Borntraeger @ 2014-09-22 14:15 UTC (permalink / raw)
To: tom.leiming, axboe
Cc: David Hildenbrand, kvm, stable, rusty, virtualization, mst,
linux-kernel, ppinatti
On 09/18/2014 11:04 AM, David Hildenbrand wrote:
> This patch should fix the bug reported in https://lkml.org/lkml/2014/9/11/249.
>
> We have to initialize at least the atomic_flags and the cmd_flags when
> allocating storage for the requests.
>
> Otherwise blk_mq_timeout_check() might dereference uninitialized pointers when
> racing with the creation of a request.
>
> Also move the reset of cmd_flags for the initializing code to the point where a
> request is freed. So we will never end up with pending flush request indicators
> that might trigger dereferences of invalid pointers in blk_mq_timeout_check().
>
> Cc: stable@vger.kernel.org
> Signed-off-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
Can you please add
Reported-by: Paulo De Rezende Pinatti <ppinatti@linux.vnet.ibm.com>
Tested-by: Paulo De Rezende Pinatti <ppinatti@linux.vnet.ibm.com>
as Paulo did the testing work?
We thing this patch is fine and should go upstream.
Christian
> ---
> block/blk-mq.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index 383ea0c..eed6340 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -203,7 +203,6 @@ __blk_mq_alloc_request(struct blk_mq_alloc_data *data, int rw)
> if (tag != BLK_MQ_TAG_FAIL) {
> rq = data->hctx->tags->rqs[tag];
>
> - rq->cmd_flags = 0;
> if (blk_mq_tag_busy(data->hctx)) {
> rq->cmd_flags = REQ_MQ_INFLIGHT;
> atomic_inc(&data->hctx->nr_active);
> @@ -258,6 +257,7 @@ static void __blk_mq_free_request(struct blk_mq_hw_ctx *hctx,
>
> if (rq->cmd_flags & REQ_MQ_INFLIGHT)
> atomic_dec(&hctx->nr_active);
> + rq->cmd_flags = 0;
>
> clear_bit(REQ_ATOM_STARTED, &rq->atomic_flags);
> blk_mq_put_tag(hctx, tag, &ctx->last_tag);
> @@ -1404,6 +1404,8 @@ static struct blk_mq_tags *blk_mq_init_rq_map(struct blk_mq_tag_set *set,
> left -= to_do * rq_size;
> for (j = 0; j < to_do; j++) {
> tags->rqs[i] = p;
> + tags->rqs[i]->atomic_flags = 0;
> + tags->rqs[i]->cmd_flags = 0;
> if (set->ops->init_request) {
> if (set->ops->init_request(set->driver_data,
> tags->rqs[i], hctx_idx, i,
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] blk-mq: Avoid race condition with uninitialized requests
2014-09-22 14:15 ` Christian Borntraeger
@ 2014-09-22 14:17 ` Jens Axboe
0 siblings, 0 replies; 10+ messages in thread
From: Jens Axboe @ 2014-09-22 14:17 UTC (permalink / raw)
To: Christian Borntraeger, tom.leiming
Cc: kvm, mst, ppinatti, linux-kernel, stable, virtualization,
David Hildenbrand
On 2014-09-22 08:15, Christian Borntraeger wrote:
> On 09/18/2014 11:04 AM, David Hildenbrand wrote:
>> This patch should fix the bug reported in https://lkml.org/lkml/2014/9/11/249.
>>
>> We have to initialize at least the atomic_flags and the cmd_flags when
>> allocating storage for the requests.
>>
>> Otherwise blk_mq_timeout_check() might dereference uninitialized pointers when
>> racing with the creation of a request.
>>
>> Also move the reset of cmd_flags for the initializing code to the point where a
>> request is freed. So we will never end up with pending flush request indicators
>> that might trigger dereferences of invalid pointers in blk_mq_timeout_check().
>>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
>
> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
>
> Can you please add
> Reported-by: Paulo De Rezende Pinatti <ppinatti@linux.vnet.ibm.com>
> Tested-by: Paulo De Rezende Pinatti <ppinatti@linux.vnet.ibm.com>
>
> as Paulo did the testing work?
>
> We thing this patch is fine and should go upstream.
I might have to pick'n rebase the series, in which case I'll add it. But
I already queued it up last week, so if I don't, then I can't easily add
it. I wish the git notes wasn't such a horrible and unusable hack, so we
had a chance to annotate commits without having to rewrite history...
--
Jens Axboe
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] blk-mq: Avoid race condition with uninitialized requests
@ 2014-09-22 14:17 ` Jens Axboe
0 siblings, 0 replies; 10+ messages in thread
From: Jens Axboe @ 2014-09-22 14:17 UTC (permalink / raw)
To: Christian Borntraeger, tom.leiming
Cc: David Hildenbrand, kvm, stable, rusty, virtualization, mst,
linux-kernel, ppinatti
On 2014-09-22 08:15, Christian Borntraeger wrote:
> On 09/18/2014 11:04 AM, David Hildenbrand wrote:
>> This patch should fix the bug reported in https://lkml.org/lkml/2014/9/11/249.
>>
>> We have to initialize at least the atomic_flags and the cmd_flags when
>> allocating storage for the requests.
>>
>> Otherwise blk_mq_timeout_check() might dereference uninitialized pointers when
>> racing with the creation of a request.
>>
>> Also move the reset of cmd_flags for the initializing code to the point where a
>> request is freed. So we will never end up with pending flush request indicators
>> that might trigger dereferences of invalid pointers in blk_mq_timeout_check().
>>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
>
> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
>
> Can you please add
> Reported-by: Paulo De Rezende Pinatti <ppinatti@linux.vnet.ibm.com>
> Tested-by: Paulo De Rezende Pinatti <ppinatti@linux.vnet.ibm.com>
>
> as Paulo did the testing work?
>
> We thing this patch is fine and should go upstream.
I might have to pick'n rebase the series, in which case I'll add it. But
I already queued it up last week, so if I don't, then I can't easily add
it. I wish the git notes wasn't such a horrible and unusable hack, so we
had a chance to annotate commits without having to rewrite history...
--
Jens Axboe
^ permalink raw reply [flat|nested] 10+ messages in thread