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;
Subject: Re: [PATCH] blk-ioc: protect ioc_destroy_icq() by 'queue_lock'
To:     Yu Kuai <yukuai1@huaweicloud.com>, hch@lst.de, dlemoal@kernel.org,
        quic_pragalla@quicinc.com, axboe@kernel.dk
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>
From:   Yu Kuai <yukuai1@huaweicloud.com>
Message-ID: <03ffbdc4-66e2-5508-f632-e3a1999f40df@huaweicloud.com>
Date:   Mon, 5 Jun 2023 20:58:05 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101
 Thunderbird/60.8.0
MIME-Version: 1.0
In-Reply-To: <20230531073435.2923422-1-yukuai1@huaweicloud.com>
Content-Type: text/plain; charset=gbk; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

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?

Sincerely apologize for this trouble.

Thanks,
Kuai
> -		if (!(icq->flags & ICQ_DESTROYED))
> -			ioc_destroy_icq(icq);
> +		ioc_destroy_icq(icq);
>   		spin_unlock_irq(&icq->ioc->lock);
>   	}
> -	rcu_read_unlock();
> +	spin_unlock_irq(&q->queue_lock);
>   }
>   #else /* CONFIG_BLK_ICQ */
>   static inline void ioc_exit_icqs(struct io_context *ioc)
>