Re: [PATCH -next v6 5/5] md: protect md_thread with rcu

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

在 2023/04/13 23:10, Logan Gunthorpe 写道:


On 2023-04-13 05:32, Yu Kuai wrote:
From: Yu Kuai <yukuai3@xxxxxxxxxx>

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@xxxxxxxxxx>
---
  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

--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/dm-devel




[Index of Archives]     [DM Crypt]     [Fedora Desktop]     [ATA RAID]     [Fedora Marketing]     [Fedora Packaging]     [Fedora SELinux]     [Yosemite Discussion]     [KDE Users]     [Fedora Docs]

  Powered by Linux