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

[Maxim Patlasov]

Andrey,


On 08/19/2016 03:44 AM, Andrey Ryabinin wrote:

>
> 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.

Makes sense. Will you send the patch upstream?

>
> 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().

"freeze ploop" might "thaw" immediately if !sb. Yeah, this is racy, but 
the chances of such a race are negligibly slim. On the other hand, your 
fix for freeze_bdev() is so simple and straightforward, that I'd vote 
for that (even if the fix is rejected upstream for some reasons).
Thanks,
Maxim