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

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

 



Hi Coly,
Thanks for reviewing the patch! You raised a good point about the race. I also
think it should be addressed. Even though the time window is small, it will
still happen sooner or later.

I would like to keep this "destory mutex" patch unchanged, and send another
patch to fix the issue based on your approach. Please take a look. Thanks!

Thanks,
Liang

On Sun, Jul 2, 2017 at 2:43 AM, Coly Li <i@xxxxxxx> wrote:
> 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 RAID]     [Linux SCSI]     [Linux ATA RAID]     [IDE]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Device Mapper]

  Powered by Linux