device mapper and the BLKFLSBUF ioctl

Hi

I found a bug in dm regarding the BLKFLSBUF ioctl.

The BLKFLSBUF ioctl can be called on a block device and it flushes the buffer cache. There is one exception - when it is called on ramdisk, it actually destroys all ramdisk data (it works like a discard on the full device).

The device mapper passes this ioctl down to the underlying device, so when the ioctl is called on a logical volume, it can be used to destroy the underlying volume group if the volume group is on ramdisk.

For example:

# modprobe brd rd_size=1048576
# pvcreate /dev/ram0
# vgcreate ram_vg /dev/ram0
# lvcreate -L 16M -n ram_lv ram_vg
# blockdev --flushbufs /dev/ram_vg/ram_lv
	--- and now the whole volume group is gone, all data on the 
		ramdisk were replaced with zeroes

The BLKFLSBUF ioctl is only allowed with CAP_SYS_ADMIN, so there shouldn't be security implications with this.

Whan to do with it? The best thing would be to drop this special ramdisk behavior and make the BLKFLSBUF ioctl flush the buffer cache on ramdisk like on all other block devices. But there may be many users having scripts that depend on this special behavior.

Another possibility is to stop the device mapper from passing the BLKFLSBUF ioctl down.

Quoted 30 lines · Mikulas Patocka

Hi

I found a bug in dm regarding the BLKFLSBUF ioctl.

The BLKFLSBUF ioctl can be called on a block device and it flushes the buffer cache. There is one exception - when it is called on ramdisk, it actually destroys all ramdisk data (it works like a discard on the full device).

The device mapper passes this ioctl down to the underlying device, so when the ioctl is called on a logical volume, it can be used to destroy the underlying volume group if the volume group is on ramdisk.

For example:

# modprobe brd rd_size=1048576
# pvcreate /dev/ram0
# vgcreate ram_vg /dev/ram0
# lvcreate -L 16M -n ram_lv ram_vg
# blockdev --flushbufs /dev/ram_vg/ram_lv
	--- and now the whole volume group is gone, all data on the 
		ramdisk were replaced with zeroes

The BLKFLSBUF ioctl is only allowed with CAP_SYS_ADMIN, so there shouldn't be security implications with this.

Whan to do with it? The best thing would be to drop this special ramdisk behavior and make the BLKFLSBUF ioctl flush the buffer cache on ramdisk like on all other block devices. But there may be many users having scripts that depend on this special behavior.

Quoted 2 lines

Another possibility is to stop the device mapper from passing the BLKFLSBUF ioctl down.

If anything DM is being consistent with what the underlying device is meant to do.

brd_ioctl() destroys the data in response to BLKFLSBUF.. I'm missing why this is a DM-specific problem.

Quoted 36 lines · Mike Snitzer

Quoted 32 lines · Mikulas Patocka

Hi

I found a bug in dm regarding the BLKFLSBUF ioctl.

The BLKFLSBUF ioctl can be called on a block device and it flushes the buffer cache. There is one exception - when it is called on ramdisk, it actually destroys all ramdisk data (it works like a discard on the full device).

The device mapper passes this ioctl down to the underlying device, so when the ioctl is called on a logical volume, it can be used to destroy the underlying volume group if the volume group is on ramdisk.

For example:

# modprobe brd rd_size=1048576
# pvcreate /dev/ram0
# vgcreate ram_vg /dev/ram0
# lvcreate -L 16M -n ram_lv ram_vg
# blockdev --flushbufs /dev/ram_vg/ram_lv
	--- and now the whole volume group is gone, all data on the 
		ramdisk were replaced with zeroes

The BLKFLSBUF ioctl is only allowed with CAP_SYS_ADMIN, so there shouldn't be security implications with this.

Whan to do with it? The best thing would be to drop this special ramdisk behavior and make the BLKFLSBUF ioctl flush the buffer cache on ramdisk like on all other block devices. But there may be many users having scripts that depend on this special behavior.

Another possibility is to stop the device mapper from passing the BLKFLSBUF ioctl down.

Quoted 5 lines

If anything DM is being consistent with what the underlying device is meant to do.

