Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <286f350c-b1b3-694d-b969-d06747b52144@kernel.dk>
Date:   Mon, 5 Jun 2023 10:52:18 -0600
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux aarch64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Subject: Re: [PATCH] blk-ioc: protect ioc_destroy_icq() by 'queue_lock'
Content-Language: en-US
To:     Yu Kuai <yukuai1@huaweicloud.com>, hch@lst.de, dlemoal@kernel.org,
        quic_pragalla@quicinc.com
Cc:     linux-block@vger.kernel.org, linux-kernel@vger.kernel.org,
        yi.zhang@huawei.com, yangerkun@huawei.com,
        "yukuai (C)" <yukuai3@huawei.com>
References: <20230531073435.2923422-1-yukuai1@huaweicloud.com>
 <03ffbdc4-66e2-5508-f632-e3a1999f40df@huaweicloud.com>
From:   Jens Axboe <axboe@kernel.dk>
In-Reply-To: <03ffbdc4-66e2-5508-f632-e3a1999f40df@huaweicloud.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 6/5/23 6:58 AM, Yu Kuai wrote:
> Hi, Jens
> 
> 在 2023/05/31 15:34, Yu Kuai 写道:
>> From: Yu Kuai <yukuai3@huawei.com>
>>
>> Currently, icq is tracked by both request_queue(icq->q_node) and
>> task(icq->ioc_node), and ioc_clear_queue() from elevator exit is not
>> safe because it can access the list without protection:
>>
>> ioc_clear_queue            ioc_release_fn
>>   lock queue_lock
>>   list_splice
>>   /* move queue list to a local list */
>>   unlock queue_lock
>>   /*
>>    * lock is released, the local list
>>    * can be accessed through task exit.
>>    */
>>
>>                 lock ioc->lock
>>                 while (!hlist_empty)
>>                  icq = hlist_entry
>>                  lock queue_lock
>>                   ioc_destroy_icq
>>                    delete icq->ioc_node
>>   while (!list_empty)
>>    icq = list_entry()           list_del icq->q_node
>>    /*
>>     * This is not protected by any lock,
>>     * list_entry concurrent with list_del
>>     * is not safe.
>>     */
>>
>>                  unlock queue_lock
>>                 unlock ioc->lock
>>
>> Fix this problem by protecting list 'icq->q_node' by queue_lock from
>> ioc_clear_queue().
>>
>> Reported-and-tested-by: Pradeep Pragallapati <quic_pragalla@quicinc.com>
>> Link: https://lore.kernel.org/lkml/20230517084434.18932-1-quic_pragalla@quicinc.com/
>> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
>> ---
>>   block/blk-ioc.c | 30 +++++++++++++-----------------
>>   1 file changed, 13 insertions(+), 17 deletions(-)
>>
>> diff --git a/block/blk-ioc.c b/block/blk-ioc.c
>> index 63fc02042408..d5db92e62c43 100644
>> --- a/block/blk-ioc.c
>> +++ b/block/blk-ioc.c
>> @@ -77,6 +77,10 @@ static void ioc_destroy_icq(struct io_cq *icq)
>>       struct elevator_type *et = q->elevator->type;
>>         lockdep_assert_held(&ioc->lock);
>> +    lockdep_assert_held(&q->queue_lock);
>> +
>> +    if (icq->flags & ICQ_DESTROYED)
>> +        return;
>>         radix_tree_delete(&ioc->icq_tree, icq->q->id);
>>       hlist_del_init(&icq->ioc_node);
>> @@ -128,12 +132,7 @@ static void ioc_release_fn(struct work_struct *work)
>>               spin_lock(&q->queue_lock);
>>               spin_lock(&ioc->lock);
>>   -            /*
>> -             * The icq may have been destroyed when the ioc lock
>> -             * was released.
>> -             */
>> -            if (!(icq->flags & ICQ_DESTROYED))
>> -                ioc_destroy_icq(icq);
>> +            ioc_destroy_icq(icq);
>>                 spin_unlock(&q->queue_lock);
>>               rcu_read_unlock();
>> @@ -171,23 +170,20 @@ static bool ioc_delay_free(struct io_context *ioc)
>>    */
>>   void ioc_clear_queue(struct request_queue *q)
>>   {
>> -    LIST_HEAD(icq_list);
>> -
>>       spin_lock_irq(&q->queue_lock);
>> -    list_splice_init(&q->icq_list, &icq_list);
>> -    spin_unlock_irq(&q->queue_lock);
>> -
>> -    rcu_read_lock();
>> -    while (!list_empty(&icq_list)) {
>> +    while (!list_empty(&q->icq_list)) {
>>           struct io_cq *icq =
>> -            list_entry(icq_list.next, struct io_cq, q_node);
>> +            list_first_entry(&q->icq_list, struct io_cq, q_node);
>>   +        /*
>> +         * Other context won't hold ioc lock to wait for queue_lock, see
>> +         * details in ioc_release_fn().
>> +         */
>>           spin_lock_irq(&icq->ioc->lock);
> 
> Sorry that I made a mistake here to use spin_lock_irq() for recursive
> locking.
> 
> Should I resend this patch or send a new fix patch?

Your patch is already staged in for-6.5/block, so please send a patch
that fixes up the current tree.

-- 
Jens Axboe