Message ID | 20230531073435.2923422-1-yukuai1@huaweicloud.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:994d:0:b0:3d9:f83d:47d9 with SMTP id k13csp2717364vqr; Wed, 31 May 2023 01:23:40 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ7odwZKH2WDKo6b2DWa47YxRmFAD2sXTsrEmJJbPrmezLjk+9KRXhxJxbc3NpQ/toEv6uSt X-Received: by 2002:a05:6a20:d38b:b0:10a:fad7:43bf with SMTP id iq11-20020a056a20d38b00b0010afad743bfmr4596500pzb.39.1685521420538; Wed, 31 May 2023 01:23:40 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1685521420; cv=none; d=google.com; s=arc-20160816; b=F0f0FQvXlsS5Kf549HS7YFUPrTK8blkRJ5cx5wLOb6OESDLaYjgf5I0CmEHPvrQQFU 5ePrlNLlwwTsc9mS6icoj30dI12Ghedx5f6GCOe2FYqG8GhbdMXe/H426LQPordgJYzo 3stKVOU2kvYUZrsYjR8DYZrekzui2z7tDfs20Uy/JZyFDvQzuao0S0PdFQTwDextuNaj wWmL78IHrFdA2NpgL4Pz3xlDrnpR3rMmwgw+/H9T5SyyLoCx5Nfpwa6xiX1x6TKjzQ2u 3I+uEjYzcC2bVa36SqxJzrdY8ZKSFi/clwdL54K+1XC+ZF0Tg+meMSSzYFJvaIgz+RD2 UEJQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from; bh=grP6g14iosM9+92oxOxNfIDVH2cZGCR0qx/fIYH88Hs=; b=iaDqOJmkbYyc9Fsyu3E+uCFbvjQqeOELyFx8UMy/t7J6JxnUuaB21nn0DbBQXajt2t +jD9HEhlv1Vf2l4Aiw3JkIx4LBFFdCS29xfxYAS92gIxixnFxHN61lvP0/6GgSbizNmO ErzzzJVjuaIRK1kyAOAGkB2acO21CxeJc0KVpoQtn0AeZk3weOUfAWGcpYccdzM9VEZF kUoiVOM95GbtayFn76u2eMuTOPhBfrcLduMVhTMPjm/VIygaJTI9VhxenjjzCf4StiEK A07mPgDvi7X/2rI1lm7b3BK9DFLDqQWpo0T61IyKgX1CXaqQVNyU1zbax0iqpuvtc7J1 LpTw== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id p124-20020a622982000000b006439b23e8b7si3095467pfp.9.2023.05.31.01.23.28; Wed, 31 May 2023 01:23:40 -0700 (PDT) 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; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S234690AbjEaHis (ORCPT <rfc822;callmefire3@gmail.com> + 99 others); Wed, 31 May 2023 03:38:48 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:47056 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232077AbjEaHiq (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Wed, 31 May 2023 03:38:46 -0400 Received: from dggsgout11.his.huawei.com (unknown [45.249.212.51]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id E37E71A1; Wed, 31 May 2023 00:38:24 -0700 (PDT) Received: from mail02.huawei.com (unknown [172.30.67.153]) by dggsgout11.his.huawei.com (SkyGuard) with ESMTP id 4QWLj83DzPz4f3lXJ; Wed, 31 May 2023 15:38:20 +0800 (CST) Received: from huaweicloud.com (unknown [10.175.104.67]) by APP4 (Coremail) with SMTP id gCh0CgD3rLBp+XZk_RNsKg--.65307S4; Wed, 31 May 2023 15:38:19 +0800 (CST) From: Yu Kuai <yukuai1@huaweicloud.com> To: hch@lst.de, dlemoal@kernel.org, quic_pragalla@quicinc.com, axboe@kernel.dk Cc: linux-block@vger.kernel.org, linux-kernel@vger.kernel.org, yukuai3@huawei.com, yukuai1@huaweicloud.com, yi.zhang@huawei.com, yangerkun@huawei.com Subject: [PATCH] blk-ioc: protect ioc_destroy_icq() by 'queue_lock' Date: Wed, 31 May 2023 15:34:35 +0800 Message-Id: <20230531073435.2923422-1-yukuai1@huaweicloud.com> X-Mailer: git-send-email 2.39.2 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-CM-TRANSID: gCh0CgD3rLBp+XZk_RNsKg--.65307S4 X-Coremail-Antispam: 1UD129KBjvJXoWxWF18uryfWr4xKF18KF4kZwb_yoW5AFykpr yrWanxC348Jr4xWrWDKa17u3ySga1F9rsFyr4fGrZ5ArnFvrsIg3W8AryFqF9YqFZ7XrZ8 Xr47J395Cr4UCwUanT9S1TB71UUUUUUqnTZGkaVYY2UrUUUUjbIjqfuFe4nvWSU5nxnvy2 9KBjDU0xBIdaVrnRJUUUvY14x267AKxVW8JVW5JwAFc2x0x2IEx4CE42xK8VAvwI8IcIk0 rVWrJVCq3wAFIxvE14AKwVWUJVWUGwA2ocxC64kIII0Yj41l84x0c7CEw4AK67xGY2AK02 1l84ACjcxK6xIIjxv20xvE14v26F1j6w1UM28EF7xvwVC0I7IYx2IY6xkF7I0E14v26r4U JVWxJr1l84ACjcxK6I8E87Iv67AKxVW0oVCq3wA2z4x0Y4vEx4A2jsIEc7CjxVAFwI0_Gc CE3s1le2I262IYc4CY6c8Ij28IcVAaY2xG8wAqx4xG64xvF2IEw4CE5I8CrVC2j2WlYx0E 2Ix0cI8IcVAFwI0_Jr0_Jr4lYx0Ex4A2jsIE14v26r1j6r4UMcvjeVCFs4IE7xkEbVWUJV W8JwACjcxG0xvY0x0EwIxGrwACjI8F5VA0II8E6IAqYI8I648v4I1lFIxGxcIEc7CjxVA2 Y2ka0xkIwI1l42xK82IYc2Ij64vIr41l4I8I3I0E4IkC6x0Yz7v_Jr0_Gr1lx2IqxVAqx4 xG67AKxVWUJVWUGwC20s026x8GjcxK67AKxVWUGVWUWwC2zVAF1VAY17CE14v26r1q6r43 MIIYrxkI7VAKI48JMIIF0xvE2Ix0cI8IcVAFwI0_Jr0_JF4lIxAIcVC0I7IYx2IY6xkF7I 0E14v26r1j6r4UMIIF0xvE42xK8VAvwI8IcIk0rVWrZr1j6s0DMIIF0xvEx4A2jsIE14v2 6r1j6r4UMIIF0xvEx4A2jsIEc7CjxVAFwI0_Gr0_Gr1UYxBIdaVFxhVjvjDU0xZFpf9x0J UdHUDUUUUU= X-CM-SenderInfo: 51xn3trlr6x35dzhxuhorxvhhfrp/ X-CFilter-Loop: Reflected X-Spam-Status: No, score=-1.5 required=5.0 tests=BAYES_00,KHOP_HELO_FCRDNS, MAY_BE_FORGED,SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1767397309273478365?= X-GMAIL-MSGID: =?utf-8?q?1767397309273478365?= |
Series |
blk-ioc: protect ioc_destroy_icq() by 'queue_lock'
|
|
Commit Message
Yu Kuai
May 31, 2023, 7:34 a.m. UTC
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(-)
Comments
Looks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>
On Wed, 31 May 2023 15:34:35 +0800, Yu Kuai wrote: > 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. > */ > > [...] Applied, thanks! [1/1] blk-ioc: protect ioc_destroy_icq() by 'queue_lock' commit: 2dea233fdc6a00e53bf2ee5cd4b6fca353fd81f8 Best regards,
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) >
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.
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); - 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)