[PATCH] dm-crypt: add mapping table option to allowing discard requests

[PATCH] dm-crypt: add mapping table option to allowing discard requests

[Milan Broz]

Add optional parameter field to dmcrypt table and support
"allow_discards" option.

Discard requests bypass crypt queue processing, bio
request is simple remapped to underlying device.

Note that discard will be never enabled by default because
of security consequences, it is up to administrator
to enable it for encrypted devices.

(Note that userspace cryptsetup will not understand new
optional parameters yet, support for this will come later.)

Signed-off-by: Milan Broz <mbroz@redhat.com>
---
 Documentation/device-mapper/dm-crypt.txt |   19 +++++++++++++-
 drivers/md/dm-crypt.c                    |   39 ++++++++++++++++++++++++++---
 2 files changed, 53 insertions(+), 5 deletions(-)

diff --git a/Documentation/device-mapper/dm-crypt.txt b/Documentation/device-mapper/dm-crypt.txt
index 6b5c42d..08a36a7 100644
--- a/Documentation/device-mapper/dm-crypt.txt
+++ b/Documentation/device-mapper/dm-crypt.txt
@@ -4,7 +4,7 @@ dm-crypt
 Device-Mapper's "crypt" target provides transparent encryption of block devices
 using the kernel crypto API.
 
