lkml.org 
[lkml]   [2023]   [Apr]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
SubjectRe: [PATCH -next v6 5/5] md: protect md_thread with rcu
From
Date
Hi,

在 2023/04/13 23:10, Logan Gunthorpe 写道:
>
>
> On 2023-04-13 05:32, Yu Kuai wrote:
>> From: Yu Kuai <yukuai3@huawei.com>
>>
>> Our test reports a uaf for 'mddev->sync_thread':
>>
>> T1 T2
>> md_start_sync
>> md_register_thread
>> // mddev->sync_thread is set
>> raid1d
>> md_check_recovery
>> md_reap_sync_thread
>> md_unregister_thread
>> kfree
>>
>> md_wakeup_thread
>> wake_up
>> ->sync_thread was freed
>>
>> Root cause is that there is a small windown between register thread and
>> wake up thread, where the thread can be freed concurrently.
>>
>> Currently, a global spinlock 'pers_lock' is borrowed to protect
>> 'mddev->thread', this problem can be fixed likewise, however, there are
>> similar problems elsewhere, and use a global lock for all the cases is
>> not good.
>>
>> This patch protect all md_thread with rcu.
>>
>> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
>> ---
>> drivers/md/md-bitmap.c | 10 ++++--
>> drivers/md/md-cluster.c | 7 ++--
>> drivers/md/md-multipath.c | 4 +--
>> drivers/md/md.c | 69 ++++++++++++++++++---------------------
>> drivers/md/md.h | 8 ++---
>> drivers/md/raid1.c | 7 ++--
>> drivers/md/raid1.h | 2 +-
>> drivers/md/raid10.c | 21 +++++++-----
>> drivers/md/raid10.h | 2 +-
>> drivers/md/raid5-cache.c | 22 ++++++++-----
>> drivers/md/raid5.c | 15 +++++----
>> drivers/md/raid5.h | 2 +-
>> 12 files changed, 91 insertions(+), 78 deletions(-)
>>
>> diff --git a/drivers/md/md-bitmap.c b/drivers/md/md-bitmap.c
>> index 29fd41ef55a6..ab27f66dbb1f 100644
>> --- a/drivers/md/md-bitmap.c
>> +++ b/drivers/md/md-bitmap.c
>> @@ -1221,13 +1221,19 @@ static bitmap_counter_t *md_bitmap_get_counter(struct bitmap_counts *bitmap,
>> static void mddev_set_timeout(struct mddev *mddev, unsigned long timeout,
>> bool force)
>> {
>> - struct md_thread *thread = mddev->thread;
>> + struct md_thread *thread;
>> +
>> + rcu_read_lock();
>> + thread = rcu_dereference(mddev->thread);
>>
>> if (!thread)
>> - return;
>> + goto out;
>>
>> if (force || thread->timeout < MAX_SCHEDULE_TIMEOUT)
>> thread->timeout = timeout;
>> +
>> +out:
>> + rcu_read_unlock();
>> }
>>
>> /*
>> diff --git a/drivers/md/md-cluster.c b/drivers/md/md-cluster.c
>> index 10e0c5381d01..672dfa6a40ec 100644
>> --- a/drivers/md/md-cluster.c
>> +++ b/drivers/md/md-cluster.c
>> @@ -362,8 +362,8 @@ static void __recover_slot(struct mddev *mddev, int slot)
>>
>> set_bit(slot, &cinfo->recovery_map);
>> if (!cinfo->recovery_thread) {
>> - cinfo->recovery_thread = md_register_thread(recover_bitmaps,
>> - mddev, "recover");
>> + rcu_assign_pointer(cinfo->recovery_thread,
>> + md_register_thread(recover_bitmaps, mddev, "recover"));
>> if (!cinfo->recovery_thread) {
>> pr_warn("md-cluster: Could not create recovery thread\n");
>> return;
>> @@ -889,7 +889,8 @@ static int join(struct mddev *mddev, int nodes)
>> }
>> /* Initiate the communication resources */
>> ret = -ENOMEM;
>> - cinfo->recv_thread = md_register_thread(recv_daemon, mddev, "cluster_recv");
>> + rcu_assign_pointer(cinfo->recv_thread,
>> + md_register_thread(recv_daemon, mddev, "cluster_recv"));
>> if (!cinfo->recv_thread) {
>
> This looks like it'll hit sparse warnings. without an
> rcu_access_pointer(). Might be nicer to use a temporary variable, check
> that it's not null, then call rcu_assign_pointer().

Sorry this is because I missed following change:

diff --git a/drivers/md/md-cluster.c b/drivers/md/md-cluster.c
index 672dfa6a40ec..2f64673b6ed2 100644
--- a/drivers/md/md-cluster.c
+++ b/drivers/md/md-cluster.c
@@ -75,14 +75,14 @@ struct md_cluster_info {
sector_t suspend_hi;
int suspend_from; /* the slot which broadcast suspend_lo/hi */

- struct md_thread *recovery_thread;
+ struct md_thread __rcu *recovery_thread;
unsigned long recovery_map;
/* communication loc resources */
struct dlm_lock_resource *ack_lockres;
struct dlm_lock_resource *message_lockres;
struct dlm_lock_resource *token_lockres;
struct dlm_lock_resource *no_new_dev_lockres;
- struct md_thread *recv_thread;
+ struct md_thread __rcu *recv_thread;
struct completion newdisk_completion;
wait_queue_head_t wait;
unsigned long state;
Thanks,
Kuai

\
 
 \ /
  Last update: 2023-04-14 03:27    [W:0.060 / U:3.600 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site