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

[Yu Kuai <yukuai1@xxxxxxxxxxxxxxx>]

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