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

[Andrey Ryabinin]

On 08/19/2016 02:40 AM, Maxim Patlasov wrote:
> 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.
> 

Disagreed. This would mean that all callers supposed to synchronize freeze_bdev()/thaw_bdev() sequence.
Otherwise we can get:

CPU 0:					CPU1:
	freeze_bdev() //return NULL
					freeze_bdev() //NULL-ptr deref
					thaw_bdev()
	thaw_bdev()

So, how do you propose to fix that case? If freeze_bdev() on CPU1 is illegal, that would mean that every freeze_bdev()...thaw_bdev() should
be in critical section, iow surrounded by some per-bdev lock. While comment near freeze_bdev() says that it totally ok if multiple freeze
request arrive simultaneously.

Note, that this patch is also still affected by this bug, e.g.:
	1) cryptsetup open /dev/ploop0p1 test
	2) freeze ploop
	3) dmsetup suspend test //suspend calls lock_fs()->freeze_bdev() -> NULL ptr deref


So the only sane way to fix this is to add NULL check before drop_super().