brd_ioctl() destroys the data in response to BLKFLSBUF.. I'm missing why this is a DM-specific problem.

The problem is that if we call it on a logical volume, it destroys all logical volumes on the give physical volume.

I think the right fix is to kill off the BLKFLSBUF special case in brd. Yes, it break compatibility - but in this case the compatibility breaks more than it helps.

Quoted 3 lines · Christoph Hellwig

I think the right fix is to kill off the BLKFLSBUF special case in brd. Yes, it break compatibility - but in this case the compatibility breaks more than it helps.

Jens, please pick up this patch:

From: Mike Snitzer <[email protected]>
Date: Tue, 25 Oct 2016 10:25:07 -0400
Subject: [PATCH] brd: remove support for BLKFLSBUF

Quoted 7 lines · Mike Snitzer

Quoted 3 lines · Christoph Hellwig

I think the right fix is to kill off the BLKFLSBUF special case in brd. Yes, it break compatibility - but in this case the compatibility breaks more than it helps.

Quoted 5 lines

Jens, please pick up this patch:

From: Mike Snitzer <[email protected]>
Date: Tue, 25 Oct 2016 10:25:07 -0400
Subject: [PATCH] brd: remove support for BLKFLSBUF

Added, thanks.

Signature · 1 line

Jens Axboe

The brd driver refuses misaligned discard requests with an error. However, this is suboptimal, misaligned requests could be handled by discarding a part of the request that is aligned on a page boundary. This patch changes the code so that it handles misaligned requests.

Quoted 4 lines · Mikulas Patocka

The brd driver refuses misaligned discard requests with an error. However, this is suboptimal, misaligned requests could be handled by discarding a part of the request that is aligned on a page boundary. This patch changes the code so that it handles misaligned requests.

Hello Mikulas,

