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

[Andrey Ryabinin]

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.

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()?
And how did you get this crash?

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