| Message ID | 20230612135228.10702-1-sergei.shtepa@veeam.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | blksnap - block devices snapshots module | expand |
I'm of course a little byassed by having spent a lot of my own time
on this, but this version now looks ready to merge to me:
Acked-by: Christoph Hellwig <hch@lst.de>
But as Jens just merged my series to reopen the open flag we'll also
need to fold this in:
diff --git a/drivers/block/blksnap/diff_area.c b/drivers/block/blksnap/diff_area.c
index 169fa003b6d66d..0848c947591508 100644
--- a/drivers/block/blksnap/diff_area.c
+++ b/drivers/block/blksnap/diff_area.c
@@ -128,7 +128,7 @@ void diff_area_free(struct kref *kref)
xa_destroy(&diff_area->chunk_map);
if (diff_area->orig_bdev) {
- blkdev_put(diff_area->orig_bdev, FMODE_READ | FMODE_WRITE);
+ blkdev_put(diff_area->orig_bdev, NULL);
diff_area->orig_bdev = NULL;
}
@@ -214,7 +214,8 @@ struct diff_area *diff_area_new(dev_t dev_id, struct diff_storage *diff_storage)
pr_debug("Open device [%u:%u]\n", MAJOR(dev_id), MINOR(dev_id));
- bdev = blkdev_get_by_dev(dev_id, FMODE_READ | FMODE_WRITE, NULL, NULL);
+ bdev = blkdev_get_by_dev(dev_id, BLK_OPEN_READ | BLK_OPEN_WRITE, NULL,
+ NULL);
if (IS_ERR(bdev)) {
int err = PTR_ERR(bdev);
@@ -224,7 +225,7 @@ struct diff_area *diff_area_new(dev_t dev_id, struct diff_storage *diff_storage)
diff_area = kzalloc(sizeof(struct diff_area), GFP_KERNEL);
if (!diff_area) {
- blkdev_put(bdev, FMODE_READ | FMODE_WRITE);
+ blkdev_put(bdev, NULL);
return ERR_PTR(-ENOMEM);
}
diff --git a/drivers/block/blksnap/diff_storage.c b/drivers/block/blksnap/diff_storage.c
index 1787fa6931a816..f3814474b9804a 100644
--- a/drivers/block/blksnap/diff_storage.c
+++ b/drivers/block/blksnap/diff_storage.c
@@ -123,7 +123,7 @@ void diff_storage_free(struct kref *kref)
}
while ((storage_bdev = first_storage_bdev(diff_storage))) {
- blkdev_put(storage_bdev->bdev, FMODE_READ | FMODE_WRITE);
+ blkdev_put(storage_bdev->bdev, NULL);
list_del(&storage_bdev->link);
kfree(storage_bdev);
}
@@ -138,7 +138,7 @@ static struct block_device *diff_storage_add_storage_bdev(
struct storage_bdev *storage_bdev, *existing_bdev = NULL;
struct block_device *bdev;
- bdev = blkdev_get_by_path(bdev_path, FMODE_READ | FMODE_WRITE,
+ bdev = blkdev_get_by_path(bdev_path, BLK_OPEN_READ | BLK_OPEN_WRITE,
NULL, NULL);
if (IS_ERR(bdev)) {
pr_err("Failed to open device. errno=%ld\n", PTR_ERR(bdev));
@@ -153,14 +153,14 @@ static struct block_device *diff_storage_add_storage_bdev(
spin_unlock(&diff_storage->lock);
if (existing_bdev->bdev == bdev) {
- blkdev_put(bdev, FMODE_READ | FMODE_WRITE);
+ blkdev_put(bdev, NULL);
return existing_bdev->bdev;
}
storage_bdev = kzalloc(sizeof(struct storage_bdev) +
strlen(bdev_path) + 1, GFP_KERNEL);
if (!storage_bdev) {
- blkdev_put(bdev, FMODE_READ | FMODE_WRITE);
+ blkdev_put(bdev, NULL);
return ERR_PTR(-ENOMEM);
}
On Mon, Jun 12, 2023 at 03:52:17PM +0200, Sergei Shtepa wrote: > Hi all. > > I am happy to offer a improved version of the Block Devices Snapshots > Module. It allows to create non-persistent snapshots of any block devices. > The main purpose of such snapshots is to provide backups of block devices. > See more in Documentation/block/blksnap.rst. How does blksnap interact with blk-crypto? I.e., what happens if a bio with a ->bi_crypt_context set is submitted to a block device that has blksnap active? If you are unfamiliar with blk-crypto, please read Documentation/block/inline-encryption.rst It looks like blksnap hooks into the block layer directly, via the new "blkfilter" mechanism. I'm concerned that it might ignore ->bi_crypt_context and write data to the disk in plaintext, when it is supposed to be encrypted. - Eric
On Mon, Jun 12, 2023 at 09:19:11AM -0700, Eric Biggers wrote: > On Mon, Jun 12, 2023 at 03:52:17PM +0200, Sergei Shtepa wrote: > > Hi all. > > > > I am happy to offer a improved version of the Block Devices Snapshots > > Module. It allows to create non-persistent snapshots of any block devices. > > The main purpose of such snapshots is to provide backups of block devices. > > See more in Documentation/block/blksnap.rst. > > How does blksnap interact with blk-crypto? > > I.e., what happens if a bio with a ->bi_crypt_context set is submitted to a > block device that has blksnap active? > > If you are unfamiliar with blk-crypto, please read > Documentation/block/inline-encryption.rst > > It looks like blksnap hooks into the block layer directly, via the new > "blkfilter" mechanism. I'm concerned that it might ignore ->bi_crypt_context > and write data to the disk in plaintext, when it is supposed to be encrypted. Yeah. Same for integrity. I guess for now the best would be to not allow attaching a filter to block devices that have encryption or integrity enabled and then look into that as a separate project fully reviewed by the respective experts.
On 6/12/23 18:19, Eric Biggers wrote: > This is the first time you've received an email from this sender > ebiggers@kernel.org, please exercise caution when clicking on links or opening > attachments. > > > On Mon, Jun 12, 2023 at 03:52:17PM +0200, Sergei Shtepa wrote: > > Hi all. > > > > I am happy to offer a improved version of the Block Devices Snapshots > > Module. It allows to create non-persistent snapshots of any block devices. > > The main purpose of such snapshots is to provide backups of block devices. > > See more in Documentation/block/blksnap.rst. > > How does blksnap interact with blk-crypto? > > I.e., what happens if a bio with a ->bi_crypt_context set is submitted to a > block device that has blksnap active? > > If you are unfamiliar with blk-crypto, please read > Documentation/block/inline-encryption.rst Thank you, this is an important point. Yes, that's right. The current version of blksnap can cause blk-crypto to malfunction while holding a snapshot. When handling bios from the file system, the ->bi_crypt_context is preserved. But the bio requests serving the snapshot are executed without context. I think that the snapshot will be unreadable. But I don't see any obstacles in the way of blksnap and blk-crypto compatibility. If DM implements support for blk-crypto, then the same principle can be applied for blksnap. I think that the integration of blksnap with blk-crypto may be one of the stages of further development. The dm-crypto should work properly. It is noteworthy that in 7 years of using the out-of-tree module to take a snapshot, I have not encountered cases of such problems. But incompatibility with blk-crypto is possible, this is already a pain for some users. I will request this information from our support team. > > It looks like blksnap hooks into the block layer directly, via the new > "blkfilter" mechanism. I'm concerned that it might ignore ->bi_crypt_context > and write data to the disk in plaintext, when it is supposed to be encrypted. No. The "blkfilter" mechanism should not affect the operation of blk-crypto. It does not change the bio. Only a module that has been attached and provides its own filtering algorithm, such as blksnap, can violate the logic of blk-crypto. Therefore, until the blksnap module is loaded, blk-crypto should work as before.
On Tue, Jun 13, 2023 at 12:12:19PM +0200, Sergei Shtepa wrote: > On 6/12/23 18:19, Eric Biggers wrote: > > This is the first time you've received an email from this sender > > ebiggers@kernel.org, please exercise caution when clicking on links or opening > > attachments. > > > > > > On Mon, Jun 12, 2023 at 03:52:17PM +0200, Sergei Shtepa wrote: > > > Hi all. > > > > > > I am happy to offer a improved version of the Block Devices Snapshots > > > Module. It allows to create non-persistent snapshots of any block devices. > > > The main purpose of such snapshots is to provide backups of block devices. > > > See more in Documentation/block/blksnap.rst. > > > > How does blksnap interact with blk-crypto? > > > > I.e., what happens if a bio with a ->bi_crypt_context set is submitted to a > > block device that has blksnap active? > > > > If you are unfamiliar with blk-crypto, please read > > Documentation/block/inline-encryption.rst > > Thank you, this is an important point. Yes, that's right. > The current version of blksnap can cause blk-crypto to malfunction while > holding a snapshot. When handling bios from the file system, the > ->bi_crypt_context is preserved. But the bio requests serving the snapshot > are executed without context. I think that the snapshot will be unreadable. Well not only would the resulting snapshot be unreadable, but plaintext data would be written to disk, contrary to the intent of the submitter of the bios. That would be a security vulnerability. If the initial version of blksnap isn't going to be compatible with blk-crypto, that is tolerable for now, but there needs to be an explicit check to cause an error to be returned if the two features are combined, before anything is written to disk. - Eric