Re: [PATCH v4 3/3] blk-mq: Fix a race between iterating over requests and freeing requests

[Khazhy Kumykov <khazhy@google.com>]

[-- Attachment #1: Type: text/plain, Size: 2346 bytes --]

On Fri, Apr 2, 2021 at 8:26 PM Bart Van Assche <bvanassche@acm.org> wrote:
>
> On 4/2/21 4:59 PM, Khazhy Kumykov wrote:
> > On Sun, Mar 28, 2021 at 7:00 PM Bart Van Assche <bvanassche@acm.org> wrote:
> >> @@ -209,7 +209,12 @@ static bool bt_iter(struct sbitmap *bitmap, unsigned int bitnr, void *data)
> >>
> >>         if (!reserved)
> >>                 bitnr += tags->nr_reserved_tags;
> >> -       rq = tags->rqs[bitnr];
> >> +       /*
> >> +        * See also the percpu_ref_tryget() and blk_queue_exit() calls in
> >> +        * blk_mq_queue_tag_busy_iter().
> >> +        */
> >> +       rq = rcu_dereference_check(tags->rqs[bitnr],
> >> +                       !percpu_ref_is_zero(&hctx->queue->q_usage_counter));
> >
> > do we need to worry about rq->q != hctx->queue here? i.e., could we
> > run into use-after-free on rq->q == hctx->queue check below, since
> > rq->q->q_usage_counter might not be raised? Once we verify rq->q ==
> > hctx->queue, i agree q_usage_counter is sufficient then
>
> That's a great question. I will change the second
> rcu_dereference_check() argument into 'true' and elaborate the comment
> above rcu_dereference_check().
>
> >> -static bool bt_tags_iter(struct sbitmap *bitmap, unsigned int bitnr, void *data)
> >> +static bool __bt_tags_iter(struct sbitmap *bitmap, unsigned int bitnr,
> >> +                          void *data)
> >>  {
> >>         struct bt_tags_iter_data *iter_data = data;
> >>         struct blk_mq_tags *tags = iter_data->tags;
> >> @@ -275,7 +286,7 @@ static bool bt_tags_iter(struct sbitmap *bitmap, unsigned int bitnr, void *data)
> >>         if (iter_data->flags & BT_TAG_ITER_STATIC_RQS)
> >>                 rq = tags->static_rqs[bitnr];
> >>         else
> >> -               rq = tags->rqs[bitnr];
> >> +               rq = rcu_dereference_check(tags->rqs[bitnr], true);
> >
> > lockdep_is_held(&tags->iter_rwsem) ?
>
> I will change the second rcu_dereference_check() argument into the
> following:
>
> rcu_read_lock_held() || lockdep_is_held(&tags->iter_rwsem)
rcu_dereference_check() already has a || rcu_read_lock_held(), fwiw
>
> >> +               /*
> >> +                * Freeing tags happens with the request queue frozen so the
> >> +                * srcu dereference below is protected by the request queue
> >
> > s/srcu/rcu
>
> Thanks, will fix.
>
> Bart.

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 3996 bytes --]