Re: [PATCH 06/19] bcache: explicitly destory mutex while exiting

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

 



On 2017/7/1 上午4:42, bcache@xxxxxxxxxxxxxxxxxx wrote:
> From: Liang Chen <liangchen.linux@xxxxxxxxx>
> 
> mutex_destroy does nothing most of time, but it's better to call
> it to make the code future proof and it also has some meaning
> for like mutex debug.
> 
> Signed-off-by: Liang Chen <liangchen.linux@xxxxxxxxx>
> Reviewed-by: Eric Wheeler <bcache@xxxxxxxxxxxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx
> ---
>  drivers/md/bcache/super.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
> index 48b8c20..1f84791 100644
> --- a/drivers/md/bcache/super.c
> +++ b/drivers/md/bcache/super.c
> @@ -2089,6 +2089,7 @@ static void bcache_exit(void)
>  	if (bcache_major)
>  		unregister_blkdev(bcache_major, "bcache");
>  	unregister_reboot_notifier(&reboot);
> +	mutex_destroy(&bch_register_lock>  }
>  
>  static int __init bcache_init(void)
> @@ -2106,6 +2107,7 @@ static int __init bcache_init(void)
>  
>  	bcache_major = register_blkdev(0, "bcache");
>  	if (bcache_major < 0) {
> +		mutex_destroy(&bch_register_lock);
>  		unregister_reboot_notifier(&reboot);
>  		return bcache_major;
>  	}
> 

Hi Liang,

Current code might have a potential race in a very corner case, see,
2084 static int __init bcache_init(void)
2085 {
2086         static const struct attribute *files[] = {
2087                 &ksysfs_register.attr,
2088                 &ksysfs_register_quiet.attr,
2089                 NULL
2090         };
2091
2092         mutex_init(&bch_register_lock);
2093         init_waitqueue_head(&unregister_wait);
2094         register_reboot_notifier(&reboot);
2095         closure_debug_init();
2096
2097         bcache_major = register_blkdev(0, "bcache");
2098         if (bcache_major < 0) {
2099                 unregister_reboot_notifier(&reboot);
2100                 return bcache_major;
2101         }
2102
2103         if (!(bcache_wq = alloc_workqueue("bcache", WQ_MEM_RECLAIM,
0)) ||
2104             !(bcache_kobj = kobject_create_and_add("bcache",
fs_kobj)) ||
2105             sysfs_create_files(bcache_kobj, files) ||
2106             bch_request_init() ||
2107             bch_debug_init(bcache_kobj))
2108                 goto err;
2109
2110         return 0;
2111 err:
2112         bcache_exit();
2113         return -ENOMEM;
2114 }

At line 2107, most of bache stuffs are ready to work, only a debugfs
entry not created yet. If in the time gap between line 2106 and line
2017, another user space tool just registers cache and backing devices.
Then bch_debug_init() failed, and bcache_exit() gets called. In this
case, I doubt bcache_exit() can handle all the references correctly.

The race is very rare, and almost won't happen in real life. So, if you
don't care about it, the patch can be simpler like this,
diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
index e57353e39168..fb5453a46a03 100644
--- a/drivers/md/bcache/super.c
+++ b/drivers/md/bcache/super.c
@@ -2070,6 +2070,7 @@ static struct notifier_block reboot = {

 static void bcache_exit(void)
 {
+       mutex_destroy(&bch_register_lock);
        bch_debug_exit();
        bch_request_exit();
        if (bcache_kobj)
@@ -2089,7 +2090,6 @@ static int __init bcache_init(void)
                NULL
        };

-       mutex_init(&bch_register_lock);
        init_waitqueue_head(&unregister_wait);
        register_reboot_notifier(&reboot);
        closure_debug_init();
@@ -2107,6 +2107,7 @@ static int __init bcache_init(void)
            bch_debug_init(bcache_kobj))
                goto err;

+       mutex_init(&bch_register_lock);
        return 0;
 err:
        bcache_exit();
---
And if you do care about the race, maybe you should do something like this,
diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
index e57353e39168..ca1d8b7a7815 100644
--- a/drivers/md/bcache/super.c
+++ b/drivers/md/bcache/super.c
@@ -2079,6 +2079,7 @@ static void bcache_exit(void)
        if (bcache_major)
                unregister_blkdev(bcache_major, "bcache");
        unregister_reboot_notifier(&reboot);
+       mutex_unlock(&bch_register_lock);
 }

 static int __init bcache_init(void)
@@ -2090,6 +2091,7 @@ static int __init bcache_init(void)
        };

        mutex_init(&bch_register_lock);
+       mutex_lock(&bch_register_lock);
        init_waitqueue_head(&unregister_wait);
        register_reboot_notifier(&reboot);
        closure_debug_init();
@@ -2097,6 +2099,8 @@ static int __init bcache_init(void)
        bcache_major = register_blkdev(0, "bcache");
        if (bcache_major < 0) {
                unregister_reboot_notifier(&reboot);
+               mutex_unlock(&bch_register_lock);
+               mutex_destroy(&bch_register_lock);
                return bcache_major;
        }

@@ -2104,9 +2108,12 @@ static int __init bcache_init(void)
            !(bcache_kobj = kobject_create_and_add("bcache", fs_kobj)) ||
            sysfs_create_files(bcache_kobj, files) ||
            bch_request_init() ||
-           bch_debug_init(bcache_kobj))
+           bch_debug_init(bcache_kobj)) {
+               mutex_unlock(&bch_register_lock);
                goto err;
+       }

+       mutex_unlock(&bch_register_lock);
        return 0;
 err:
        bcache_exit();
---

Personally I think the first approach with only one new line code added,
your original version will add two new lines of code.

Just FYI. Thanks.

-- 
Coly Li



[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]