RE: [PATCH 1/2] block:hdev: support BLKSECDISCARD
> What is the use case for exposing secure erase in qemu? The whole concept for > a LBA based secure erase is generally not a very smart idea for flash based > media.. Hi, Christoph We got a user requirement: support BLKSECDISCARD in VM. Which is: ioctl(BLKSECDISCARD) in guest -> qemu backend perform secure discard on native block device accordingly. This is a requirement to meet the security level of the project. So I sent out these patches to see whether qemu could accept such changes.
Re: [PATCH 1/2] block:hdev: support BLKSECDISCARD
On Tue, Nov 16, 2021 at 09:53:39PM -0800, Christoph Hellwig wrote: > On Tue, Nov 16, 2021 at 10:58:30AM +, Stefan Hajnoczi wrote: > > Question for Jens and Christoph: > > > > Is there a way for userspace to detect whether a Linux block device > > supports SECDISCARD? > > I don't know of one. > > > If not, then maybe a new sysfs attribute can be added: > > This looks correct, but if we start looking into SECDISCARD seriously > I'd like to split the max_sectors settings for it from discard as that > is currently a bit of a mess. If we then expose the secure erase max > sectors in sysfs that would also be a good indicator. That is probably better because userspace would the queue limits information too. Thanks! > What is the use case for exposing secure erase in qemu? The whole > concept for a LBA based secure erase is generally not a very smart > idea for flash based media.. Yadong, please see Christoph's question above. Stefan signature.asc Description: PGP signature
Re: [PATCH 1/2] block:hdev: support BLKSECDISCARD
On Tue, Nov 16, 2021 at 10:58:30AM +, Stefan Hajnoczi wrote: > Question for Jens and Christoph: > > Is there a way for userspace to detect whether a Linux block device > supports SECDISCARD? I don't know of one. > If not, then maybe a new sysfs attribute can be added: This looks correct, but if we start looking into SECDISCARD seriously I'd like to split the max_sectors settings for it from discard as that is currently a bit of a mess. If we then expose the secure erase max sectors in sysfs that would also be a good indicator. What is the use case for exposing secure erase in qemu? The whole concept for a LBA based secure erase is generally not a very smart idea for flash based media..
Re: [PATCH 1/2] block:hdev: support BLKSECDISCARD
On Tue, Nov 16, 2021 at 02:03:15AM +, Qi, Yadong wrote: > > > Add a new option "secdiscard" for block drive. Parse it and record it > > > in bs->open_flags as bit(BDRV_O_SECDISCARD). > > > > > > Add a new BDRV_REQ_SECDISCARD bit for secure discard request from > > > virtual device. > > > > > > For host_device backend: implement by ioctl(BLKSECDISCARD) on real > > > host block device. > > > For other backend, no implementation. > > > > > > E.g.: > > > qemu-system-x86_64 \ > > > ... \ > > > -drive > > file=/dev/mmcblk0p2,if=none,format=raw,discard=on,secdiscard=on,id=sd0 \ > > > -device virtio-blk-pci,drive=sd0,id=sd0_vblk \ > > > ... > > > > I'm curious why there is explicit control over this feature (-drive > > secdiscard=on|off). For example, is there a reason why users would want to > > disable secdiscard on a drive that supports it? I guess there is no way to > > emulate > > it correctly so secdiscard=on on a drive that doesn't support it produces > > an error? > > Yes. AFAIK, there is no way to emulate a "secure" discard. But it seems also > hard to > detect whether a host drive support secdiscard unless it really perform a real > secdiscard. So I choose to add an option for user to enable it for virtual > block. > There is an assumption that user knows whether host support secdiscard. Question for Jens and Christoph: Is there a way for userspace to detect whether a Linux block device supports SECDISCARD? If not, then maybe a new sysfs attribute can be added: diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c index cef1f713370b..c946ef49ac15 100644 --- a/block/blk-sysfs.c +++ b/block/blk-sysfs.c @@ -304,6 +304,7 @@ QUEUE_SYSFS_BIT_FNS(nonrot, NONROT, 1); QUEUE_SYSFS_BIT_FNS(random, ADD_RANDOM, 0); QUEUE_SYSFS_BIT_FNS(iostats, IO_STAT, 0); QUEUE_SYSFS_BIT_FNS(stable_writes, STABLE_WRITES, 0); +QUEUE_SYSFS_BIT_FNS(secure_erase, SECERASE, 0); #undef QUEUE_SYSFS_BIT_FNS static ssize_t queue_zoned_show(struct request_queue *q, char *page) Stefan signature.asc Description: PGP signature
RE: [PATCH 1/2] block:hdev: support BLKSECDISCARD
> > Add a new option "secdiscard" for block drive. Parse it and record it > > in bs->open_flags as bit(BDRV_O_SECDISCARD). > > > > Add a new BDRV_REQ_SECDISCARD bit for secure discard request from > > virtual device. > > > > For host_device backend: implement by ioctl(BLKSECDISCARD) on real > > host block device. > > For other backend, no implementation. > > > > E.g.: > > qemu-system-x86_64 \ > > ... \ > > -drive > file=/dev/mmcblk0p2,if=none,format=raw,discard=on,secdiscard=on,id=sd0 \ > > -device virtio-blk-pci,drive=sd0,id=sd0_vblk \ > > ... > > I'm curious why there is explicit control over this feature (-drive > secdiscard=on|off). For example, is there a reason why users would want to > disable secdiscard on a drive that supports it? I guess there is no way to > emulate > it correctly so secdiscard=on on a drive that doesn't support it produces an > error? Yes. AFAIK, there is no way to emulate a "secure" discard. But it seems also hard to detect whether a host drive support secdiscard unless it really perform a real secdiscard. So I choose to add an option for user to enable it for virtual block. There is an assumption that user knows whether host support secdiscard. Best Regard Yadong
RE: [PATCH 1/2] block:hdev: support BLKSECDISCARD
>
> Notably absent: qapi/block-core.json. Without changing this, the option can't
> be
> available in -blockdev, which is the primary option to configure block device
> backends.
>
> This patch seems to contain multiple logical changes that should be split into
> separate patches:
>
> * Adding a flags parameter to .bdrv_co_pdiscard
>
> * Support for the new feature in the core block layer (should be done
> with -blockdev)
>
> * Convenience magic for -drive (BDRV_O_SECDISCARD). It's not clear that
> this should be done at all because the option is really specific to
> one single block driver (file-posix). I think in your patch, all
> other block drivers silently ignore the option, which is not what we
> want.
Sorry for the absent for -blockdev. Will try add that.
Also I will try to split the patches.
And for the BDRV_O_SECDISCARD, it is specific for file-posix.c(host_device).
Maybe
I need to add the option only for file-posix.c.
>
> > diff --git a/block.c b/block.c
> > index 580cb77a70..4f05e96d12 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -1128,6 +1128,32 @@ int bdrv_parse_discard_flags(const char *mode,
> int *flags)
> > return 0;
> > }
> >
> > +/**
> > + * Set open flags for a given secdiscard mode
> > + *
> > + * Return 0 on success, -1 if the secdiscard mode was invalid.
> > + */
> > +int bdrv_parse_secdiscard_flags(const char *mode, int *flags, Error
> > +**errp) {
> > +*flags &= ~BDRV_O_SECDISCARD;
> > +
> > +if (!strcmp(mode, "off")) {
> > +/* do nothing */
> > +} else if (!strcmp(mode, "on")) {
> > +if (!(*flags & BDRV_O_UNMAP)) {
> > +error_setg(errp, "cannot enable secdiscard when discard is "
> > + "disabled!");
> > +return -1;
> > +}
>
> This check has nothing to do with parsing the option, it's validating its
> value.
>
> You don't even need a new function to parse it, because there is already
> qemu_opt_get_bool(). Duplicating it means only that you're inconsistent with
> other boolean options, which alternatively accept "yes"/"no", "true"/"false",
> "y/n".
Sure. Will use qemu_opt_get_bool() instead.
>
> > +
> > +*flags |= BDRV_O_SECDISCARD;
> > +} else {
> > +return -1;
> > +}
> > +
> > +return 0;
> > +}
> > +
> > /**
> > * Set open flags for a given cache mode
> > *
> > @@ -1695,6 +1721,11 @@ QemuOptsList bdrv_runtime_opts = {
> > .type = QEMU_OPT_STRING,
> > .help = "discard operation (ignore/off, unmap/on)",
> > },
> > +{
> > +.name = BDRV_OPT_SECDISCARD,
> > +.type = QEMU_OPT_STRING,
> > +.help = "secure discard operation (off, on)",
> > +},
> > {
> > .name = BDRV_OPT_FORCE_SHARE,
> > .type = QEMU_OPT_BOOL,
> > @@ -1735,6 +1766,7 @@ static int bdrv_open_common(BlockDriverState *bs,
> BlockBackend *file,
> > const char *driver_name = NULL;
> > const char *node_name = NULL;
> > const char *discard;
> > +const char *secdiscard;
> > QemuOpts *opts;
> > BlockDriver *drv;
> > Error *local_err = NULL;
> > @@ -1829,6 +1861,16 @@ static int bdrv_open_common(BlockDriverState
> *bs, BlockBackend *file,
> > }
> > }
> >
> > +
> > +secdiscard = qemu_opt_get(opts, BDRV_OPT_SECDISCARD);
> > +if (secdiscard != NULL) {
> > +if (bdrv_parse_secdiscard_flags(secdiscard, >open_flags,
> > +errp) != 0) {
> > +ret = -EINVAL;
> > +goto fail_opts;
> > +}
> > +}
> > +
> > bs->detect_zeroes =
> > bdrv_parse_detect_zeroes(opts, bs->open_flags, _err);
> > if (local_err) {
> > @@ -3685,6 +3727,10 @@ static BlockDriverState *bdrv_open_inherit(const
> char *filename,
> > , options, flags, options);
> > }
> >
> > +if (g_strcmp0(qdict_get_try_str(options, BDRV_OPT_SECDISCARD), "on")) {
> > +flags |= BDRV_O_SECDISCARD;
>
> Indentation is off.
Will fix it.
>
> > --- a/block/file-posix.c
> > +++ b/block/file-posix.c
> > @@ -160,6 +160,7 @@ typedef struct BDRVRawState {
> > bool is_xfs:1;
> > #endif
> > bool has_discard:1;
> > +bool has_secdiscard:1;
> > bool has_write_zeroes:1;
> > bool discard_zeroes:1;
> > bool use_linux_aio:1;
>
> has_secdiscard is only set to false in raw_open_common() and never changed or
> used.
Will remove it.
>
> > @@ -727,6 +728,7 @@ static int raw_open_common(BlockDriverState *bs,
> > QDict *options, #endif /* !defined(CONFIG_LINUX_IO_URING) */
> >
> > s->has_discard = true;
> > +s->has_secdiscard = false;
> > s->has_write_zeroes = true;
> > if ((bs->open_flags & BDRV_O_NOCACHE) != 0 && !dio_byte_aligned(s->fd))
> {
> > s->needs_alignment = true;
> > @@ -765,6 +767,7 @@ static int raw_open_common(BlockDriverState *bs,
> QDict *options,
> >
Re: [PATCH 1/2] block:hdev: support BLKSECDISCARD
On Mon, Nov 15, 2021 at 12:51:59PM +0800, yadong...@intel.com wrote: > From: Yadong Qi > > Add a new option "secdiscard" for block drive. Parse it and > record it in bs->open_flags as bit(BDRV_O_SECDISCARD). > > Add a new BDRV_REQ_SECDISCARD bit for secure discard request > from virtual device. > > For host_device backend: implement by ioctl(BLKSECDISCARD) on > real host block device. > For other backend, no implementation. > > E.g.: > qemu-system-x86_64 \ > ... \ > -drive > file=/dev/mmcblk0p2,if=none,format=raw,discard=on,secdiscard=on,id=sd0 \ > -device virtio-blk-pci,drive=sd0,id=sd0_vblk \ > ... I'm curious why there is explicit control over this feature (-drive secdiscard=on|off). For example, is there a reason why users would want to disable secdiscard on a drive that supports it? I guess there is no way to emulate it correctly so secdiscard=on on a drive that doesn't support it produces an error? signature.asc Description: PGP signature
Re: [PATCH 1/2] block:hdev: support BLKSECDISCARD
Am 15.11.2021 um 05:51 hat yadong...@intel.com geschrieben:
> From: Yadong Qi
>
> Add a new option "secdiscard" for block drive. Parse it and
> record it in bs->open_flags as bit(BDRV_O_SECDISCARD).
>
> Add a new BDRV_REQ_SECDISCARD bit for secure discard request
> from virtual device.
>
> For host_device backend: implement by ioctl(BLKSECDISCARD) on
> real host block device.
> For other backend, no implementation.
>
> E.g.:
> qemu-system-x86_64 \
> ... \
> -drive
> file=/dev/mmcblk0p2,if=none,format=raw,discard=on,secdiscard=on,id=sd0 \
> -device virtio-blk-pci,drive=sd0,id=sd0_vblk \
> ...
>
> Signed-off-by: Yadong Qi
> ---
> block.c | 46 +
> block/blkdebug.c | 5 ++--
> block/blklogwrites.c | 6 ++--
> block/blkreplay.c| 5 ++--
> block/block-backend.c| 15 ++
> block/copy-before-write.c| 5 ++--
> block/copy-on-read.c | 5 ++--
> block/coroutines.h | 6 ++--
> block/file-posix.c | 50
> block/filter-compress.c | 5 ++--
> block/io.c | 5 ++--
> block/mirror.c | 5 ++--
> block/nbd.c | 3 +-
> block/nvme.c | 3 +-
> block/preallocate.c | 5 ++--
> block/qcow2-refcount.c | 4 +--
> block/qcow2.c| 3 +-
> block/raw-format.c | 5 ++--
> block/throttle.c | 5 ++--
> hw/block/virtio-blk.c| 2 +-
> hw/ide/core.c| 1 +
> hw/nvme/ctrl.c | 3 +-
> hw/scsi/scsi-disk.c | 2 +-
> include/block/block.h| 13 +++--
> include/block/block_int.h| 2 +-
> include/block/raw-aio.h | 4 ++-
> include/sysemu/block-backend.h | 1 +
> tests/unit/test-block-iothread.c | 9 +++---
> 28 files changed, 171 insertions(+), 52 deletions(-)
Notably absent: qapi/block-core.json. Without changing this, the option
can't be available in -blockdev, which is the primary option to configure
block device backends.
This patch seems to contain multiple logical changes that should be
split into separate patches:
* Adding a flags parameter to .bdrv_co_pdiscard
* Support for the new feature in the core block layer (should be done
with -blockdev)
* Convenience magic for -drive (BDRV_O_SECDISCARD). It's not clear that
this should be done at all because the option is really specific to
one single block driver (file-posix). I think in your patch, all
other block drivers silently ignore the option, which is not what we
want.
> diff --git a/block.c b/block.c
> index 580cb77a70..4f05e96d12 100644
> --- a/block.c
> +++ b/block.c
> @@ -1128,6 +1128,32 @@ int bdrv_parse_discard_flags(const char *mode, int
> *flags)
> return 0;
> }
>
> +/**
> + * Set open flags for a given secdiscard mode
> + *
> + * Return 0 on success, -1 if the secdiscard mode was invalid.
> + */
> +int bdrv_parse_secdiscard_flags(const char *mode, int *flags, Error **errp)
> +{
> +*flags &= ~BDRV_O_SECDISCARD;
> +
> +if (!strcmp(mode, "off")) {
> +/* do nothing */
> +} else if (!strcmp(mode, "on")) {
> +if (!(*flags & BDRV_O_UNMAP)) {
> +error_setg(errp, "cannot enable secdiscard when discard is "
> + "disabled!");
> +return -1;
> +}
This check has nothing to do with parsing the option, it's validating
its value.
You don't even need a new function to parse it, because there is already
qemu_opt_get_bool(). Duplicating it means only that you're inconsistent
with other boolean options, which alternatively accept "yes"/"no",
"true"/"false", "y/n".
> +
> +*flags |= BDRV_O_SECDISCARD;
> +} else {
> +return -1;
> +}
> +
> +return 0;
> +}
> +
> /**
> * Set open flags for a given cache mode
> *
> @@ -1695,6 +1721,11 @@ QemuOptsList bdrv_runtime_opts = {
> .type = QEMU_OPT_STRING,
> .help = "discard operation (ignore/off, unmap/on)",
> },
> +{
> +.name = BDRV_OPT_SECDISCARD,
> +.type = QEMU_OPT_STRING,
> +.help = "secure discard operation (off, on)",
> +},
> {
> .name = BDRV_OPT_FORCE_SHARE,
> .type = QEMU_OPT_BOOL,
> @@ -1735,6 +1766,7 @@ static int bdrv_open_common(BlockDriverState *bs,
> BlockBackend *file,
> const char *driver_name = NULL;
> const char *node_name = NULL;
> const char *discard;
> +const char *secdiscard;
> QemuOpts *opts;
> BlockDriver *drv;
> Error *local_err = NULL;
> @@ -1829,6 +1861,16 @@ static int bdrv_open_common(BlockDriverState *bs,
> BlockBackend *file,
> }
> }
>
> +
> +secdiscard = qemu_opt_get(opts, BDRV_OPT_SECDISCARD);
> +if (secdiscard
