Message ID | 20230315061810.653263-1-yukuai1@huaweicloud.com |
---|---|
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:5915:0:0:0:0:0 with SMTP id v21csp2163848wrd; Tue, 14 Mar 2023 23:21:15 -0700 (PDT) X-Google-Smtp-Source: AK7set+Jr9f294uVK+5i3GItN2CRs9pUq5d20JFn4ZzoTaycklkP46zFbFBAZBVqcBfDaeFpMvmw X-Received: by 2002:a17:902:fb08:b0:19c:ff35:35d1 with SMTP id le8-20020a170902fb0800b0019cff3535d1mr1450545plb.6.1678861275154; Tue, 14 Mar 2023 23:21:15 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1678861275; cv=none; d=google.com; s=arc-20160816; b=QaAADs+94zgaTW6BH0V18EHm5VzDKIGLUmGEVc9h98xn37uD4GDSHt2KIuEoPL8671 eVojbaTFnfDeIaC/SA/LXZwajXKRK5JmXq0+7+WQzWr6ZQlGutcRFBH9gjjtyUEcqr0M 73Cv578sMbLPjXOB8Ad7GHjmP+66COw+EucFWY034IZJaplG2bBPBxnuuQiDHgu2pz0N +Csmd+P8t1m0hzhNOTZ/ntnfBh851lekHp7xo6O/7oncelaMOA7oE49CblDCTeMTGhDm dK0rdXreSYKyw9zmbnQGSBnBTPwb4z4wDnOy4Cl3MGEutZwTXYe5WmBg0lnXuUHf8ZbI Pn0Q== 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=QaMckrd94cQZDWjN8e3uFmXbrnS9hc0ZbPP/btJCqbg=; b=W0ldoVZd50wBe2RukuZv23L5khsBkj77Wx71hoY6tweZXgeBQnLp5+im/SPESgcKfh s9FAkwnOnNIw7Oy51SVYPZFazasouv08yNSbpTftku9dkAjmy1LqycFwjhk/Z0JTRErD 33YsCQ1FyEJw7eWr/dJ82nHNOv0EMUh0rne6CJV3lvF5gChsNGGKq4lzW49r7wHLTOJQ r5/fK2rdFBHyx5dmIEcJeZ205WfYUAnw5yjEwDNQlgp7g9rN8eH8tUPu/mBiozb51JYp ExGAeGIN8wKkZuhWx1kmp+hYxAj3wbUvStb9tcnMVONpftefi9FT44CupDGMq5U+t7kU LVlw== 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 b19-20020a170902ed1300b0019c3ce49a13si4241799pld.372.2023.03.14.23.21.00; Tue, 14 Mar 2023 23:21:15 -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 S230463AbjCOGTC (ORCPT <rfc822;realc9580@gmail.com> + 99 others); Wed, 15 Mar 2023 02:19:02 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:52106 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229506AbjCOGTA (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Wed, 15 Mar 2023 02:19:00 -0400 Received: from dggsgout11.his.huawei.com (unknown [45.249.212.51]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 0AA4E26582; Tue, 14 Mar 2023 23:18:59 -0700 (PDT) Received: from mail02.huawei.com (unknown [172.30.67.169]) by dggsgout11.his.huawei.com (SkyGuard) with ESMTP id 4Pc0b16Gwfz4f4XQf; Wed, 15 Mar 2023 14:18:53 +0800 (CST) Received: from huaweicloud.com (unknown [10.175.127.227]) by APP3 (Coremail) with SMTP id _Ch0CgCnUyFNYxFklvvMEw--.20241S4; Wed, 15 Mar 2023 14:18:55 +0800 (CST) From: Yu Kuai <yukuai1@huaweicloud.com> To: agk@redhat.com, snitzer@kernel.org, song@kernel.org Cc: linux-kernel@vger.kernel.org, linux-raid@vger.kernel.org, yukuai3@huawei.com, yukuai1@huaweicloud.com, yi.zhang@huawei.com, yangerkun@huawei.com Subject: [PATCH v2 0/5] md: fix uaf for sync_thread Date: Wed, 15 Mar 2023 14:18:05 +0800 Message-Id: <20230315061810.653263-1-yukuai1@huaweicloud.com> X-Mailer: git-send-email 2.31.1 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-CM-TRANSID: _Ch0CgCnUyFNYxFklvvMEw--.20241S4 X-Coremail-Antispam: 1UD129KBjvJXoW7Ar1UKr13Cr45CrW8Kr48Zwb_yoW8AFW3pF yfJry3Zr48ArsxZrnrXFyjka45Jw1Igay7KryxCw4fu3W5XrWYqr4YyFWkZF9rZFyfJFsr Xr15JF1kuF4DKaDanT9S1TB71UUUUUUqnTZGkaVYY2UrUUUUjbIjqfuFe4nvWSU5nxnvy2 9KBjDU0xBIdaVrnRJUUUyG14x267AKxVW8JVW5JwAFc2x0x2IEx4CE42xK8VAvwI8IcIk0 rVWrJVCq3wAFIxvE14AKwVWUJVWUGwA2ocxC64kIII0Yj41l84x0c7CEw4AK67xGY2AK02 1l84ACjcxK6xIIjxv20xvE14v26w1j6s0DM28EF7xvwVC0I7IYx2IY6xkF7I0E14v26r4U JVWxJr1l84ACjcxK6I8E87Iv67AKxVW0oVCq3wA2z4x0Y4vEx4A2jsIEc7CjxVAFwI0_Gc CE3s1le2I262IYc4CY6c8Ij28IcVAaY2xG8wAqx4xG64xvF2IEw4CE5I8CrVC2j2WlYx0E 2Ix0cI8IcVAFwI0_Jr0_Jr4lYx0Ex4A2jsIE14v26r1j6r4UMcvjeVCFs4IE7xkEbVWUJV W8JwACjcxG0xvY0x0EwIxGrwACjI8F5VA0II8E6IAqYI8I648v4I1l42xK82IYc2Ij64vI r41l4I8I3I0E4IkC6x0Yz7v_Jr0_Gr1lx2IqxVAqx4xG67AKxVWUJVWUGwC20s026x8Gjc xK67AKxVWUGVWUWwC2zVAF1VAY17CE14v26r1q6r43MIIYrxkI7VAKI48JMIIF0xvE2Ix0 cI8IcVAFwI0_Jr0_JF4lIxAIcVC0I7IYx2IY6xkF7I0E14v26r1j6r4UMIIF0xvE42xK8V AvwI8IcIk0rVWrZr1j6s0DMIIF0xvEx4A2jsIE14v26r1j6r4UMIIF0xvEx4A2jsIEc7Cj xVAFwI0_Jr0_GrUvcSsGvfC2KfnxnUUI43ZEXa7VUbXdbUUUUUU== X-CM-SenderInfo: 51xn3trlr6x35dzhxuhorxvhhfrp/ X-CFilter-Loop: Reflected X-Spam-Status: No, score=-0.5 required=5.0 tests=BAYES_00,KHOP_HELO_FCRDNS, MAY_BE_FORGED,SPF_HELO_NONE,SPF_NONE 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?1760413640400738404?= X-GMAIL-MSGID: =?utf-8?q?1760413640400738404?= |
Series |
md: fix uaf for sync_thread
|
|
Message
Yu Kuai
March 15, 2023, 6:18 a.m. UTC
From: Yu Kuai <yukuai3@huawei.com>
Changes in v2:
- fix a compile error for for md-cluster in patch 2
- replace spin_lock/unlock with spin_lock/unlock_irq in patch 5
- don't wake up inside the new lock in md wakeup_thread in patch 5
Our test reports a uaf for 'mddev->sync_thread':
T1 T2
md_start_sync
md_register_thread
raid1d
md_check_recovery
md_reap_sync_thread
md_unregister_thread
kfree
md_wakeup_thread
wake_up
->sync_thread was freed
Currently, a global spinlock 'pers_lock' is borrowed to protect
'mddev->thread', this problem can be fixed likewise, however, there might
be similar problem for other md_thread, and I really don't like the idea to
borrow a global lock.
This patchset do some refactor, and then use a disk level spinlock to
protect md_thread in relevant apis.
I tested this pathset with mdadm tests, and there are no new regression,
by the way, following test will failed with or without this patchset:
01raid6integ
04r1update
05r6tor0
10ddf-create
10ddf-fail-spare
10ddf-fail-stop-readd
10ddf-geometry
Yu Kuai (5):
md: pass a md_thread pointer to md_register_thread()
md: refactor md_wakeup_thread()
md: use md_thread api to wake up sync_thread
md: pass a mddev to md_unregister_thread()
md: protect md_thread with a new disk level spin lock
drivers/md/dm-raid.c | 6 +-
drivers/md/md-bitmap.c | 6 +-
drivers/md/md-cluster.c | 39 +++++----
drivers/md/md-multipath.c | 8 +-
drivers/md/md.c | 162 ++++++++++++++++++++------------------
drivers/md/md.h | 15 ++--
drivers/md/raid1.c | 19 +++--
drivers/md/raid10.c | 31 ++++----
drivers/md/raid5-cache.c | 19 +++--
drivers/md/raid5-ppl.c | 2 +-
drivers/md/raid5.c | 48 ++++++-----
11 files changed, 177 insertions(+), 178 deletions(-)
Comments
Dear Logan, Am 15.03.23 um 07:18 schrieb Yu Kuai: > From: Yu Kuai <yukuai3@huawei.com> > > Changes in v2: > - fix a compile error for for md-cluster in patch 2 > - replace spin_lock/unlock with spin_lock/unlock_irq in patch 5 > - don't wake up inside the new lock in md wakeup_thread in patch 5 > > Our test reports a uaf for 'mddev->sync_thread': > > T1 T2 > md_start_sync > md_register_thread > raid1d > md_check_recovery > md_reap_sync_thread > md_unregister_thread > kfree > > md_wakeup_thread > wake_up > ->sync_thread was freed > > Currently, a global spinlock 'pers_lock' is borrowed to protect > 'mddev->thread', this problem can be fixed likewise, however, there might > be similar problem for other md_thread, and I really don't like the idea to > borrow a global lock. > > This patchset do some refactor, and then use a disk level spinlock to > protect md_thread in relevant apis. > > I tested this pathset with mdadm tests, and there are no new regression, > by the way, following test will failed with or without this patchset: > > 01raid6integ > 04r1update > 05r6tor0 > 10ddf-create > 10ddf-fail-spare > 10ddf-fail-stop-readd > 10ddf-geometry As you improved the tests in the past, can you confirm, these failed on your test systems too and are fixed now? […] Kind regards, Paul
On 2023-03-15 02:30, Paul Menzel wrote: > Am 15.03.23 um 07:18 schrieb Yu Kuai: >> I tested this pathset with mdadm tests, and there are no new regression, >> by the way, following test will failed with or without this patchset: >> >> 01raid6integ >> 04r1update >> 05r6tor0 >> 10ddf-create >> 10ddf-fail-spare >> 10ddf-fail-stop-readd >> 10ddf-geometry > > As you improved the tests in the past, can you confirm, these failed on > your test systems too and are fixed now? Hmm, well Yu did not claim that those tests were fixed. If you re-read what was said, the tests listed failed with or without the new changes. As I read it, Yu asserts no new regressions were created with the patch set, not that failing tests were fixed. Unfortunately, the tests listed are largely not ones I saw failing the last time I ran the tests (though it's been a few months since I last tried). I know 01raid6integ used to fail some of the time, but the other 6 tests mentioned worked the last time I ran them; and there are many other tests that failed when I ran them. (My notes on which tests are broken are included in the most recent mdadm tree in tests/*.broken) I was going to try and confirm that no new regressions were introduced by Yu's patches, but seems the tests are getting worse. I tried running the tests on the current md-next branch and found that one of the early tests, 00raid5-zero, hangs indefinitely. I quickly ran the same test on v6.3-rc2 and found that it runs just fine there. So it looks like there's already a regression in md-next that is not part of this series and I don't have the time to dig into the root cause right now. Yu's patches don't apply cleanly to v6.3-rc2 and I can't run the tests against md-next; so I didn't bother running them, but I did do a quick review. The locking changes make sense to me so it might be worth merging for correctness. However, I'm not entirely sure it's the best solution -- the md thread stuff seems like a bit of a mess and passing an mddev to thread functions that were not related to the mddev to get a lock seems to just make the mess a bit worse. For example, it seems a bit ugly to me for the lock mddev->thread_lock to protect the access of a pointer in struct r5l_log. Just spit-balling, but perhaps RCU would be more appropriate here. Then md_wakeup_thread() would just need to hold the RCU read lock when dereferencing, and md_unregister_thread() would just need to synchronize_rcu() before stopping and freeing the thread. This has the benefit of not requiring the mddev object for every md_thread and would probably require a lot less churn than the current patches. Logan
Hi, 在 2023/03/16 6:55, Logan Gunthorpe 写道: > > > On 2023-03-15 02:30, Paul Menzel wrote: >> Am 15.03.23 um 07:18 schrieb Yu Kuai: >>> I tested this pathset with mdadm tests, and there are no new regression, >>> by the way, following test will failed with or without this patchset: >>> >>> 01raid6integ >>> 04r1update >>> 05r6tor0 >>> 10ddf-create >>> 10ddf-fail-spare >>> 10ddf-fail-stop-readd >>> 10ddf-geometry >> >> As you improved the tests in the past, can you confirm, these failed on >> your test systems too and are fixed now? > > Hmm, well Yu did not claim that those tests were fixed. If you re-read > what was said, the tests listed failed with or without the new changes. > As I read it, Yu asserts no new regressions were created with the patch > set, not that failing tests were fixed. > > Unfortunately, the tests listed are largely not ones I saw failing the > last time I ran the tests (though it's been a few months since I last > tried). I know 01raid6integ used to fail some of the time, but the other > 6 tests mentioned worked the last time I ran them; and there are many > other tests that failed when I ran them. (My notes on which tests are > broken are included in the most recent mdadm tree in tests/*.broken) > > I was going to try and confirm that no new regressions were introduced > by Yu's patches, but seems the tests are getting worse. I tried running > the tests on the current md-next branch and found that one of the early > tests, 00raid5-zero, hangs indefinitely. I quickly ran the same test on > v6.3-rc2 and found that it runs just fine there. So it looks like > there's already a regression in md-next that is not part of this series > and I don't have the time to dig into the root cause right now. > > Yu's patches don't apply cleanly to v6.3-rc2 and I can't run the tests > against md-next; so I didn't bother running them, but I did do a quick > review. The locking changes make sense to me so it might be worth > merging for correctness. However, I'm not entirely sure it's the best > solution -- the md thread stuff seems like a bit of a mess and passing > an mddev to thread functions that were not related to the mddev to get a > lock seems to just make the mess a bit worse. > > For example, it seems a bit ugly to me for the lock mddev->thread_lock > to protect the access of a pointer in struct r5l_log. Just spit-balling, > but perhaps RCU would be more appropriate here. Then md_wakeup_thread() > would just need to hold the RCU read lock when dereferencing, and > md_unregister_thread() would just need to synchronize_rcu() before > stopping and freeing the thread. This has the benefit of not requiring > the mddev object for every md_thread and would probably require a lot > less churn than the current patches. Thanks for your suggestion, this make sense to me. I'll try to use rcu. Thanks, Kuai > > Logan > > > > > . >
On Wed, Mar 15, 2023 at 6:26 PM Yu Kuai <yukuai1@huaweicloud.com> wrote: > > Hi, > > 在 2023/03/16 6:55, Logan Gunthorpe 写道: [...] > > I was going to try and confirm that no new regressions were introduced > > by Yu's patches, but seems the tests are getting worse. I tried running > > the tests on the current md-next branch and found that one of the early > > tests, 00raid5-zero, hangs indefinitely. I quickly ran the same test on I am not able to repro the issue with 00raid5-zero. (I did a rebase before running the test, so that might be the reason). > > v6.3-rc2 and found that it runs just fine there. So it looks like > > there's already a regression in md-next that is not part of this series > > and I don't have the time to dig into the root cause right now. > > > > Yu's patches don't apply cleanly to v6.3-rc2 and I can't run the tests > > against md-next; so I didn't bother running them, but I did do a quick > > review. The locking changes make sense to me so it might be worth > > merging for correctness. However, I'm not entirely sure it's the best > > solution -- the md thread stuff seems like a bit of a mess and passing > > an mddev to thread functions that were not related to the mddev to get a > > lock seems to just make the mess a bit worse. > > > > For example, it seems a bit ugly to me for the lock mddev->thread_lock > > to protect the access of a pointer in struct r5l_log. Just spit-balling, > > but perhaps RCU would be more appropriate here. Then md_wakeup_thread() > > would just need to hold the RCU read lock when dereferencing, and > > md_unregister_thread() would just need to synchronize_rcu() before > > stopping and freeing the thread. This has the benefit of not requiring > > the mddev object for every md_thread and would probably require a lot > > less churn than the current patches. > > Thanks for your suggestion, this make sense to me. I'll try to use rcu. Yu Kuai, do you plan to resend the set with Logan suggestions? Thanks, Song
Hi, Song! 在 2023/03/29 7:31, Song Liu 写道: > On Wed, Mar 15, 2023 at 6:26 PM Yu Kuai <yukuai1@huaweicloud.com> wrote: >> >> Hi, >> >> 在 2023/03/16 6:55, Logan Gunthorpe 写道: > [...] >>> I was going to try and confirm that no new regressions were introduced >>> by Yu's patches, but seems the tests are getting worse. I tried running >>> the tests on the current md-next branch and found that one of the early >>> tests, 00raid5-zero, hangs indefinitely. I quickly ran the same test on > > I am not able to repro the issue with 00raid5-zero. (I did a rebase before > running the test, so that might be the reason). > >>> v6.3-rc2 and found that it runs just fine there. So it looks like >>> there's already a regression in md-next that is not part of this series >>> and I don't have the time to dig into the root cause right now. >>> >>> Yu's patches don't apply cleanly to v6.3-rc2 and I can't run the tests >>> against md-next; so I didn't bother running them, but I did do a quick >>> review. The locking changes make sense to me so it might be worth >>> merging for correctness. However, I'm not entirely sure it's the best >>> solution -- the md thread stuff seems like a bit of a mess and passing >>> an mddev to thread functions that were not related to the mddev to get a >>> lock seems to just make the mess a bit worse. >>> >>> For example, it seems a bit ugly to me for the lock mddev->thread_lock >>> to protect the access of a pointer in struct r5l_log. Just spit-balling, >>> but perhaps RCU would be more appropriate here. Then md_wakeup_thread() >>> would just need to hold the RCU read lock when dereferencing, and >>> md_unregister_thread() would just need to synchronize_rcu() before >>> stopping and freeing the thread. This has the benefit of not requiring >>> the mddev object for every md_thread and would probably require a lot >>> less churn than the current patches. >> >> Thanks for your suggestion, this make sense to me. I'll try to use rcu. > > Yu Kuai, do you plan to resend the set with Logan suggestions? Yes, of course, it's just some other problems is triggered while I'm testing the patchset, I'll resend the set once all tests is passed. Thanks, Kuai > > Thanks, > Song > . >