Re: [PATCH 4.14 095/140] bcache: fix crashes in duplicate cache device register

[Michael Lyle]

Though note you're still not safe from -that-.  If there's duplicate
UUIDs around because you've duplicated devices, there's just no sane
way to tell which is the "right one" to attach to.

Mike

On Tue, Mar 13, 2018 at 9:19 AM, Marc MERLIN <marc@xxxxxxxxxxx> wrote:
> On Tue, Mar 13, 2018 at 04:24:58PM +0100, Greg Kroah-Hartman wrote:
>> 4.14-stable review patch.  If anyone has any objections, please let me know.
>
> Just in case someone is considering whether it's important to merge, the
> bug did crash my kernel of course, but I'm virtually certain it was also
> responsible for corrupting my existing bcache device enough that I had
> to restore it from backup.
>
> Thanks again to Tang for fixing it.
>
>
>> ------------------
>>
>> From: Tang Junhui <tang.junhui@xxxxxxxxxx>
>>
>> commit cc40daf91bdddbba72a4a8cd0860640e06668309 upstream.
>>
>> Kernel crashed when register a duplicate cache device, the call trace is
>> bellow:
>> [  417.643790] CPU: 1 PID: 16886 Comm: bcache-register Tainted: G
>>    W  OE    4.15.5-amd64-preempt-sysrq-20171018 #2
>> [  417.643861] Hardware name: LENOVO 20ERCTO1WW/20ERCTO1WW, BIOS
>> N1DET41W (1.15 ) 12/31/2015
>> [  417.643870] RIP: 0010:bdevname+0x13/0x1e
>> [  417.643876] RSP: 0018:ffffa3aa9138fd38 EFLAGS: 00010282
>> [  417.643884] RAX: 0000000000000000 RBX: ffff8c8f2f2f8000 RCX: ffffd6701f8
>> c7edf
>> [  417.643890] RDX: ffffa3aa9138fd88 RSI: ffffa3aa9138fd88 RDI: 00000000000
>> 00000
>> [  417.643895] RBP: ffffa3aa9138fde0 R08: ffffa3aa9138fae8 R09: 00000000000
>> 1850e
>> [  417.643901] R10: ffff8c8eed34b271 R11: ffff8c8eed34b250 R12: 00000000000
>> 00000
>> [  417.643906] R13: ffffd6701f78f940 R14: ffff8c8f38f80000 R15: ffff8c8ea7d
>> 90000
>> [  417.643913] FS:  00007fde7e66f500(0000) GS:ffff8c8f61440000(0000) knlGS:
>> 0000000000000000
>> [  417.643919] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> [  417.643925] CR2: 0000000000000314 CR3: 00000007e6fa0001 CR4: 00000000003
>> 606e0
>> [  417.643931] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 00000000000
>> 00000
>> [  417.643938] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 00000000000
>> 00400
>> [  417.643946] Call Trace:
>> [  417.643978]  register_bcache+0x1117/0x1270 [bcache]
>> [  417.643994]  ? slab_pre_alloc_hook+0x15/0x3c
>> [  417.644001]  ? slab_post_alloc_hook.isra.44+0xa/0x1a
>> [  417.644013]  ? kernfs_fop_write+0xf6/0x138
>> [  417.644020]  kernfs_fop_write+0xf6/0x138
>> [  417.644031]  __vfs_write+0x31/0xcc
>> [  417.644043]  ? current_kernel_time64+0x10/0x36
>> [  417.644115]  ? __audit_syscall_entry+0xbf/0xe3
>> [  417.644124]  vfs_write+0xa5/0xe2
>> [  417.644133]  SyS_write+0x5c/0x9f
>> [  417.644144]  do_syscall_64+0x72/0x81
>> [  417.644161]  entry_SYSCALL_64_after_hwframe+0x3d/0xa2
>> [  417.644169] RIP: 0033:0x7fde7e1c1974
>> [  417.644175] RSP: 002b:00007fff13009a38 EFLAGS: 00000246 ORIG_RAX: 0000000
>> 000000001
>> [  417.644183] RAX: ffffffffffffffda RBX: 0000000001658280 RCX: 00007fde7e1c
>> 1974
>> [  417.644188] RDX: 000000000000000a RSI: 0000000001658280 RDI: 000000000000
>> 0001
>> [  417.644193] RBP: 000000000000000a R08: 0000000000000003 R09: 000000000000
>> 0077
>> [  417.644198] R10: 000000000000089e R11: 0000000000000246 R12: 000000000000
>> 0001
>> [  417.644203] R13: 000000000000000a R14: 7fffffffffffffff R15: 000000000000
>> 0000
>> [  417.644213] Code: c7 c2 83 6f ee 98 be 20 00 00 00 48 89 df e8 6c 27 3b 0
>> 0 48 89 d8 5b c3 0f 1f 44 00 00 48 8b 47 70 48 89 f2 48 8b bf 80 00 00 00 <8
>> b> b0 14 03 00 00 e9 73 ff ff ff 0f 1f 44 00 00 48 8b 47 40 39
>> [  417.644302] RIP: bdevname+0x13/0x1e RSP: ffffa3aa9138fd38
>> [  417.644306] CR2: 0000000000000314
>>
>> When registering duplicate cache device in register_cache(), after failure
>> on calling register_cache_set(), bch_cache_release() will be called, then
>> bdev will be freed, so bdevname(bdev, name) caused kernel crash.
>>
>> Since bch_cache_release() will free bdev, so in this patch we make sure
>> bdev being freed if register_cache() fail, and do not free bdev again in
>> register_bcache() when register_cache() fail.
>>
>> Signed-off-by: Tang Junhui <tang.junhui@xxxxxxxxxx>
>> Reported-by: Marc MERLIN <marc@xxxxxxxxxxx>
>> Tested-by: Michael Lyle <mlyle@xxxxxxxx>
>> Reviewed-by: Michael Lyle <mlyle@xxxxxxxx>
>> Cc: <stable@xxxxxxxxxxxxxxx>
>> Signed-off-by: Jens Axboe <axboe@xxxxxxxxx>
>> Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
>>
>> ---
>>  drivers/md/bcache/super.c |   16 ++++++++++------
>>  1 file changed, 10 insertions(+), 6 deletions(-)
>>
>> --- a/drivers/md/bcache/super.c
>> +++ b/drivers/md/bcache/super.c
>> @@ -1181,7 +1181,7 @@ static void register_bdev(struct cache_s
>>
>>       return;
>>  err:
>> -     pr_notice("error opening %s: %s", bdevname(bdev, name), err);
>> +     pr_notice("error %s: %s", bdevname(bdev, name), err);
>>       bcache_device_stop(&dc->disk);
>>  }
>>
>> @@ -1849,6 +1849,8 @@ static int register_cache(struct cache_s
>>       const char *err = NULL; /* must be set for any error case */
>>       int ret = 0;
>>
>> +     bdevname(bdev, name);
>> +
>>       memcpy(&ca->sb, sb, sizeof(struct cache_sb));
>>       ca->bdev = bdev;
>>       ca->bdev->bd_holder = ca;
>> @@ -1857,11 +1859,12 @@ static int register_cache(struct cache_s
>>       ca->sb_bio.bi_io_vec[0].bv_page = sb_page;
>>       get_page(sb_page);
>>
>> -     if (blk_queue_discard(bdev_get_queue(ca->bdev)))
>> +     if (blk_queue_discard(bdev_get_queue(bdev)))
>>               ca->discard = CACHE_DISCARD(&ca->sb);
>>
>>       ret = cache_alloc(ca);
>>       if (ret != 0) {
>> +             blkdev_put(bdev, FMODE_READ|FMODE_WRITE|FMODE_EXCL);
>>               if (ret == -ENOMEM)
>>                       err = "cache_alloc(): -ENOMEM";
>>               else
>> @@ -1884,14 +1887,14 @@ static int register_cache(struct cache_s
>>               goto out;
>>       }
>>
>> -     pr_info("registered cache device %s", bdevname(bdev, name));
>> +     pr_info("registered cache device %s", name);
>>
>>  out:
>>       kobject_put(&ca->kobj);
>>
>>  err:
>>       if (err)
>> -             pr_notice("error opening %s: %s", bdevname(bdev, name), err);
>> +             pr_notice("error %s: %s", name, err);
>>
>>       return ret;
>>  }
>> @@ -1980,6 +1983,7 @@ static ssize_t register_bcache(struct ko
>>       if (err)
>>               goto err_close;
>>
>> +     err = "failed to register device";
>>       if (SB_IS_BDEV(sb)) {
>>               struct cached_dev *dc = kzalloc(sizeof(*dc), GFP_KERNEL);
>>               if (!dc)
>> @@ -1994,7 +1998,7 @@ static ssize_t register_bcache(struct ko
>>                       goto err_close;
>>
>>               if (register_cache(sb, sb_page, bdev, ca) != 0)
>> -                     goto err_close;
>> +                     goto err;
>>       }
>>  out:
>>       if (sb_page)
>> @@ -2007,7 +2011,7 @@ out:
>>  err_close:
>>       blkdev_put(bdev, FMODE_READ|FMODE_WRITE|FMODE_EXCL);
>>  err:
>> -     pr_info("error opening %s: %s", path, err);
>> +     pr_info("error %s: %s", path, err);
>>       ret = -EINVAL;
>>       goto out;
>>  }
>>
>>
>>
>
> --
> "A mouse is a device used to point at the xterm you want to type in" - A.S.R.
> Microsoft is to operating systems ....
>                                       .... what McDonalds is to gourmet cooking
> Home page: http://marc.merlins.org/                       | PGP 7F55D5F27AAF9D08