[Devel] [PATCH rh7] ploop: add support for dm-crypted ploops

[Maxim Patlasov]

On 08/18/2016 09:49 AM, Andrey Ryabinin wrote:

>
> On 08/18/2016 04:21 AM, Maxim Patlasov wrote:
>> Andrey,
>>
>>
>> ploop_freeze() must use __ploop_get_dm_crypt_bdev(), not ploop_get_dm_crypt_bdev()!
>>
> YUp.
>
>> Another problem is that the patch does not ensure that plo->dm_crypt_bdev stays the same between PLOOP_IOC_FREEZE/THAW. I could easily get:
>>
>> [  636.600190] BUG: unable to handle kernel NULL pointer dereference at 0000000000000068
>> [  636.600934] IP: [<ffffffff810ac5d3>] up_read+0x13/0x30
>> [  636.601446] PGD 222f5a067 PUD 224bc6067 PMD 0
>> [  636.601915] Oops: 0002 [#1] SMP
>> ...
>>
>> [  636.623264] Call Trace:
>> [  636.623597]  [<ffffffff811fd346>] drop_super+0x16/0x30
>> [  636.624104]  [<ffffffff81237010>] freeze_bdev+0x60/0xe0
>> [  636.624627]  [<ffffffffa0541a3e>] ploop_ioctl+0x2b8e/0x2c80 [ploop]
>> [  636.625237]  [<ffffffff811acb44>] ? handle_mm_fault+0x5b4/0xf50
>> [  636.625822]  [<ffffffff812d41ef>] blkdev_ioctl+0x2df/0x770
>> [  636.626439]  [<ffffffff812365d1>] block_ioctl+0x41/0x50
>> [  636.627162]  [<ffffffff8120d515>] do_vfs_ioctl+0x255/0x4f0
>> [  636.627827]  [<ffffffff8120d804>] SyS_ioctl+0x54/0xa0
>>
>> due to this problem.
> I don't see direct relation between this crash and changing plo->dm_crypt_bdev in between freeze/thaw.

In ideal world freeze+thaw+freeze+thaw must change bd_fsfreeze_count 
like this: 0 --> 1 --> 0 --> 1 --> 0. But if plo->dm_crypt_bdev changes 
in between, the first increment may be applied to one bdev, then 
decrement to another bdev, then the second increment again to the first 
bdev.

>
> The way I read this
>
> struct super_block *freeze_bdev(struct block_device *bdev)
> ...
> 	if (++bdev->bd_fsfreeze_count > 1) {
>
> 		sb = get_super(bdev);  // this returned NULL, so we don't have active super block on this device.
> 		drop_super(sb); // NULL ptr deref
> 		mutex_unlock(&bdev->bd_fsfreeze_mutex);
> 		return sb;
> 	
>
> AFAIU, this can happen if we call freeze_bdev() twice on block device which doesn't have mounted fs.
> Isn't this a bug in freeze_bdev()?

No. The bug is to call freeze_bdev second time if the first time it 
returned sb == NULL.

> And how did you get this crash?

1) freeze (increments ploop bdev counter)
2) cryptsetup luksOpen
3) thaw (do nothing)
4) cryptsetup luksClose
5) freeze (attempts to increment ploop bdev counter again)

>
>> Similar might exist in ploop_snapshot(): you bdput plo->dm_crypt_bdev in find_and_freeze_bdev(), so it can disappear by the time we need it for thaw_bdev after calling complete_snapshot.
>>
>>
>> Btw, it's usually good idea to give a patch some simple testing prior sending it for review ;)
>>
> http://devopsreactions.tumblr.com/post/88260308392/testing-my-own-code

:)

>
>> Thanks,
>>
>> Maxim
>>
>>
>>