[PATCH] Adaptation secure erase forwarding for 4.1x kernels

[PATCH] Adaptation secure erase forwarding for 4.1x kernels

[Denis Semakin]

Hello.
Here is fixed patch for modern 4.1x kernels.
The idea is to forward secure erase request within device mapper layer to
block device driver which can support secure erase.
Could you please review?
Thanks.

diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
index 7eb3e2a..d955a57 100644
--- a/drivers/md/dm-table.c
+++ b/drivers/md/dm-table.c
@@ -1846,6 +1846,33 @@ static bool dm_table_supports_discards(struct dm_table *t)
        return true;
 }

+static int device_not_secerase_capable(struct dm_target *ti,
+                                          struct dm_dev *dev, sector_t start,
+                                          sector_t len, void *data)
+{
+       struct request_queue *q = bdev_get_queue(dev->bdev);
+
+       return q && !blk_queue_secure_erase(q);
+}
+
+static bool dm_targets_support_secure_erase(struct dm_table *t)
+{
+       unsigned int i = 0;
+       struct dm_target *ti;
+
+       while (i < dm_table_get_num_targets(t)) {
+               ti = dm_table_get_target(t, i++);
+
+               if (!ti->type->iterate_devices ||
+                   ti->type->iterate_devices(ti, device_not_secerase_capable,
+                                             NULL))
+               return false;
+       }
+
+       return true;
+
+}
+
 void dm_table_set_restrictions(struct dm_table *t, struct request_queue *q,
                               struct queue_limits *limits)
 {
@@ -1867,6 +1894,9 @@ void dm_table_set_restrictions(struct dm_table *t, struct request_queue *q,
        } else
                queue_flag_set_unlocked(QUEUE_FLAG_DISCARD, q);

+       if (dm_targets_support_secure_erase(t))
+               queue_flag_set_unlocked(QUEUE_FLAG_SECERASE, q);
+
        if (dm_table_supports_flush(t, (1UL << QUEUE_FLAG_WC))) {
                wc = true;
                if (dm_table_supports_flush(t, (1UL << QUEUE_FLAG_FUA)))

-- 
Best regards,

Denis Semakin

[PATCH] dm table: add support for secure erase forwarding [was: Re: Adaptation secure erase forwarding for 4.1x kernels]

[Mike Snitzer]

On Tue, Mar 13 2018 at  5:23am -0400,
Denis Semakin <d.semakin@omprussia.ru> wrote:

> Hello.
> Here is fixed patch for modern 4.1x kernels.
> The idea is to forward secure erase request within device mapper layer to
> block device driver which can support secure erase.
> Could you please review?

There were various issues with your patch that I cleaned up, please see
the following.

But I'm left skeptical that this is enough.  Don't targets need to
explicitly handle these REQ_OP_SECURE_ERASE requests?  Similar to how
REQ_OP_DISCARD is handled?

I'd feel safer about having targets opt-in with setting (a new)
ti->num_secure_erase_bios.

Which DM target(s) have you been wanting to pass REQ_OP_SECURE_ERASE
bios?

Mike


From: Denis Semakin <d.semakin@omprussia.ru>
Date: Tue, 13 Mar 2018 13:23:45 +0400
Subject: [PATCH] dm table: add support for secure erase forwarding

Set QUEUE_FLAG_SECERASE in DM device's queue_flags if a DM table's
data devices support secure erase.

Signed-off-by: Denis Semakin <d.semakin@omprussia.ru>
Signed-off-by: Mike Snitzer <snitzer@redhat.com>
---
 drivers/md/dm-table.c |   28 ++++++++++++++++++++++++++++
 1 files changed, 28 insertions(+), 0 deletions(-)

diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
index 7eb3e2a..d857369 100644
--- a/drivers/md/dm-table.c
+++ b/drivers/md/dm-table.c
@@ -1846,6 +1846,31 @@ static bool dm_table_supports_discards(struct dm_table *t)
 	return true;
 }
 
+static int device_not_secure_erase_capable(struct dm_target *ti,
+					   struct dm_dev *dev, sector_t start,
+					   sector_t len, void *data)
+{
+       struct request_queue *q = bdev_get_queue(dev->bdev);
+
+       return q && !blk_queue_secure_erase(q);
+}
+
+static bool dm_table_supports_secure_erase(struct dm_table *t)
+{
+       struct dm_target *ti;
+       unsigned int i;
+
+       for (i = 0; i < dm_table_get_num_targets(t); i++) {
+               ti = dm_table_get_target(t, i);
+
+               if (!ti->type->iterate_devices ||
+                   ti->type->iterate_devices(ti, device_not_secure_erase_capable, NULL))
+		       return false;
+       }
+
+       return true;
+}
+
 void dm_table_set_restrictions(struct dm_table *t, struct request_queue *q,
 			       struct queue_limits *limits)
 {
@@ -1867,6 +1892,9 @@ void dm_table_set_restrictions(struct dm_table *t, struct request_queue *q,
 	} else
 		queue_flag_set_unlocked(QUEUE_FLAG_DISCARD, q);
 
+	if (dm_table_supports_secure_erase(t))
+		queue_flag_set_unlocked(QUEUE_FLAG_SECERASE, q);
+
 	if (dm_table_supports_flush(t, (1UL << QUEUE_FLAG_WC))) {
 		wc = true;
 		if (dm_table_supports_flush(t, (1UL << QUEUE_FLAG_FUA)))
-- 
1.7.4.4

Re: [PATCH] dm table: add support for secure erase forwarding [was: Re: Adaptation secure erase forwarding for 4.1x kernels]

[Denis Semakin]

Hi.
Soon or later everybody start to think about security.
One of the most frequently requirement is 100% reliable data deletion from
any device in case of compromising or loss or theft.

For this, drive and memory cards manufacturers provide ERASE and TRIM features
which can notice (inform) controller of the device to erase sectors
(write down only zeros or one or random data). Features can be triggered
by calling ioctl() requests or a mount options (like ext4 does). But this works
only with whole device -- /dev/sdX, /dev/mmcblk0pX...
But what if for some security reasons we need to secure delete a single file.
A file-system layer provide this functionality... one may call __blkdev_issue_discard()
with BLKDEV_DISCARD_SECURE flag.
But...
All this works well if there is no virtual layer (like device-mapper)
between file-system and block-layer, because if device driver supports
this feature it can set up related flag in request_queue flags.

I have ext4 lvm partitions on my test instance and a drive which can
secure erase sectors.
Without lvm it works, with lvm it doesn't.
That's the purpose if this patch - to provide the opportunity to secure erase
given sectors (through device-mapper layer, forward request) that were assigned for regular file.



>But I'm left skeptical that this is enough.  Don't targets need to
>explicitly handle these REQ_OP_SECURE_ERASE requests?  Similar to how
>REQ_OP_DISCARD is handled?

I think yes, REQ_OP_DISCARD will not secure erase the data and it can be possible
to get it from device.

>I'd feel safer about having targets opt-in with setting (a new)
>ti->num_secure_erase_bios.

Well... May it make sense but I didn't see any reasons to add it in patch.

>Which DM target(s) have you been wanting to pass REQ_OP_SECURE_ERASE
>bios?
I think first of all a linear target of course should have this. For others I'm not sure, I need
to investigate.

Hopefully, I answered to all your question.


Denis

----- Исходное сообщение -----
От: "snitzer" <snitzer@redhat.com>
Кому: "Denis Semakin" <d.semakin@omprussia.ru>
Копия: "dm-devel" <dm-devel@redhat.com>
Отправленные: Четверг, 22 Март 2018 г 18:10:46
Тема: [PATCH] dm table: add support for secure erase forwarding [was: Re: Adaptation secure erase forwarding for 4.1x kernels]

On Tue, Mar 13 2018 at  5:23am -0400,
Denis Semakin <d.semakin@omprussia.ru> wrote:

> Hello.
> Here is fixed patch for modern 4.1x kernels.
> The idea is to forward secure erase request within device mapper layer to
> block device driver which can support secure erase.
> Could you please review?

There were various issues with your patch that I cleaned up, please see
the following.

But I'm left skeptical that this is enough.  Don't targets need to
explicitly handle these REQ_OP_SECURE_ERASE requests?  Similar to how
REQ_OP_DISCARD is handled?

I'd feel safer about having targets opt-in with setting (a new)
ti->num_secure_erase_bios.

Which DM target(s) have you been wanting to pass REQ_OP_SECURE_ERASE
bios?

Mike


From: Denis Semakin <d.semakin@omprussia.ru>
Date: Tue, 13 Mar 2018 13:23:45 +0400
Subject: [PATCH] dm table: add support for secure erase forwarding

Set QUEUE_FLAG_SECERASE in DM device's queue_flags if a DM table's
data devices support secure erase.

Signed-off-by: Denis Semakin <d.semakin@omprussia.ru>
Signed-off-by: Mike Snitzer <snitzer@redhat.com>
---
 drivers/md/dm-table.c |   28 ++++++++++++++++++++++++++++
 1 files changed, 28 insertions(+), 0 deletions(-)

diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
index 7eb3e2a..d857369 100644
--- a/drivers/md/dm-table.c
+++ b/drivers/md/dm-table.c
@@ -1846,6 +1846,31 @@ static bool dm_table_supports_discards(struct dm_table *t)
 	return true;
 }
 
+static int device_not_secure_erase_capable(struct dm_target *ti,
+					   struct dm_dev *dev, sector_t start,
+					   sector_t len, void *data)
+{
+       struct request_queue *q = bdev_get_queue(dev->bdev);
+
+       return q && !blk_queue_secure_erase(q);
+}
+
+static bool dm_table_supports_secure_erase(struct dm_table *t)
+{
+       struct dm_target *ti;
+       unsigned int i;
+
+       for (i = 0; i < dm_table_get_num_targets(t); i++) {
+               ti = dm_table_get_target(t, i);
+
+               if (!ti->type->iterate_devices ||
+                   ti->type->iterate_devices(ti, device_not_secure_erase_capable, NULL))
+		       return false;
+       }
+
+       return true;
+}
+
 void dm_table_set_restrictions(struct dm_table *t, struct request_queue *q,
 			       struct queue_limits *limits)
 {
@@ -1867,6 +1892,9 @@ void dm_table_set_restrictions(struct dm_table *t, struct request_queue *q,
 	} else
 		queue_flag_set_unlocked(QUEUE_FLAG_DISCARD, q);
 
+	if (dm_table_supports_secure_erase(t))
+		queue_flag_set_unlocked(QUEUE_FLAG_SECERASE, q);
+
 	if (dm_table_supports_flush(t, (1UL << QUEUE_FLAG_WC))) {
 		wc = true;
 		if (dm_table_supports_flush(t, (1UL << QUEUE_FLAG_FUA)))
-- 
1.7.4.4
-- 


--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH] dm table: add support for secure erase forwarding [was: Re: Adaptation secure erase forwarding for 4.1x kernels]

[Denis Semakin]

Additional.

>Which DM target(s) have you been wanting to pass REQ_OP_SECURE_ERASE
>bios?
I've tested only with linear targets, so I suppose it's needed to
check what kind of target(s) we have.


----- Исходное сообщение -----
От: "snitzer" <snitzer@redhat.com>
Кому: "Denis Semakin" <d.semakin@omprussia.ru>
Копия: "dm-devel" <dm-devel@redhat.com>
Отправленные: Четверг, 22 Март 2018 г 18:10:46
Тема: [PATCH] dm table: add support for secure erase forwarding [was: Re: Adaptation secure erase forwarding for 4.1x kernels]

On Tue, Mar 13 2018 at  5:23am -0400,
Denis Semakin <d.semakin@omprussia.ru> wrote:

> Hello.
> Here is fixed patch for modern 4.1x kernels.
> The idea is to forward secure erase request within device mapper layer to
> block device driver which can support secure erase.
> Could you please review?

There were various issues with your patch that I cleaned up, please see
the following.

But I'm left skeptical that this is enough.  Don't targets need to
explicitly handle these REQ_OP_SECURE_ERASE requests?  Similar to how
REQ_OP_DISCARD is handled?

I'd feel safer about having targets opt-in with setting (a new)
ti->num_secure_erase_bios.

Which DM target(s) have you been wanting to pass REQ_OP_SECURE_ERASE
bios?

Mike


From: Denis Semakin <d.semakin@omprussia.ru>
Date: Tue, 13 Mar 2018 13:23:45 +0400
Subject: [PATCH] dm table: add support for secure erase forwarding

Set QUEUE_FLAG_SECERASE in DM device's queue_flags if a DM table's
data devices support secure erase.

Signed-off-by: Denis Semakin <d.semakin@omprussia.ru>
Signed-off-by: Mike Snitzer <snitzer@redhat.com>
---
 drivers/md/dm-table.c |   28 ++++++++++++++++++++++++++++
 1 files changed, 28 insertions(+), 0 deletions(-)

diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
index 7eb3e2a..d857369 100644
--- a/drivers/md/dm-table.c
+++ b/drivers/md/dm-table.c
@@ -1846,6 +1846,31 @@ static bool dm_table_supports_discards(struct dm_table *t)
 	return true;
 }
 
+static int device_not_secure_erase_capable(struct dm_target *ti,
+					   struct dm_dev *dev, sector_t start,
+					   sector_t len, void *data)
+{
+       struct request_queue *q = bdev_get_queue(dev->bdev);
+
+       return q && !blk_queue_secure_erase(q);
+}
+
+static bool dm_table_supports_secure_erase(struct dm_table *t)
+{
+       struct dm_target *ti;
+       unsigned int i;
+
+       for (i = 0; i < dm_table_get_num_targets(t); i++) {
+               ti = dm_table_get_target(t, i);
+
+               if (!ti->type->iterate_devices ||
+                   ti->type->iterate_devices(ti, device_not_secure_erase_capable, NULL))
+		       return false;
+       }
+
+       return true;
+}
+
 void dm_table_set_restrictions(struct dm_table *t, struct request_queue *q,
 			       struct queue_limits *limits)
 {
@@ -1867,6 +1892,9 @@ void dm_table_set_restrictions(struct dm_table *t, struct request_queue *q,
 	} else
 		queue_flag_set_unlocked(QUEUE_FLAG_DISCARD, q);
 
+	if (dm_table_supports_secure_erase(t))
+		queue_flag_set_unlocked(QUEUE_FLAG_SECERASE, q);
+
 	if (dm_table_supports_flush(t, (1UL << QUEUE_FLAG_WC))) {
 		wc = true;
 		if (dm_table_supports_flush(t, (1UL << QUEUE_FLAG_FUA)))
-- 
1.7.4.4
-- 
Best regards,

Denis Semakin
Software Developer
Open Mobile Platform

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH] dm table: add support for secure erase forwarding [was: Re: Adaptation secure erase forwarding for 4.1x kernels]

[Denis Semakin]

>I'd feel safer about having targets opt-in with setting (a new)
>ti->num_secure_erase_bios.
May be add a new field "bool secure_erase_supported:1" in dm_target structure instead?
And set up it "true" in constructor for linear targets.


----- Исходное сообщение -----
От: "snitzer" <snitzer@redhat.com>
Кому: "Denis Semakin" <d.semakin@omprussia.ru>
Копия: "dm-devel" <dm-devel@redhat.com>
Отправленные: Четверг, 22 Март 2018 г 18:10:46
Тема: [PATCH] dm table: add support for secure erase forwarding [was: Re: Adaptation secure erase forwarding for 4.1x kernels]

On Tue, Mar 13 2018 at  5:23am -0400,
Denis Semakin <d.semakin@omprussia.ru> wrote:

> Hello.
> Here is fixed patch for modern 4.1x kernels.
> The idea is to forward secure erase request within device mapper layer to
> block device driver which can support secure erase.
> Could you please review?

There were various issues with your patch that I cleaned up, please see
the following.

But I'm left skeptical that this is enough.  Don't targets need to
explicitly handle these REQ_OP_SECURE_ERASE requests?  Similar to how
REQ_OP_DISCARD is handled?

I'd feel safer about having targets opt-in with setting (a new)
ti->num_secure_erase_bios.

Which DM target(s) have you been wanting to pass REQ_OP_SECURE_ERASE
bios?

Mike


From: Denis Semakin <d.semakin@omprussia.ru>
Date: Tue, 13 Mar 2018 13:23:45 +0400
Subject: [PATCH] dm table: add support for secure erase forwarding

Set QUEUE_FLAG_SECERASE in DM device's queue_flags if a DM table's
data devices support secure erase.

Signed-off-by: Denis Semakin <d.semakin@omprussia.ru>
Signed-off-by: Mike Snitzer <snitzer@redhat.com>
---
 drivers/md/dm-table.c |   28 ++++++++++++++++++++++++++++
 1 files changed, 28 insertions(+), 0 deletions(-)

diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
index 7eb3e2a..d857369 100644
--- a/drivers/md/dm-table.c
+++ b/drivers/md/dm-table.c
@@ -1846,6 +1846,31 @@ static bool dm_table_supports_discards(struct dm_table *t)
 	return true;
 }
 
+static int device_not_secure_erase_capable(struct dm_target *ti,
+					   struct dm_dev *dev, sector_t start,
+					   sector_t len, void *data)
+{
+       struct request_queue *q = bdev_get_queue(dev->bdev);
+
+       return q && !blk_queue_secure_erase(q);
+}
+
+static bool dm_table_supports_secure_erase(struct dm_table *t)
+{
+       struct dm_target *ti;
+       unsigned int i;
+
+       for (i = 0; i < dm_table_get_num_targets(t); i++) {
+               ti = dm_table_get_target(t, i);
+
+               if (!ti->type->iterate_devices ||
+                   ti->type->iterate_devices(ti, device_not_secure_erase_capable, NULL))
+		       return false;
+       }
+
+       return true;
+}
+
 void dm_table_set_restrictions(struct dm_table *t, struct request_queue *q,
 			       struct queue_limits *limits)
 {
@@ -1867,6 +1892,9 @@ void dm_table_set_restrictions(struct dm_table *t, struct request_queue *q,
 	} else
 		queue_flag_set_unlocked(QUEUE_FLAG_DISCARD, q);
 
+	if (dm_table_supports_secure_erase(t))
+		queue_flag_set_unlocked(QUEUE_FLAG_SECERASE, q);
+
 	if (dm_table_supports_flush(t, (1UL << QUEUE_FLAG_WC))) {
 		wc = true;
 		if (dm_table_supports_flush(t, (1UL << QUEUE_FLAG_FUA)))
-- 
1.7.4.4
-- 
Best regards,

Denis Semakin
Software Developer
Open Mobile Platform

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: dm table: add support for secure erase forwarding [was: Re: Adaptation secure erase forwarding for 4.1x kernels]

[Mike Snitzer]

On Fri, Mar 23 2018 at  4:14am -0400,
Denis Semakin <d.semakin@omprussia.ru> wrote:

> Hi.
> Soon or later everybody start to think about security.
> One of the most frequently requirement is 100% reliable data deletion from
> any device in case of compromising or loss or theft.
> 
> For this, drive and memory cards manufacturers provide ERASE and TRIM features
> which can notice (inform) controller of the device to erase sectors
> (write down only zeros or one or random data). Features can be triggered
> by calling ioctl() requests or a mount options (like ext4 does). But this works
> only with whole device -- /dev/sdX, /dev/mmcblk0pX...
> But what if for some security reasons we need to secure delete a single file.
> A file-system layer provide this functionality... one may call __blkdev_issue_discard()
> with BLKDEV_DISCARD_SECURE flag.
> But...
> All this works well if there is no virtual layer (like device-mapper)
> between file-system and block-layer, because if device driver supports
> this feature it can set up related flag in request_queue flags.
> 
> I have ext4 lvm partitions on my test instance and a drive which can
> secure erase sectors.
> Without lvm it works, with lvm it doesn't.
> That's the purpose if this patch - to provide the opportunity to secure erase
> given sectors (through device-mapper layer, forward request) that were assigned for regular file.

I was aware of all that.  I understand that DM currently isn't setting
the QUEUE_FLAG_SECERASE, etc.

> >But I'm left skeptical that this is enough.  Don't targets need to
> >explicitly handle these REQ_OP_SECURE_ERASE requests?  Similar to how
> >REQ_OP_DISCARD is handled?
> 
> I think yes, REQ_OP_DISCARD will not secure erase the data and it can be possible
> to get it from device.

Think you misunderstood me.

I meant that: DM must take special care to properly handle
REQ_OP_DISCARD.  Particularly more complex targets like thinp, etc.

But even more basic targets (e.g. dm-stripe.c) have code to optimize
handling of REQ_OP_DISCARD.  We'd likely want REQ_OP_SECURE_ERASE to be
using dm-stripe.c's REQ_OP_DISCARD optimization too.

> >I'd feel safer about having targets opt-in with setting (a new)
> >ti->num_secure_erase_bios.
> 
> Well... May it make sense but I didn't see any reasons to add it in patch.

Thinking further, it is absolutely needed, otherwise dm-stripe.c won't
be able to use the optimization I mentioned above.  In addition, other
targets shouldn't be getting REQ_OP_SECURE_ERASE unless there is a real
need.  ti->num_secure_erase_bios will allow for a much safer and
controlled rollout of this support.
 
> >Which DM target(s) have you been wanting to pass REQ_OP_SECURE_ERASE
> >bios?
> I think first of all a linear target of course should have this. For others I'm not sure, I need
> to investigate.

I think "linear" and "striped" would be the ideal first targets to add
this support to.

I can work through adding the other changes at some point soon.

Mike

Re: dm table: add support for secure erase forwarding [was: Re: Adaptation secure erase forwarding for 4.1x kernels]

[Mike Snitzer]

On Fri, Mar 23 2018 at 10:47am -0400,
Denis Semakin <d.semakin@omprussia.ru> wrote:

> >I'd feel safer about having targets opt-in with setting (a new)
> >ti->num_secure_erase_bios.
> May be add a new field "bool secure_erase_supported:1" in dm_target structure instead?
> And set up it "true" in constructor for linear targets.

No, that type of change is for a DM target to support a feature even if
the underlying device doesn't.

As my previous email elaborated, ti->num_secure_erase_bios is the right
way forward.

Mike

Re: dm table: add support for secure erase forwarding [was: Re: Adaptation secure erase forwarding for 4.1x kernels]

[Denis Semakin]

Hi, sorry for my misunderstanding.


>Thinking further, it is absolutely needed, otherwise dm-stripe.c won't
>be able to use the optimization I mentioned above.  In addition, other
>targets shouldn't be getting REQ_OP_SECURE_ERASE unless there is a real
>need.  ti->num_secure_erase_bios will allow for a much safer and
>controlled rollout of this support.

Ok. I got it.

>As my previous email elaborated, ti->num_secure_erase_bios is the right
>way forward.

Then a patch for this may look like:

From 2b9dfd5af813023cdaded4d91b4c25cec8a1783d Mon Sep 17 00:00:00 2001
From: Denis Semakin <d.semakin@omprussia.ru>
Date: Mon, 26 Mar 2018 10:44:14 +0300
Subject: [PATCH] Add num_secure_erase_bios field

Signed-off-by: Denis Semakin <d.semakin@omprussia.ru>
---
 drivers/md/dm-linear.c        | 1 +
 drivers/md/dm-table.c         | 3 +++
 include/linux/device-mapper.h | 2 ++
 3 files changed, 6 insertions(+)

diff --git a/drivers/md/dm-linear.c b/drivers/md/dm-linear.c
index d5f8eff..ff751b0 100644
--- a/drivers/md/dm-linear.c
+++ b/drivers/md/dm-linear.c
@@ -59,6 +59,7 @@ static int linear_ctr(struct dm_target *ti, unsigned int argc, char **argv)

        ti->num_flush_bios = 1;
        ti->num_discard_bios = 1;
+       ti->num_secure_erase_bios = 1;
        ti->num_write_same_bios = 1;
        ti->num_write_zeroes_bios = 1;
        ti->private = lc;
diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
index d857369..0acffdb 100644
--- a/drivers/md/dm-table.c
+++ b/drivers/md/dm-table.c
@@ -1863,6 +1863,9 @@ static bool dm_table_supports_secure_erase(struct dm_table *t)
        for (i = 0; i < dm_table_get_num_targets(t); i++) {
                ti = dm_table_get_target(t, i);

+              if (!ti->num_secure_erase_bios)
+                      return false;
+
                if (!ti->type->iterate_devices ||
                    ti->type->iterate_devices(ti, device_not_secure_erase_capable, NULL))
                       return false;
diff --git a/include/linux/device-mapper.h b/include/linux/device-mapper.h
index 1e2426c..233ac8f 100644
--- a/include/linux/device-mapper.h
+++ b/include/linux/device-mapper.h
@@ -267,6 +267,8 @@ struct dm_target {
         */
        unsigned num_discard_bios;

+       unsigned num_secure_erase_bios;
+
        /*
         * The number of WRITE SAME bios that will be submitted to the target.
         * The bio number can be accessed with dm_bio_get_target_bio_nr.
-- 
1.9.1

What do you think?

Denis.

Re: dm table: add support for secure erase forwarding [was: Re: Adaptation secure erase forwarding for 4.1x kernels]

[Denis Semakin]

Hi. Here is a full patch for adding num_secure_erase_bios field in struct dm_targer for linear DMs
and QUEUE_FLAG_SECERASE forwarding.
Could you please review.

Thanks.

diff --git a/drivers/md/dm-linear.c b/drivers/md/dm-linear.c
index d5f8eff..ff751b0 100644
--- a/drivers/md/dm-linear.c
+++ b/drivers/md/dm-linear.c
@@ -59,6 +59,7 @@ static int linear_ctr(struct dm_target *ti, unsigned int argc, char **argv)

        ti->num_flush_bios = 1;
        ti->num_discard_bios = 1;
+       ti->num_secure_erase_bios = 1;
        ti->num_write_same_bios = 1;
        ti->num_write_zeroes_bios = 1;
        ti->private = lc;
diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
index 7eb3e2a..13a2c23 100644
--- a/drivers/md/dm-table.c
+++ b/drivers/md/dm-table.c
@@ -1846,6 +1846,36 @@ static bool dm_table_supports_discards(struct dm_table *t)
        return true;
 }

+static int device_not_secerase_capable(struct dm_target *ti,
+                                          struct dm_dev *dev, sector_t start,
+                                          sector_t len, void *data)
+{
+       struct request_queue *q = bdev_get_queue(dev->bdev);
+
+       return q && !blk_queue_secure_erase(q);
+}
+
+static bool dm_targets_support_secure_erase(struct dm_table *t)
+{
+       unsigned int i = 0;
+       struct dm_target *ti;
+
+       while (i < dm_table_get_num_targets(t)) {
+               ti = dm_table_get_target(t, i++);
+
+               if (!ti->num_secure_erase_bios)
+                       return false;
+
+               if (!ti->type->iterate_devices ||
+                   ti->type->iterate_devices(ti, device_not_secerase_capable,
+                                             NULL))
+               return false;
+       }
+
+       return true;
+
+}
+
 void dm_table_set_restrictions(struct dm_table *t, struct request_queue *q,
                               struct queue_limits *limits)
 {
@@ -1867,6 +1897,9 @@ void dm_table_set_restrictions(struct dm_table *t, struct request_queue *q,
        } else
                queue_flag_set_unlocked(QUEUE_FLAG_DISCARD, q);

+       if (dm_targets_support_secure_erase(t))
+               queue_flag_set_unlocked(QUEUE_FLAG_SECERASE, q);
+
        if (dm_table_supports_flush(t, (1UL << QUEUE_FLAG_WC))) {
                wc = true;
                if (dm_table_supports_flush(t, (1UL << QUEUE_FLAG_FUA)))
diff --git a/include/linux/device-mapper.h b/include/linux/device-mapper.h
index 1e2426c..233ac8f 100644
--- a/include/linux/device-mapper.h
+++ b/include/linux/device-mapper.h
@@ -267,6 +267,8 @@ struct dm_target {
         */
        unsigned num_discard_bios;

+       unsigned num_secure_erase_bios;
+
        /*
         * The number of WRITE SAME bios that will be submitted to the target.
         * The bio number can be accessed with dm_bio_get_target_bio_nr.

Re: dm table: add support for secure erase forwarding [was: Re: Adaptation secure erase forwarding for 4.1x kernels]

[Denis Semakin]

Also, will it be usefully to add a "get...()" function for
num_secure_erase_bios? Let's say get_num_secure_erase_bios(struct dm_target &ti);
As it's done for num_discard_bios...

----- Исходное сообщение -----
От: "snitzer" <snitzer@redhat.com>
Кому: "Denis Semakin" 
Копия: "dm-devel" <dm-devel@redhat.com>
Отправленные: Пятница, 23 Март 2018 г 18:38:44
Тема: Re: dm table: add support for secure erase forwarding [was: Re: Adaptation secure erase forwarding for 4.1x kernels]

On Fri, Mar 23 2018 at 10:47am -0400,
Denis Semakin  wrote:

> >I'd feel safer about having targets opt-in with setting (a new)
> >ti->num_secure_erase_bios.
> May be add a new field "bool secure_erase_supported:1" in dm_target structure instead?
> And set up it "true" in constructor for linear targets.

No, that type of change is for a DM target to support a feature even if
the underlying device doesn't.

As my previous email elaborated, ti->num_secure_erase_bios is the right
way forward.

Mike
-- 
Best regards,

Denis Semakin
Software Developer
Open Mobile Platform

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: dm table: add support for secure erase forwarding [was: Re: Adaptation secure erase forwarding for 4.1x kernels]

[Mike Snitzer]

On Mon, Mar 26 2018 at 10:12am -0400,
Denis Semakin <d.semakin@omprussia.ru> wrote:

> Also, will it be usefully to add a "get...()" function for
> num_secure_erase_bios? Let's say get_num_secure_erase_bios(struct dm_target &ti);
> As it's done for num_discard_bios...

Yes, it is required.  I'll share a link to the final patch shortly, I've
still attributed the work to you.

BTW, please look into your text editor's settings: seems your patches
are using spaces rather than tabs (and you aren't even using 8 spaces?)

Mike

Re: dm table: add support for secure erase forwarding [was: Re: Adaptation secure erase forwarding for 4.1x kernels]

[Denis Semakin]

Hi.
>> Also, will it be usefully to add a "get...()" function for
>> num_secure_erase_bios? Let's say get_num_secure_erase_bios(struct dm_target &ti);
>> As it's done for num_discard_bios...
>
>Yes, it is required.  I'll share a link to the final patch shortly, I've
>still attributed the work to you.

Thanks.
I've already seen your final patch in device-mapper git repository
and I have nothing to add from my side.

>
>BTW, please look into your text editor's settings: seems your patches
>are using spaces rather than tabs (and you aren't even using 8 spaces?)

It's not blame of editor (I prefer to use git), it's email client re-formats a message.
Next time I'll do it via git send-mail.

>Mike
-- 
Best regards,

Denis

Re: dm table: add support for secure erase forwarding [was: Re: Adaptation secure erase forwarding for 4.1x kernels]

[Denis Semakin]

>> 
>> BTW, please look into your text editor's settings: seems your patches
>> are using spaces rather than tabs (and you aren't even using 8 spaces?)
> 

> It's not blame of editor (I prefer to use git), it's email client re-formats a message.
I prefer to use vim of course

> Next time I'll do it via git send-mail.

>> Mike
> -- 
> Best regards,
> 
> Denis
-- 
Best regards,

Denis

Re: [PATCH] dm table: add support for secure erase forwarding [was: Re: Adaptation secure erase forwarding for 4.1x kernels]

[Denis Semakin]

>I'd feel safer about having targets opt-in with setting (a new)
>ti->num_secure_erase_bios.
May be add a new field "bool secure_erase_supported:1" in dm_target structure instead?
And set up it "true" in constructor for linear targets.

Denis

Re: [PATCH] dm table: add support for secure erase forwarding [was: Re: Adaptation secure erase forwarding for 4.1x kernels]

[Denis Semakin]

Additional.

>Which DM target(s) have you been wanting to pass REQ_OP_SECURE_ERASE
>bios?
I've tested only with linear targets, so I suppose it's needed to
check what kind of target(s) we have.

Denis

Re: [PATCH] dm table: add support for secure erase forwarding [was: Re: Adaptation secure erase forwarding for 4.1x kernels]

[Denis Semakin]

Hi.
Soon or later everybody start to think about security.
One of the most frequently requirement is 100% reliable data deletion from
any device in case of compromising or loss or theft.

For this, drive and memory cards manufacturers provide ERASE and TRIM features
which can notice (inform) controller of the device to erase sectors
(write down only zeros or one or random data). Features can be triggered
by calling ioctl() requests or a mount options (like ext4 does). But this works
only with whole device -- /dev/sdX, /dev/mmcblk0pX...
But what if for some security reasons we need to secure delete a single file.
A file-system layer provide this functionality... one may call __blkdev_issue_discard()
with BLKDEV_DISCARD_SECURE flag.
But...
All this works well if there is no virtual layer (like device-mapper)
between file-system and block-layer, because if device driver supports
this feature it can set up related flag in request_queue flags.

I have ext4 lvm partitions on my test instance and a drive which can
secure erase sectors.
Without lvm it works, with lvm it doesn't.
That's the purpose if this patch - to provide the opportunity to secure erase
given sectors (through device-mapper layer, forward request) that were assigned for regular file.



>But I'm left skeptical that this is enough.  Don't targets need to
>explicitly handle these REQ_OP_SECURE_ERASE requests?  Similar to how
>REQ_OP_DISCARD is handled?

I think yes, REQ_OP_DISCARD will not secure erase the data and it can be possible
to get it from device.

>I'd feel safer about having targets opt-in with setting (a new)
>ti->num_secure_erase_bios.

Well... May it make sense but I didn't see any reasons to add it in patch.

>Which DM target(s) have you been wanting to pass REQ_OP_SECURE_ERASE
>bios?
I think first of all a linear target of course should have this. For others I'm not sure, I need
to investigate.

Hopefully, I answered to all your question.


Denis