We do not want this kind of discard request processing in every block driver. This is why I think that this kind of processing should be added to the block layer. See also "[PATCH v3 0/5] Make blkdev_issue_discard() submit aligned discard requests" (http://www.spinics.net/lists/linux-block/msg02360.html).

Bart.

I think the proper thing would be to move "discard_zeroes_data" flag into the bio itself - there would be REQ_OP_DISCARD and REQ_OP_DISCARD_ZERO - and if the device doesn't support REQ_OP_DISCARD_ZERO, it rejects the bio and the caller is supposed to do zeroing manually.

Quoted 4 lines · Mikulas Patocka

I think the proper thing would be to move "discard_zeroes_data" flag into the bio itself - there would be REQ_OP_DISCARD and REQ_OP_DISCARD_ZERO - and if the device doesn't support REQ_OP_DISCARD_ZERO, it rejects the bio and the caller is supposed to do zeroing manually.

Yes, Martin and I have come to a similar conclusion recently. An additional aspect is that NVMe has a Write Zeroes command which is more limited than what REQ_OP_WRITE_SAME does.

So I think the right way is to add a REQ_OP_WRITE_ZEROES (or REQ_OP_ZERO) and have modifies if it may discard or not.

We could detect if the REQ_OP_WRITE_SAME command contains all zeroes and if it does, turn it into "Write Zeroes" or TRIM command (if the device guarantees zeroing on trim). If it doesn't contain all zeroes and the device doesn't support non-zero WRITE SAME, then reject it.

Or maybe we could add a new command REQ_OP_WRITE_ZEROES - I'm not sure which of these two possibilities is better.

[adding Chaitanya to Cc]

Quoted 4 lines · Mikulas Patocka

We could detect if the REQ_OP_WRITE_SAME command contains all zeroes and if it does, turn it into "Write Zeroes" or TRIM command (if the device guarantees zeroing on trim). If it doesn't contain all zeroes and the device doesn't support non-zero WRITE SAME, then reject it.

I don't like this because it's very inefficient - we have to allocate a payload first and then compare the whole payload for very operation.

Quoted 2 lines

Or maybe we could add a new command REQ_OP_WRITE_ZEROES - I'm not sure which of these two possibilities is better.

Chaitanya actually did an initial prototype implementation of this for NVMe that he shared with me. I liked it a lot and I think he'll be ready to post it in a few days. Now that we have the REQ_OP* values instead of mapping different command types to flags it's actually surprisingly easy to add new block layer operations.

I don't like the idea of complicating the code by turning discards into writes.

The flag "discard_zeroes_data" is actually misdesigned, because the storage stack can change dynamically while bios are in progress. You can send a discard bio to a device mapper device that has "discard_zeroes_data" - and while the bio is in progress, the device mapper stack can be reconfigured to redirect the bio to another device that doesn't have "discard_zeroes_data" - and the bio won't zero data and the caller that issued it has no way to find it out.

I think the proper thing would be to move "discard_zeroes_data" flag into the bio itself - there would be REQ_OP_DISCARD and REQ_OP_DISCARD_ZERO - and if the device doesn't support REQ_OP_DISCARD_ZERO, it rejects the bio and the caller is supposed to do zeroing manually.

Quoted 2 lines · Mikulas Patocka

I don't like the idea of complicating the code by turning discards into writes.

That's not what my patch series does. The only writes added by my patch series are those for the non-aligned head and tail of the range passed to blkdev_issue_zeroout().

Quoted 8 lines

The flag "discard_zeroes_data" is actually misdesigned, because the storage stack can change dynamically while bios are in progress. You can send a discard bio to a device mapper device that has "discard_zeroes_data" - and while the bio is in progress, the device mapper stack can be reconfigured to redirect the bio to another device that doesn't have "discard_zeroes_data" - and the bio won't zero data and the caller that issued it has no way to find it out.

Quoted 4 lines

I think the proper thing would be to move "discard_zeroes_data" flag into the bio itself - there would be REQ_OP_DISCARD and REQ_OP_DISCARD_ZERO - and if the device doesn't support REQ_OP_DISCARD_ZERO, it rejects the bio and the caller is supposed to do zeroing manually.

Sorry but I don't like this proposal. I think that a much better solution would be to pause I/O before making any changes to the discard_zeroes_data flag.

Bart.

Quoted 9 lines · Bart Van Assche

Quoted 4 lines · Mikulas Patocka

I think the proper thing would be to move "discard_zeroes_data" flag into the bio itself - there would be REQ_OP_DISCARD and REQ_OP_DISCARD_ZERO - and if the device doesn't support REQ_OP_DISCARD_ZERO, it rejects the bio and the caller is supposed to do zeroing manually.

Sorry but I don't like this proposal. I think that a much better solution would be to pause I/O before making any changes to the discard_zeroes_data flag.

The device mapper pauses all bios when it switches the table - but the bio was submitted with assumption that it goes to a device withe "discard_zeroes_data" set, then the bio is paused, device mapper table is switched, the bio is unpaused, and now it can go to a device without "discard_zeroes_data".

Quoted 13 lines · Mikulas Patocka

Quoted 11 lines · Bart Van Assche

Quoted 6 lines · Mikulas Patocka

I think the proper thing would be to move "discard_zeroes_data" flag into
the bio itself - there would be REQ_OP_DISCARD and REQ_OP_DISCARD_ZERO - and if the device doesn't support REQ_OP_DISCARD_ZERO, it rejects the bio
and the caller is supposed to do zeroing manually.

Sorry but I don't like this proposal. I think that a much better solution would be to pause I/O before making any changes to the discard_zeroes_data flag.

Quoted 5 lines

The device mapper pauses all bios when it switches the table - but the bio was submitted with assumption that it goes to a device withe "discard_zeroes_data" set, then the bio is paused, device mapper table is switched, the bio is unpaused, and now it can go to a device without "discard_zeroes_data".

Hello Mikulas,

Sorry if I wasn't clear enough. What I meant is to wait until all outstanding requests have finished

before modifying the discard_zeroes_data flag - the kind of operation that is performed by e.g. blk_mq_freeze_queue().

Modifying the discard_zeroes_data flag after a bio has been submitted and before it has completed could lead to several classes of subtle bugs. Functions like __blkdev_issue_discard() assume that the value of the discard_zeroes_data flag does not change after this function has been called and before the submitted requests completed.

Bart.