[v3,3/3] md: protect md_thread with rcu

  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 might
be similar problem for other md_thread, and I really don't like the idea to
borrow a global lock.

This patch protect md_thread with rcu.

Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 drivers/md/md.c | 32 ++++++++++----------------------
 1 file changed, 10 insertions(+), 22 deletions(-)
  

Hi Yu,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on song-md/md-next]
[also build test WARNING on device-mapper-dm/for-next linus/master v6.3-rc4 next-20230330]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Yu-Kuai/md-pass-a-md_thread-pointer-to-md_register_thread/20230330-202251
base:   git://git.kernel.org/pub/scm/linux/kernel/git/song/md.git md-next
patch link:    https://lore.kernel.org/r/20230330202046.795213-4-yukuai1%40huaweicloud.com
patch subject: [PATCH v3 3/3] md: protect md_thread with rcu
config: x86_64-randconfig-s023 (https://download.01.org/0day-ci/archive/20230331/202303310252.5OJzxk7h-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.4-39-gce1a6720-dirty
        # https://github.com/intel-lab-lkp/linux/commit/b6edd339e54dc14576816f285b99e0e1815ed1e0
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Yu-Kuai/md-pass-a-md_thread-pointer-to-md_register_thread/20230330-202251
        git checkout b6edd339e54dc14576816f285b99e0e1815ed1e0
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=x86_64 olddefconfig
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/md/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303310252.5OJzxk7h-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> drivers/md/md.c:7905:18: sparse: sparse: incompatible types in comparison expression (different address spaces):
>> drivers/md/md.c:7905:18: sparse:    struct md_thread [noderef] __rcu *
>> drivers/md/md.c:7905:18: sparse:    struct md_thread *
   drivers/md/md.c:7939:9: sparse: sparse: incompatible types in comparison expression (different address spaces):
   drivers/md/md.c:7939:9: sparse:    struct md_thread [noderef] __rcu *
   drivers/md/md.c:7939:9: sparse:    struct md_thread *
   drivers/md/md.c:7952:9: sparse: sparse: incompatible types in comparison expression (different address spaces):
   drivers/md/md.c:7952:9: sparse:    struct md_thread [noderef] __rcu *
   drivers/md/md.c:7952:9: sparse:    struct md_thread *

vim +7905 drivers/md/md.c

  7899	
  7900	void md_wakeup_thread(struct md_thread **threadp)
  7901	{
  7902		struct md_thread *thread;
  7903	
  7904		rcu_read_lock();
> 7905		thread = rcu_dereference(*threadp);
  7906		if (thread) {
  7907			pr_debug("md: waking up MD thread %s.\n", thread->tsk->comm);
  7908			set_bit(THREAD_WAKEUP, &thread->flags);
  7909			wake_up(&thread->wqueue);
  7910		}
  7911		rcu_read_unlock();
  7912	}
  7913	EXPORT_SYMBOL(md_wakeup_thread);
  7914
  

On 2023-03-30 14:20, 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 might
> be similar problem for other md_thread, and I really don't like the idea to
> borrow a global lock.
> 
> This patch protect md_thread with rcu.
> 
> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
> ---
>  drivers/md/md.c | 32 ++++++++++----------------------
>  1 file changed, 10 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 9e80c5491c9a..161231e01faa 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -70,11 +70,7 @@
>  #include "md-bitmap.h"
>  #include "md-cluster.h"
>  
> -/* pers_list is a list of registered personalities protected
> - * by pers_lock.
> - * pers_lock does extra service to protect accesses to
> - * mddev->thread when the mutex cannot be held.
> - */
> +/* pers_list is a list of registered personalities protected by pers_lock. */
>  static LIST_HEAD(pers_list);
>  static DEFINE_SPINLOCK(pers_lock);
>  
> @@ -802,13 +798,8 @@ void mddev_unlock(struct mddev *mddev)
>  	} else
>  		mutex_unlock(&mddev->reconfig_mutex);
>  
> -	/* As we've dropped the mutex we need a spinlock to
> -	 * make sure the thread doesn't disappear
> -	 */
> -	spin_lock(&pers_lock);
>  	md_wakeup_thread(&mddev->thread);
>  	wake_up(&mddev->sb_wait);
> -	spin_unlock(&pers_lock);
>  }
>  EXPORT_SYMBOL_GPL(mddev_unlock);
>  
> @@ -7921,13 +7912,16 @@ static int md_thread(void *arg)
>  
>  void md_wakeup_thread(struct md_thread **threadp)
>  {
> -	struct md_thread *thread = *threadp;
> +	struct md_thread *thread;
>  
> +	rcu_read_lock();
> +	thread = rcu_dereference(*threadp);

A couple points:

I don't think we need a double pointer here. rcu_dereference() doesn't
actually do anything but annotate the fact that we are accessing a
pointer protected by rcu. It does require annotations on that pointer
(__rcu) which is checked by sparse (I suspect this patch will produce a
lot of sparse errors from kbuild bot).

I think all we need is:

void md_wakeup_thread(struct md_thread __rcu *rthread)
{
	struct md_thread *thread;
   	
	rcu_read_lock();
	thread = rcu_dereference(rthread);
	...
	rcu_read_unlock();
	
}

The __rcu annotation will have to be added to all the pointers this
function is called on as well as to md_register_thread() and
md_unregister_thread(). And anything else that uses those pointers.
Running sparse on the code and eliminating all new errors for the patch
is important.

Thanks,

Logan
  

Hi, Logan!

在 2023/03/31 3:35, Logan Gunthorpe 写道:
> 
> A couple points:
> 
> I don't think we need a double pointer here. rcu_dereference() doesn't
> actually do anything but annotate the fact that we are accessing a
> pointer protected by rcu. It does require annotations on that pointer
> (__rcu) which is checked by sparse (I suspect this patch will produce a
> lot of sparse errors from kbuild bot).
> 
> I think all we need is:
> 
> void md_wakeup_thread(struct md_thread __rcu *rthread)
> {
> 	struct md_thread *thread;
>     	
> 	rcu_read_lock();
> 	thread = rcu_dereference(rthread);
> 	...
> 	rcu_read_unlock();
> 	
> }
> 
> The __rcu annotation will have to be added to all the pointers this
> function is called on as well as to md_register_thread() and
> md_unregister_thread(). And anything else that uses those pointers.
> Running sparse on the code and eliminating all new errors for the patch
> is important.

Yes, you're right, I'll remove patch 2 and update patch 3. And I'll try
to run sparse before sending the new version.

Thanks,
Kuai
  

Hi,

在 2023/03/31 9:08, Yu Kuai 写道:
> Hi, Logan!
> 
> 在 2023/03/31 3:35, Logan Gunthorpe 写道:
>>
>> A couple points:
>>
>> I don't think we need a double pointer here. rcu_dereference() doesn't
>> actually do anything but annotate the fact that we are accessing a
>> pointer protected by rcu. It does require annotations on that pointer
>> (__rcu) which is checked by sparse (I suspect this patch will produce a
>> lot of sparse errors from kbuild bot).
>>
>> I think all we need is:
>>
>> void md_wakeup_thread(struct md_thread __rcu *rthread)
>> {
>>     struct md_thread *thread;
>>
>>     rcu_read_lock();
>>     thread = rcu_dereference(rthread);
>>     ...
>>     rcu_read_unlock();
>>
>> }
>>
>> The __rcu annotation will have to be added to all the pointers this
>> function is called on as well as to md_register_thread() and
>> md_unregister_thread(). And anything else that uses those pointers.
>> Running sparse on the code and eliminating all new errors for the patch
>> is important.
> 
> Yes, you're right, I'll remove patch 2 and update patch 3. And I'll try
> to run sparse before sending the new version.
> 

By the way, I observed lots of sparse errors and warnings for current
code, will it make sense to fix them?

Thanks,
Kuai
  

On 2023-03-31 00:36, Yu Kuai wrote:

>> Yes, you're right, I'll remove patch 2 and update patch 3. And I'll try
>> to run sparse before sending the new version.
>>
> 
> By the way, I observed lots of sparse errors and warnings for current
> code, will it make sense to fix them?

Yup, I fixed a bunch for raid5 last year. There was one I could not fix
though. I would say effort spent fixing those is well spent.

Thanks!

Logan