-Parameters: <cipher> <key> <iv_offset> <device path> <offset>
+Parameters: <cipher> <key> <iv_offset> <device path> <offset> [<#opt_params> <opt_params>]
 
 <cipher>
     Encryption cipher and an optional IV generation mode.
@@ -37,6 +37,23 @@ Parameters: <cipher> <key> <iv_offset> <device path> <offset>
 <offset>
     Starting sector within the device where the encrypted data begins.
 
+<#opt_params>
+    Number of optional parameters. If there are no optional parameters,
+    the optional paramaters section can be skipped or #opt_params can be zero.
+    Otherwise #opt_params indicates count of following arguments.
+
+    Examples of optional parameters section:
+        1 allow_discards
+
+allow_discards
+    The block discards requests are passed through the crypt device.
+    (Default is to block discards requests.)
+
+    WARNING: allowing discard on encrypted device has serious irreversible
+    security consequences, discarded blocks can be easily located on device
+    later. This can lead to leak of information from ciphertext device
+    (unique pattern for detecting filesystem type, used space etc).
+
 Example scripts
 ===============
 LUKS (Linux Unified Key Setup) is now the preferred way to set up disk
diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index c8827ff..2747483 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -1575,11 +1575,11 @@ bad_mem:
 static int crypt_ctr(struct dm_target *ti, unsigned int argc, char **argv)
 {
 	struct crypt_config *cc;
-	unsigned int key_size;
+	unsigned int key_size, opt_params;
 	unsigned long long tmpll;
 	int ret;
 
-	if (argc != 5) {
+	if (argc < 5) {
 		ti->error = "Not enough arguments";
 		return -EINVAL;
 	}
@@ -1648,6 +1648,26 @@ static int crypt_ctr(struct dm_target *ti, unsigned int argc, char **argv)
 	}
 	cc->start = tmpll;
 
+	/* Optional parameters */
+	if (argc > 5) {
+		if (sscanf(argv[5], "%u", &opt_params) < 0) {
+			ti->error = "Invalid optional parameters";
+			goto bad;
+		}
+
+		if (opt_params + 6 > argc) {
+			ti->error = "Arguments do not agree with counts given";
+			goto bad;
+		}
+
+		if (opt_params == 1 && !strcmp(argv[6], "allow_discards"))
+			ti->num_discard_requests = 1;
+		else if (opt_params) {
+			ti->error = "Invalid optional parameters";
+			goto bad;
+		}
+	}
+
 	ret = -ENOMEM;
 	cc->io_queue = alloc_workqueue("kcryptd_io",
 				       WQ_NON_REENTRANT|
@@ -1682,9 +1702,16 @@ static int crypt_map(struct dm_target *ti, struct bio *bio,
 	struct dm_crypt_io *io;
 	struct crypt_config *cc;
 
-	if (bio->bi_rw & REQ_FLUSH) {
+	/*
+	 * If bio is REQ_FLUSH or REQ_DISCARD, just bypass crypt queues.
+	 * - for REQ_FLUSH device-mapper core ensures that no IO is in-flight
+	 * - for REQ_DISCARD caller must use flush if IO ordering matters
+	 */
+	if (unlikely(bio->bi_rw & (REQ_FLUSH | REQ_DISCARD))) {
 		cc = ti->private;
 		bio->bi_bdev = cc->dev->bdev;
+		if (bio_sectors(bio))
+			bio->bi_sector = cc->start + dm_target_offset(ti, bio->bi_sector);
 		return DM_MAPIO_REMAPPED;
 	}
 
@@ -1727,6 +1754,10 @@ static int crypt_status(struct dm_target *ti, status_type_t type,
 
 		DMEMIT(" %llu %s %llu", (unsigned long long)cc->iv_offset,
 				cc->dev->name, (unsigned long long)cc->start);
+
+		if (ti->num_discard_requests)
+			DMEMIT(" 1 allow_discards");
+
 		break;
 	}
 	return 0;
@@ -1823,7 +1854,7 @@ static int crypt_iterate_devices(struct dm_target *ti,
 
 static struct target_type crypt_target = {
 	.name   = "crypt",
-	.version = {1, 10, 0},
+	.version = {1, 11, 0},
 	.module = THIS_MODULE,
 	.ctr    = crypt_ctr,
 	.dtr    = crypt_dtr,
-- 
1.7.5.4

Re: [PATCH] dm-crypt: add mapping table option to allowing discard requests

[Jeff Cook]

On 07/14/2011 05:56 AM, Milan Broz wrote:
> Add optional parameter field to dmcrypt table and support
> "allow_discards" option.
> 
> Discard requests bypass crypt queue processing, bio
> request is simple remapped to underlying device.
> 
> Note that discard will be never enabled by default because
> of security consequences, it is up to administrator
> to enable it for encrypted devices.
> 
> (Note that userspace cryptsetup will not understand new
> optional parameters yet, support for this will come later.)
> 
> Signed-off-by: Milan Broz <mbroz@redhat.com>
> [snip]

Thanks very much for this patch Milan. I will try compiling it shortly
and see where things go -- are you expecting this to be submitted for
inclusion in mainline soon?

Thanks again.

From
Jeff

Re: [PATCH] dm-crypt: add mapping table option to allowing discard requests

[Alasdair G Kergon]

On Thu, Jul 14, 2011 at 06:56:03PM -0600, Jeff Cook wrote:
> Thanks very much for this patch Milan. I will try compiling it shortly
> and see where things go -- are you expecting this to be submitted for
> inclusion in mainline soon?
 
I already submitted it for linux-next, so unless people report problems
which we can't fix quickly, it'll be submitted during the next merge window.

Alasdair

Re: dm-crypt: add mapping table option to allowing discard requests

[Mike Snitzer]

On Thu, Jul 14 2011 at  8:56pm -0400,
Jeff Cook <jeff@deserettechnology.com> wrote:

> On 07/14/2011 05:56 AM, Milan Broz wrote:
> > Add optional parameter field to dmcrypt table and support
> > "allow_discards" option.
> > 
> > Discard requests bypass crypt queue processing, bio
> > request is simple remapped to underlying device.
> > 
> > Note that discard will be never enabled by default because
> > of security consequences, it is up to administrator
> > to enable it for encrypted devices.
> > 
> > (Note that userspace cryptsetup will not understand new
> > optional parameters yet, support for this will come later.)
> > 
> > Signed-off-by: Milan Broz <mbroz@redhat.com>
> > [snip]
> 
> Thanks very much for this patch Milan. I will try compiling it shortly
> and see where things go -- are you expecting this to be submitted for
> inclusion in mainline soon?

It is already staged for linux-next (and upstream inclusion when the 3.1
merge window opens):
ftp://sources.redhat.com/pub/dm/patches/2.6-unstable/editing/patches/dm-crypt-optionally-support-discard-requests.patch

NOTE: this version of the patch depends on additional changes that were
also staged, e.g.:
ftp://sources.redhat.com/pub/dm/patches/2.6-unstable/editing/patches/dm-table-share-target-argument-parsing-functions.patch

Re: [PATCH] dm-crypt: add mapping table option to allowing discard requests

[Alasdair G Kergon]

To check the status of a dm patch, look at patchwork in the first instance.

The vast majority of dm patches are submitted to dm-devel.
They are caught by patchwork here:
  https://patchwork.kernel.org/project/dm-devel/list/?state=*

Initially status is 'New'.
Patches that are assigned to 'agk' are deemed ready for me to look at.
I set the status to 'Under Review' when I take a patch into my 'editing' quilt
tree, which is synced onto two independent servers here:
  http://www.kernel.org/pub/linux/kernel/people/agk/patches/2.6/editing/
  ftp://sources.redhat.com/pub/dm/patches/2.6-unstable/editing/patches/

At this point the patch will be below the 'next' line (as in 'linux-next') in
the quilt series file, also uploaded there.  The number at the end of the
line is the patchwork patch number.  (Multiple numbers mean I folded
patches together.)

When I've reviewed a patch, I add my sign off, move it into the linux-next
section and set the patchwork state to 'Awaiting Upstream'.

When a group of patches enters the upstream kernel, I set the state to
'Accepted' and 'archived'.

(With some minor patches that I fold into other patches, I sometimes
set the state to 'Accepted' and 'archived' immediately as it's no
longer worth tracking them separately.  The actual patch included
upstream is often not identical to the one in patchwork as I can make
small changes to it for many reasons during review.)

Periodically I take a backup of the patch directory (tagged with the kernel
version) and remove some patches no longer needed (including patches
now upstream).  (Go up the directory hierarchy to see these backups.)

Alasdair

Re: dm-crypt: add mapping table option to allowing discard requests

[Jeff Cook]

On 07/14/2011 07:13 PM, Mike Snitzer wrote:
> On Thu, Jul 14 2011 at  8:56pm -0400,
> Jeff Cook <jeff@deserettechnology.com> wrote:
>> Thanks very much for this patch Milan. I will try compiling it shortly
>> and see where things go -- are you expecting this to be submitted for
>> inclusion in mainline soon?
> 
> It is already staged for linux-next (and upstream inclusion when the 3.1
> merge window opens):
> ftp://sources.redhat.com/pub/dm/patches/2.6-unstable/editing/patches/dm-crypt-optionally-support-discard-requests.patch
> 
> NOTE: this version of the patch depends on additional changes that were
> also staged, e.g.:
> ftp://sources.redhat.com/pub/dm/patches/2.6-unstable/editing/patches/dm-table-share-target-argument-parsing-functions.patch

Sorry to be a pest, but I have a few more basic questions about the
application of these patches and their use on my encrypted disk.

First of all, I attempted to apply these patches to 3.0-rc6 and HEAD
(slightly after 3.0-rc7) and they did not apply cleanly. I was able to
get a correct application once, but then the compile failed, assumedly
due to a mispatching. Now that 3.0 has been tagged, should I assume that
the patches will apply cleanly to that tree? I tried both manual patch
via /usr/bin/patch and git apply. I suppose I can just wait for 3.1-rc1
if I must.

Secondly, I am not sure how I go about modifying the crypt table mapping
to activate the discard pass-through option.

I recognize that Milan's commit message indicates support is missing in
cryptsetup -- does this mean that a patch must be written to cryptsetup
before the new option can be activated or can this be accessed by the
lower-level dmsetup tool? I would guess the latter, though I don't
imagine it would be difficult to patch cryptsetup if needed (I haven't
looked at the code yet, though).

Is the mapping table modifiable at mount time, so this feature can be
activated on existing dm-crypt partitions, or would this require the
partition to be formatted with different options? If it can be modified
on mount, how do we set those parameters? Can I just boot off of a live
medium, mount manually with (patched) cryptsetup luksOpen, and have the
change kept on disk, or do I need to do something else?

Apologies for these noob-ish questions -- just thought I'd get much
better information straight from the source. All help is appreciated,
and I would like to express my gratitude once more for the construction
of this patch in the first place. Thanks Milan, Mike, and anyone else
involved for your effort, this will be a big help to a lot of people I
think. I almost didn't buy a SSD due to the lack of TRIM support in
dm-crypt. :)

Thanks again.

From
Jeff

Re: dm-crypt: add mapping table option to allowing discard requests

[Milan Broz]

On 07/22/2011 11:41 AM, Jeff Cook wrote:
> First of all, I attempted to apply these patches to 3.0-rc6 and HEAD
> (slightly after 3.0-rc7) and they did not apply cleanly. I was able to
> get a correct application once, but then the compile failed, assumedly
> due to a mispatching. Now that 3.0 has been tagged, should I assume that
> the patches will apply cleanly to that tree? I tried both manual patch
> via /usr/bin/patch and git apply. I suppose I can just wait for 3.1-rc1
> if I must.

So better wait for rc1, patch is already in linux-next.
My original patch was not using new table parsing function so I have
no idea which patches need to be backported now.

> Secondly, I am not sure how I go about modifying the crypt table mapping
> to activate the discard pass-through option.

You can compile upstream cryptsetup, I already added flag to support discard.

But there is intentional library version bump because I removed old api symbols
and also I want to implement some features before new version is released,
so use with care - it is not stable code yet.
 
> I recognize that Milan's commit message indicates support is missing in
> cryptsetup -- does this mean that a patch must be written to cryptsetup
> before the new option can be activated or can this be accessed by the
> lower-level dmsetup tool?

You can use dmsetup for now, I sent description how to do that to dmcrypt
list. http://article.gmane.org/gmane.linux.kernel.device-mapper.dm-crypt/5166

> I almost didn't buy a SSD due to the lack of TRIM support in
> dm-crypt. :)

TRIM is incredibly overrated:-)

Milan