| Message ID | 20180322151045.GA27598@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Mike Snitzer |
| Headers | show |
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 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
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 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
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
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 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
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
>> >> 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
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)))