dm-path-selector: fix refcount corruption

Commit Message

Junichi Nomura Feb. 5, 2009, 12:51 p.m. UTC

Hi,

Refcounting of path-selector module is not safe in SMP environment.
The counter may corrupt and trigger BUG() like this:
  kernel BUG at linux-2.6.29-rc3/drivers/md/dm-path-selector.c:90!
though it's rare under normal usage.

The bug is here:
  void dm_put_path_selector(struct path_selector_type *pst)
  {
  ...
        down_read(&_ps_lock);
        psi = __find_path_selector_type(pst->name);
        if (!psi)
                goto out;

        if (--psi->use == 0)
                module_put(psi->pst.module);

        BUG_ON(psi->use < 0);

The code manipulates the counter without exclusive lock or atomic ops.
So if 2 processors come in, the counter may corrupt.

While it could be fixed using atomic ops for the counter manipulation,
we can just drop the 'use' counter like Cheng Renquan did for dm-target:
https://www.redhat.com/archives/dm-devel/2008-December/msg00075.html

(Actually, without his patch, dm-target.c hits the same problem.)

This is a simple reproducer. Change "dev" for your environment.
(In my experiment, it used to take hours to reproduce the problem.)
-------------------------------------------------------------------
#!/bin/sh

dev=/dev/sda11
tab1="0 100 multipath 0 0 1 1 round-robin 0 1 1 $dev 10"
tab2="0 100 multipath 0 0 1 1 round-robin 0 1 1 $dev 20"

function runtest() {
  local map=$1

  echo $tab1 | dmsetup create $map
  while true; do
    echo $tab2 | dmsetup load $map
    dmsetup resume $map
    echo $tab1 | dmsetup load $map
    dmsetup resume $map
  done
}

runtest m1 &
runtest m1 &
-------------------------------------------------------------------

Comments

Jonthan Brassow Feb. 5, 2009, 9:48 p.m. UTC | #1

On Feb 5, 2009, at 6:51 AM, Jun'ichi Nomura wrote:

> @@ -136,11 +126,6 @@ int dm_unregister_path_selector(struct p
> 		return -EINVAL;
> 	}
>
> -	if (psi->use) {
> -		up_write(&_ps_lock);
> -		return -ETXTBSY;
> -	}
> -
> 	list_del(&psi->list);


We still need this in some form, don't we?  Like 'if  
(module_refcount...'?

  brassow

--
dm-devel mailing list
[e-mail]
https://www.redhat.com/mailman/listinfo/dm-devel

Junichi Nomura Feb. 6, 2009, 1:04 a.m. UTC | #2

Hi Jonathan,

Jonathan Brassow wrote:
> On Feb 5, 2009, at 6:51 AM, Jun'ichi Nomura wrote:
> 
>> @@ -136,11 +126,6 @@ int dm_unregister_path_selector(struct p
>>         return -EINVAL;
>>     }
>>
>> -    if (psi->use) {
>> -        up_write(&_ps_lock);
>> -        return -ETXTBSY;
>> -    }
>> -
>>     list_del(&psi->list);
> 
> 
> We still need this in some form, don't we?  Like 'if (module_refcount...'?

I don't think so.
dm_unregister_path_selector() is called from module_exit function. So it is called when the refcount is 0.

Thanks,

Patch

Index: linux-2.6.29-rc2/drivers/md/dm-path-selector.c
===================================================================
--- linux-2.6.29-rc2.orig/drivers/md/dm-path-selector.c
+++ linux-2.6.29-rc2/drivers/md/dm-path-selector.c
@@ -17,9 +17,7 @@ 
 
 struct ps_internal {
 	struct path_selector_type pst;
-
 	struct list_head list;
-	long use;
 };
 
 #define pst_to_psi(__pst) container_of((__pst), struct ps_internal, pst)
@@ -45,12 +43,8 @@  static struct ps_internal *get_path_sele
 
 	down_read(&_ps_lock);
 	psi = __find_path_selector_type(name);
-	if (psi) {
-		if ((psi->use == 0) && !try_module_get(psi->pst.module))
-			psi = NULL;
-		else
-			psi->use++;
-	}
+	if (psi && !try_module_get(psi->pst.module))
+		psi = NULL;
 	up_read(&_ps_lock);
 
 	return psi;
@@ -84,11 +78,7 @@  void dm_put_path_selector(struct path_se
 	if (!psi)
 		goto out;
 
-	if (--psi->use == 0)
-		module_put(psi->pst.module);
-
-	BUG_ON(psi->use < 0);
-
+	module_put(psi->pst.module);
 out:
 	up_read(&_ps_lock);
 }
@@ -136,11 +126,6 @@  int dm_unregister_path_selector(struct p
 		return -EINVAL;
 	}
 
-	if (psi->use) {
-		up_write(&_ps_lock);
-		return -ETXTBSY;
-	}
-
 	list_del(&psi->list);
 
 	up_write(&_ps_lock);