Re: [PATCH v3 2/5] virtio-pmem: Add virtio pmem driver
> > This patch adds virtio-pmem driver for KVM guest.
> >
> > Guest reads the persistent memory range information from
> > Qemu over VIRTIO and registers it on nvdimm_bus. It also
> > creates a nd_region object with the persistent memory
> > range information so that existing 'nvdimm/pmem' driver
> > can reserve this into system memory map. This way
> > 'virtio-pmem' driver uses existing functionality of pmem
> > driver to register persistent memory compatible for DAX
> > capable filesystems.
> >
> > This also provides function to perform guest flush over
> > VIRTIO from 'pmem' driver when userspace performs flush
> > on DAX memory range.
> >
> > Signed-off-by: Pankaj Gupta
> > ---
> > drivers/nvdimm/virtio_pmem.c | 84 ++
> > drivers/virtio/Kconfig | 10
> > drivers/virtio/Makefile | 1 +
> > drivers/virtio/pmem.c| 124
> > +++
> > include/linux/virtio_pmem.h | 60 +++
> > include/uapi/linux/virtio_ids.h | 1 +
> > include/uapi/linux/virtio_pmem.h | 10
>
> As with any uapi change, you need to CC the virtio dev
> mailing list (subscribers only, sorry about that).
Sure, will add virtio dev mailing list while sending v4.
Thanks,
Pankaj
>
>
> > 7 files changed, 290 insertions(+)
> > create mode 100644 drivers/nvdimm/virtio_pmem.c
> > create mode 100644 drivers/virtio/pmem.c
> > create mode 100644 include/linux/virtio_pmem.h
> > create mode 100644 include/uapi/linux/virtio_pmem.h
> >
> > diff --git a/drivers/nvdimm/virtio_pmem.c b/drivers/nvdimm/virtio_pmem.c
> > new file mode 100644
> > index 000..2a1b1ba
> > --- /dev/null
> > +++ b/drivers/nvdimm/virtio_pmem.c
> > @@ -0,0 +1,84 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * virtio_pmem.c: Virtio pmem Driver
> > + *
> > + * Discovers persistent memory range information
> > + * from host and provides a virtio based flushing
> > + * interface.
> > + */
> > +#include
> > +#include "nd.h"
> > +
> > + /* The interrupt handler */
> > +void host_ack(struct virtqueue *vq)
> > +{
> > + unsigned int len;
> > + unsigned long flags;
> > + struct virtio_pmem_request *req, *req_buf;
> > + struct virtio_pmem *vpmem = vq->vdev->priv;
> > +
> > + spin_lock_irqsave(&vpmem->pmem_lock, flags);
> > + while ((req = virtqueue_get_buf(vq, &len)) != NULL) {
> > + req->done = true;
> > + wake_up(&req->host_acked);
> > +
> > + if (!list_empty(&vpmem->req_list)) {
> > + req_buf = list_first_entry(&vpmem->req_list,
> > + struct virtio_pmem_request, list);
> > + list_del(&vpmem->req_list);
> > + req_buf->wq_buf_avail = true;
> > + wake_up(&req_buf->wq_buf);
> > + }
> > + }
> > + spin_unlock_irqrestore(&vpmem->pmem_lock, flags);
> > +}
> > +EXPORT_SYMBOL_GPL(host_ack);
> > +
> > + /* The request submission function */
> > +int virtio_pmem_flush(struct nd_region *nd_region)
> > +{
> > + int err;
> > + unsigned long flags;
> > + struct scatterlist *sgs[2], sg, ret;
> > + struct virtio_device *vdev = nd_region->provider_data;
> > + struct virtio_pmem *vpmem = vdev->priv;
> > + struct virtio_pmem_request *req;
> > +
> > + might_sleep();
> > + req = kmalloc(sizeof(*req), GFP_KERNEL);
> > + if (!req)
> > + return -ENOMEM;
> > +
> > + req->done = req->wq_buf_avail = false;
> > + strcpy(req->name, "FLUSH");
> > + init_waitqueue_head(&req->host_acked);
> > + init_waitqueue_head(&req->wq_buf);
> > + sg_init_one(&sg, req->name, strlen(req->name));
> > + sgs[0] = &sg;
> > + sg_init_one(&ret, &req->ret, sizeof(req->ret));
> > + sgs[1] = &ret;
> > +
> > + spin_lock_irqsave(&vpmem->pmem_lock, flags);
> > + err = virtqueue_add_sgs(vpmem->req_vq, sgs, 1, 1, req, GFP_ATOMIC);
> > + if (err) {
> > + dev_err(&vdev->dev, "failed to send command to virtio pmem
> > device\n");
> > +
> > + list_add_tail(&vpmem->req_list, &req->list);
> > + spin_unlock_irqrestore(&vpmem->pmem_lock, flags);
> > +
> > + /* When host has read buffer, this completes via host_ack */
> > + wait_event(req->wq_buf, req->wq_buf_avail);
> > + spin_lock_irqsave(&vpmem->pmem_lock, flags);
> > + }
> > + virtqueue_kick(vpmem->req_vq);
> > + spin_unlock_irqrestore(&vpmem->pmem_lock, flags);
> > +
> > + /* When host has read buffer, this completes via host_ack */
> > + wait_event(req->host_acked, req->done);
> > + err = req->ret;
> > + kfree(req);
> > +
> > + return err;
> > +};
> > +EXPORT_SYMBOL_GPL(virtio_pmem_flush);
> > +MODULE_LICENSE("GPL");
> > diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
> > index 3589764..9f634a2 100644
> > --- a/drivers/virtio/Kconfig
> > +++ b/drivers/virtio/Kconfig
> > @@ -42,6 +42,16 @@ config VIRTIO_PCI_LEGACY
> >
> > If unsure, say Y.
>
Re: [Qemu-devel] [PATCH v3 0/5] kvm "virtio pmem" device
> > > > > > > > On Mon, Jan 14, 2019 at 02:15:40AM -0500, Pankaj Gupta wrote: > > > > > > > > > > > > Until you have images (and hence host page cache) shared between > > > > > > > multiple guests. People will want to do this, because it means > > > > > > > they > > > > > > > only need a single set of pages in host memory for executable > > > > > > > binaries rather than a set of pages per guest. Then you have > > > > > > > multiple guests being able to detect residency of the same set of > > > > > > > pages. If the guests can then, in any way, control eviction of > > > > > > > the > > > > > > > pages from the host cache, then we have a guest-to-guest > > > > > > > information > > > > > > > leak channel. > > > > > > > > > > > > I don't think we should ever be considering something that would > > > > > > allow a > > > > > > guest to evict page's from the host's pagecache [1]. The guest > > > > > > should > > > > > > be able to kick its own references to the host's pagecache out of > > > > > > its > > > > > > own pagecache, but not be able to influence whether the host or > > > > > > another > > > > > > guest has a read-only mapping cached. > > > > > > > > > > > > [1] Unless the guest is allowed to modify the host's file; > > > > > > obviously > > > > > > truncation, holepunching, etc are going to evict pages from the > > > > > > host's > > > > > > page cache. > > > > > > > > > > This is so correct. Guest does not not evict host page cache pages > > > > > directly. > > > > > > > > They don't right now. > > > > > > > > But someone is going to end up asking for discard to work so that > > > > the guest can free unused space in the underlying spares image (i.e. > > > > make use of fstrim or mount -o discard) because they have workloads > > > > that have bursts of space usage and they need to trim the image > > > > files afterwards to keep their overall space usage under control. > > > > > > > > And then > > > > > > ...we reject / push back on that patch citing the above concern. > > > > So at what point do we draw the line? > > > > We're allowing writable DAX mappings, but as I've pointed out that > > means we are going to be allowing a potential information leak via > > files with shared extents to be directly mapped and written to. > > > > But we won't allow useful admin operations that allow better > > management of host side storage space similar to how normal image > > files are used by guests because it's an information leak vector? > > > > That's splitting some really fine hairs there... > > May I summarize that th security implications need to > be documented? > > In fact that would make a fine security implications section > in the device specification. This is a very good suggestion. I will document the security implications in details in device specification with details of what all filesystem features we don't support and why. Best regards, Pankaj > > > > > > > > > > In case of virtio-pmem & DAX, guest clears guest page cache > > > > > exceptional entries. > > > > > Its solely decision of host to take action on the host page cache > > > > > pages. > > > > > > > > > > In case of virtio-pmem, guest does not modify host file directly i.e > > > > > don't > > > > > perform hole punch & truncation operation directly on host file. > > > > > > > > ... this will no longer be true, and the nuclear landmine in this > > > > driver interface will have been armed > > > > > > I agree with the need to be careful when / if explicit cache control > > > is added, but that's not the case today. > > > > "if"? > > > > I expect it to be "when", not if. Expect the worst, plan for it now. > > > > Cheers, > > > > Dave. > > -- > > Dave Chinner > > da...@fromorbit.com > > ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH v3 0/5] kvm "virtio pmem" device
> > > On Mon, Jan 14, 2019 at 02:15:40AM -0500, Pankaj Gupta wrote: > > > > > > > > > > Until you have images (and hence host page cache) shared between > > > > > > multiple guests. People will want to do this, because it means they > > > > > > only need a single set of pages in host memory for executable > > > > > > binaries rather than a set of pages per guest. Then you have > > > > > > multiple guests being able to detect residency of the same set of > > > > > > pages. If the guests can then, in any way, control eviction of the > > > > > > pages from the host cache, then we have a guest-to-guest > > > > > > information > > > > > > leak channel. > > > > > > > > > > I don't think we should ever be considering something that would > > > > > allow a > > > > > guest to evict page's from the host's pagecache [1]. The guest > > > > > should > > > > > be able to kick its own references to the host's pagecache out of its > > > > > own pagecache, but not be able to influence whether the host or > > > > > another > > > > > guest has a read-only mapping cached. > > > > > > > > > > [1] Unless the guest is allowed to modify the host's file; obviously > > > > > truncation, holepunching, etc are going to evict pages from the > > > > > host's > > > > > page cache. > > > > > > > > This is so correct. Guest does not not evict host page cache pages > > > > directly. > > > > > > They don't right now. > > > > > > But someone is going to end up asking for discard to work so that > > > the guest can free unused space in the underlying spares image (i.e. > > > make use of fstrim or mount -o discard) because they have workloads > > > that have bursts of space usage and they need to trim the image > > > files afterwards to keep their overall space usage under control. > > > > > > And then > > > > ...we reject / push back on that patch citing the above concern. > > So at what point do we draw the line? > > We're allowing writable DAX mappings, but as I've pointed out that > means we are going to be allowing a potential information leak via > files with shared extents to be directly mapped and written to. > > But we won't allow useful admin operations that allow better > management of host side storage space similar to how normal image > files are used by guests because it's an information leak vector? First of all Thank you for all the useful discussions. I am summarizing here: - We have to live with the limitation to not support fstrim and mount -o discard options with virtio-pmem as they will evict host page cache pages. We cannot allow this for virtio-pmem for security reasons. These filesystem commands will just zero out unused pages currently. - If alot of space is unused and not freed guest can request host Administrator for truncating the host backing image. We are also planning to support qcow2 sparse image format at host side with virtio-pmem. - There is no existing solution for Qemu persistent memory emulation with write support currently. This solution provides us the paravartualized way of emulating persistent memory. It does not emulate of ACPI structures instead it just uses VIRTIO for communication between guest & host. It is fast because of its asynchronous nature and it works well. This makes use of at guest side libnvdimm API's - If disk size freeing problem with guest files trim truncate is very important for users, they can still use real hardware which will provide them both (advance disk features & page cache by pass). Considering all the above reasons I think this feature is useful from virtualization point of view. As Dave rightly said we should be careful and I think now we are careful with the security implications of this device. Thanks again for all the inputs. Best regards, Pankaj > > That's splitting some really fine hairs there... > > > > > In case of virtio-pmem & DAX, guest clears guest page cache exceptional > > > > entries. > > > > Its solely decision of host to take action on the host page cache > > > > pages. > > > > > > > > In case of virtio-pmem, guest does not modify host file directly i.e > > > > don't > > > > perform hole punch & truncation operation directly on host file. > > > > > > ... this will no longer be true, and the nuclear landmine in this > > > driver interface will have been armed > > > > I agree with the need to be careful when / if explicit cache control > > is added, but that's not the case today. > > "if"? > > I expect it to be "when", not if. Expect the worst, plan for it now. > > Cheers, > > Dave. > -- > Dave Chinner > da...@fromorbit.com > ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
[daxctl PATCH v2 0/2] daxctl: Opt-in to /sys/bus/dax ABI
Changes since v1 [1]: * Make the opt-in based on an explicit command rather than an implicit side-effect of installing a new daxctl. * Split the daxctl support from the patch implementing the opt-in * Rebase on command harness reworks and other ndctl cleanups [1]: https://lists.01.org/pipermail/linux-nvdimm/2018-November/018677.html --- Quote patch2: The kernel is implementing a '/sys/bus/dax' ABI to allow for alternate device-DAX drivers to be bound to device instances. While the kernel conversion to '/sys/bus/dax' does not effect the primary ndctl use case of putting namespaces into 'devdax' mode since that uses libnvdimm namespace device relative paths, it does break current implementations of 'ndctl list -X' and 'daxctl list'. It is also known to break fio and some pmdk versions that explicitly reference "/sys/class/dax". In order to avoid userspace regressions the kernel can be configured to maintain '/sys/class/dax' as the default ABI. However, once all '/sys/class/dax' users have been converted, or removed from the installation, an administrator can opt-in to the new '/sys/bus/dax' ABI. The 'dax migrate-device-model' command installs a modprobe rule to blacklist the dax_pmem_compat module and arrange for the dax_pmem module to auto-load in response to the detection of device-DAX instances emitted from the libnvdimm subsystem. --- Dan Williams (2): daxctl: Support the /sys/bus/dax ABI daxctl: Opt-in to /sys/bus/dax ABI .gitignore |1 Documentation/daxctl/Makefile.am |3 + .../daxctl/daxctl-migrate-device-model.txt | 47 + configure.ac |5 + daxctl/Makefile.am | 10 +++ daxctl/builtin.h |1 daxctl/daxctl.c|1 daxctl/lib/Makefile.am |2 + daxctl/lib/daxctl.conf |2 + daxctl/lib/libdaxctl-private.h | 11 +++ daxctl/lib/libdaxctl.c | 70 ++-- daxctl/migrate.c | 41 ndctl.spec.in |1 util/sysfs.c |2 - 14 files changed, 175 insertions(+), 22 deletions(-) create mode 100644 Documentation/daxctl/daxctl-migrate-device-model.txt create mode 100644 daxctl/lib/daxctl.conf create mode 100644 daxctl/migrate.c ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
[daxctl PATCH v2 2/2] daxctl: Opt-in to /sys/bus/dax ABI
The kernel is implementing a '/sys/bus/dax' ABI to allow for alternate device-DAX drivers to be bound to device instances. While the kernel conversion to '/sys/bus/dax' does not effect the primary ndctl use case of putting namespaces into 'devdax' mode since that uses libnvdimm namespace device relative paths, it does break current implementations of 'ndctl list -X' and 'daxctl list'. It is also known to break fio and some pmdk versions that explicitly reference "/sys/class/dax". In order to avoid userspace regressions the kernel can be configured to maintain '/sys/class/dax' as the default ABI. However, once all '/sys/class/dax' users have been converted, or removed from the installation, an administrator can opt-in to the new '/sys/bus/dax' ABI. The 'dax migrate-device-model' command installs a modprobe rule to blacklist the dax_pmem_compat module and arrange for the dax_pmem module to auto-load in response to the detection of device-DAX instances emitted from the libnvdimm subsystem. Reported-by: Jeff Moyer Signed-off-by: Dan Williams --- .gitignore |1 Documentation/daxctl/Makefile.am |3 + .../daxctl/daxctl-migrate-device-model.txt | 47 configure.ac |5 ++ daxctl/Makefile.am | 10 daxctl/builtin.h |1 daxctl/daxctl.c|1 daxctl/lib/Makefile.am |2 + daxctl/lib/daxctl.conf |2 + daxctl/migrate.c | 41 + ndctl.spec.in |1 11 files changed, 113 insertions(+), 1 deletion(-) create mode 100644 Documentation/daxctl/daxctl-migrate-device-model.txt create mode 100644 daxctl/lib/daxctl.conf create mode 100644 daxctl/migrate.c diff --git a/.gitignore b/.gitignore index 5a3d8e4507e5..3ef9ff7a9a2f 100644 --- a/.gitignore +++ b/.gitignore @@ -18,6 +18,7 @@ Documentation/ndctl/asciidoc.conf Documentation/daxctl/asciidoctor-extensions.rb Documentation/ndctl/asciidoctor-extensions.rb .dirstamp +daxctl/config.h daxctl/daxctl daxctl/lib/libdaxctl.la daxctl/lib/libdaxctl.lo diff --git a/Documentation/daxctl/Makefile.am b/Documentation/daxctl/Makefile.am index fc0fbe138d93..6aba035ff543 100644 --- a/Documentation/daxctl/Makefile.am +++ b/Documentation/daxctl/Makefile.am @@ -27,7 +27,8 @@ endif man1_MANS = \ daxctl.1 \ - daxctl-list.1 + daxctl-list.1 \ + daxctl-migrate-device-model.1 CLEANFILES = $(man1_MANS) diff --git a/Documentation/daxctl/daxctl-migrate-device-model.txt b/Documentation/daxctl/daxctl-migrate-device-model.txt new file mode 100644 index ..23e89f1afafc --- /dev/null +++ b/Documentation/daxctl/daxctl-migrate-device-model.txt @@ -0,0 +1,47 @@ +// SPDX-License-Identifier: GPL-2.0 + +daxctl-migrate-device-model(1) +== + +NAME + +daxctl-migrate-device-model - Opt-in to the /sys/bus/dax device-model, +allow for alternative Device-DAX instance drivers. + +SYNOPSIS + +[verse] +'daxctl migrate-device-model' + +Arrange for modprobe to disable the dax_pmem_compat, if present, and +instead deploy the dax_pmem module to convert to the /sys/bus/dax model. +Kernel versions prior to v5.1 may not support /sys/bus/dax in which case +the result of this command is a nop until the kernel is updated. The +motivation for changing from /sys/class/dax to /sys/bus/dax is to allow +for alternative drivers for Device-DAX instances, in particular the +dax_kmem driver. + +By default device-dax publishes a /dev/daxX.Y character device for +userspace to directly map performance differentiated memory. This is +fine if the memory is to be exclusively consumed / managed by userspace. +Alternatively an administrator may want the kernel to manage the memory, +make it available via malloc(), allow for over-provisioning, and / or +apply kernel-based resource control schemes to the memory. In that case +the memory fronted by a given Device-DAX instance can be assigned to the +dax_kmem driver which arranges for the core-kernel memory-management +sub-system to assume management of the memory range. + +This behavior is opt-in for consideration of existing applications / +scripts that may be hard coded to use /sys/class/dax. Fixes have been +submitted to applications known to have these direct dependencies +http://git.kernel.dk/cgit/fio/commit/?id=b08e7d6b18b4[FIO] +https://github.com/pmem/pmdk/commit/91bc8620884e[PMDK], however, there may +be others and a system-owner should be aware of the potential for +regression of Device-DAX consuming scripts, applications, or older +daxctl binaries. + +The modprobe policy established by this utility becomes effective after +the next reboot, or after all DAX related modules have been removed and +r
[daxctl PATCH v2 1/2] daxctl: Support the /sys/bus/dax ABI
The kernel is implementing a '/sys/bus/dax' ABI to allow for alternate
device-DAX drivers to be bound to device instances. In support of this
conversion, teach the libdaxctl subsystem-layout-specific code to parse
the new layout.
For backwards compatibility the implementation transparently and
optionally supports either '/sys/bus/dax' or '/sys/class/dax'.
Signed-off-by: Dan Williams
---
daxctl/lib/libdaxctl-private.h | 11 ++
daxctl/lib/libdaxctl.c | 70 +---
util/sysfs.c |2 +
3 files changed, 62 insertions(+), 21 deletions(-)
diff --git a/daxctl/lib/libdaxctl-private.h b/daxctl/lib/libdaxctl-private.h
index f7667324026f..4a462e7245d2 100644
--- a/daxctl/lib/libdaxctl-private.h
+++ b/daxctl/lib/libdaxctl-private.h
@@ -15,6 +15,17 @@
#define DAXCTL_EXPORT __attribute__ ((visibility("default")))
+enum dax_subsystem {
+ DAX_UNKNOWN,
+ DAX_CLASS,
+ DAX_BUS,
+};
+
+static const char *dax_subsystems[] = {
+ [DAX_CLASS] = "/sys/class/dax",
+ [DAX_BUS] = "/sys/bus/dax/devices",
+};
+
/**
* struct daxctl_region - container for dax_devices
*/
diff --git a/daxctl/lib/libdaxctl.c b/daxctl/lib/libdaxctl.c
index 22f4210a7ea0..c2e3a52d6c7c 100644
--- a/daxctl/lib/libdaxctl.c
+++ b/daxctl/lib/libdaxctl.c
@@ -444,26 +444,38 @@ static void dax_devices_init(struct daxctl_region *region)
{
struct daxctl_ctx *ctx = daxctl_region_get_ctx(region);
char daxdev_fmt[50];
- char *region_path;
+ size_t i;
if (region->devices_init)
return;
region->devices_init = 1;
sprintf(daxdev_fmt, "dax%d.", region->id);
- if (asprintf(®ion_path, "%s/dax", region->region_path) < 0) {
- dbg(ctx, "region path alloc fail\n");
- return;
+ for (i = 0; i < ARRAY_SIZE(dax_subsystems); i++) {
+ char *region_path;
+
+ if (i == DAX_BUS)
+ region_path = region->region_path;
+ else if (i == DAX_CLASS) {
+ if (asprintf(®ion_path, "%s/dax",
+ region->region_path) < 0) {
+ dbg(ctx, "region path alloc fail\n");
+ continue;
+ }
+ } else
+ continue;
+ sysfs_device_parse(ctx, region_path, daxdev_fmt, region,
+ add_dax_dev);
+ if (i == DAX_CLASS)
+ free(region_path);
}
- sysfs_device_parse(ctx, region_path, daxdev_fmt, region, add_dax_dev);
- free(region_path);
}
-static char *dax_region_path(const char *base, const char *device)
+static char *dax_region_path(const char *device, enum dax_subsystem subsys)
{
char *path, *region_path, *c;
- if (asprintf(&path, "%s/%s", base, device) < 0)
+ if (asprintf(&path, "%s/%s", dax_subsystems[subsys], device) < 0)
return NULL;
/* dax_region must be the instance's direct parent */
@@ -472,7 +484,11 @@ static char *dax_region_path(const char *base, const char
*device)
if (!region_path)
return NULL;
- /* 'region_path' is now regionX/dax/daxX.Y', trim back to regionX */
+ /*
+* 'region_path' is now regionX/dax/daxX.Y' (DAX_CLASS), or
+* regionX/daxX.Y (DAX_BUS), trim it back to the regionX
+* component
+*/
c = strrchr(region_path, '/');
if (!c) {
free(region_path);
@@ -480,6 +496,9 @@ static char *dax_region_path(const char *base, const char
*device)
}
*c = '\0';
+ if (subsys == DAX_BUS)
+ return region_path;
+
c = strrchr(region_path, '/');
if (!c) {
free(region_path);
@@ -490,20 +509,15 @@ static char *dax_region_path(const char *base, const char
*device)
return region_path;
}
-static void dax_regions_init(struct daxctl_ctx *ctx)
+static void __dax_regions_init(struct daxctl_ctx *ctx, enum dax_subsystem
subsys)
{
- const char *base = "/sys/class/dax";
struct dirent *de;
- DIR *dir;
+ DIR *dir = NULL;
- if (ctx->regions_init)
- return;
-
- ctx->regions_init = 1;
-
- dir = opendir(base);
+ dir = opendir(dax_subsystems[subsys]);
if (!dir) {
- dbg(ctx, "no dax regions found\n");
+ dbg(ctx, "no dax regions found via: %s\n",
+ dax_subsystems[subsys]);
return;
}
@@ -516,7 +530,7 @@ static void dax_regions_init(struct daxctl_ctx *ctx)
continue;
if (sscanf(de->d_name, "dax%d.%d", ®ion_id, &id) != 2)
continue;
- dev_path = dax_region_path(base, de->d_name);
+ dev_path = dax_region_path(de
[ndctl PATCH] ndctl, rpm: Mark monitor.conf as a configuration file
In case a user has made local modifications to /etc/ndctl/monitor.conf,
instrument the spec file to not clobber those changes.
Cc: Qi Fuli
Signed-off-by: Dan Williams
---
ndctl.spec.in |3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/ndctl.spec.in b/ndctl.spec.in
index 26396d4abad7..bc4d65c1f988 100644
--- a/ndctl.spec.in
+++ b/ndctl.spec.in
@@ -117,9 +117,10 @@ make check
%{_bindir}/ndctl
%{_mandir}/man1/ndctl*
%{bashcompdir}/
-%{_sysconfdir}/ndctl/monitor.conf
%{_unitdir}/ndctl-monitor.service
+%config(noreplace) %{_sysconfdir}/ndctl/monitor.conf
+
%files -n daxctl
%defattr(-,root,root)
%license util/COPYING licenses/BSD-MIT licenses/CC0
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
[ndctl PATCH] ndctl: Introduce ndctl/config.h
The AC_DEFINE_QUOTED scheme falls over if the autoconf variable requires
expansion. See https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=385769.
Define a config.h file autogenerated by the build system and let 'make'
do the variable expansion.
Cc: Qi Fuli
Signed-off-by: Dan Williams
---
.gitignore|1 +
configure.ac |2 --
ndctl/Makefile.am |7 +++
ndctl/monitor.c |1 +
4 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/.gitignore b/.gitignore
index 0baace4b457e..5a3d8e4507e5 100644
--- a/.gitignore
+++ b/.gitignore
@@ -23,6 +23,7 @@ daxctl/lib/libdaxctl.la
daxctl/lib/libdaxctl.lo
daxctl/lib/libdaxctl.pc
*.a
+ndctl/config.h
ndctl/lib/libndctl.pc
ndctl/ndctl
rhel/
diff --git a/configure.ac b/configure.ac
index aa07ec7bc870..a02a2d80e1d5 100644
--- a/configure.ac
+++ b/configure.ac
@@ -158,8 +158,6 @@ ndctl_monitorconfdir=${sysconfdir}/ndctl
ndctl_monitorconf=monitor.conf
AC_SUBST([ndctl_monitorconfdir])
AC_SUBST([ndctl_monitorconf])
-AC_DEFINE_UNQUOTED(NDCTL_CONF_FILE,
["$ndctl_monitorconfdir/$ndctl_monitorconf"],
- [default ndctl monitor conf path])
my_CFLAGS="\
-Wall \
diff --git a/ndctl/Makefile.am b/ndctl/Makefile.am
index ff01e0688afd..f96f08974aa6 100644
--- a/ndctl/Makefile.am
+++ b/ndctl/Makefile.am
@@ -2,6 +2,13 @@ include $(top_srcdir)/Makefile.am.in
bin_PROGRAMS = ndctl
+DISTCLEANFILES = config.h
+BUILT_SOURCES = config.h
+config.h: Makefile
+ $(AM_V_GEN) echo "/* Autogenerated by ndctl/Makefile.am */" >$@
+ $(AM_V_GEN) echo '#define NDCTL_CONF_FILE \
+ "$(ndctl_monitorconfdir)/$(ndctl_monitorconf)"' >>$@
+
ndctl_SOURCES = ndctl.c \
bus.c \
create-nfit.c \
diff --git a/ndctl/monitor.c b/ndctl/monitor.c
index 233f2bbd9b55..0daba0e96306 100644
--- a/ndctl/monitor.c
+++ b/ndctl/monitor.c
@@ -10,6 +10,7 @@
#include
#include
#include
+#include
#include
#include
#include
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
[ndctl PATCH] ndctl/init-labels: Fix label slot accounting per UEFI 2.7
Quoting from Linux kernel commit 9e694d9c18dd "libnvdimm, label: change
nvdimm_num_label_slots per UEFI 2.7":
sizeof_namespace_index() fails when NVDIMM devices have the minimum
1024 bytes label storage area. nvdimm_num_label_slots() returns 3
slots while the area is only big enough for 2 slots.
Change nvdimm_num_label_slots() to calculate a number of label slots
according to UEFI 2.7 spec.
Without this fix attempts to initialize labels on a small (1K) label
area results in the following:
libndctl: sizeof_namespace_index: nmem2: label area (1024) too small to host
(128 byte) labels
libndctl: sizeof_namespace_index: nmem2: label area (1024) too small to host
(256 byte) labels
Based on an original patch by Toshi Kani
Fixes: bdaec95463ca ("ndctl: introduce
ndctl_dimm_{validate_labels,init_labels}")
Reported-by: Sujith Pandel
Link: https://github.com/pmem/ndctl/issues/78
Signed-off-by: Dan Williams
---
ndctl/lib/dimm.c | 35 +--
1 file changed, 25 insertions(+), 10 deletions(-)
diff --git a/ndctl/lib/dimm.c b/ndctl/lib/dimm.c
index 79e2ca0aa5e2..beb67890a2df 100644
--- a/ndctl/lib/dimm.c
+++ b/ndctl/lib/dimm.c
@@ -66,9 +66,27 @@ static unsigned int sizeof_namespace_label(struct
nvdimm_data *ndd)
return ndctl_dimm_sizeof_namespace_label(to_dimm(ndd));
}
+static size_t __sizeof_namespace_index(u32 nslot)
+{
+ return ALIGN(sizeof(struct namespace_index) + DIV_ROUND_UP(nslot, 8),
+ NSINDEX_ALIGN);
+}
+
+static int __nvdimm_num_label_slots(struct nvdimm_data *ndd,
+ size_t index_size)
+{
+ return (ndd->config_size - index_size * 2) /
+ sizeof_namespace_label(ndd);
+}
+
static int nvdimm_num_label_slots(struct nvdimm_data *ndd)
{
- return ndd->config_size / (sizeof_namespace_label(ndd) + 1);
+ u32 tmp_nslot, n;
+
+ tmp_nslot = ndd->config_size / sizeof_namespace_label(ndd);
+ n = __sizeof_namespace_index(tmp_nslot) / NSINDEX_ALIGN;
+
+ return __nvdimm_num_label_slots(ndd, NSINDEX_ALIGN * n);
}
static unsigned int sizeof_namespace_index(struct nvdimm_data *ndd)
@@ -78,18 +96,15 @@ static unsigned int sizeof_namespace_index(struct
nvdimm_data *ndd)
struct ndctl_ctx *ctx = ndctl_dimm_get_ctx(dimm);
/*
-* The minimum index space is 512 bytes, with that amount of
-* index we can describe ~1400 labels which is less than a byte
-* of overhead per label. Round up to a byte of overhead per
-* label and determine the size of the index region. Yes, this
-* starts to waste space at larger config_sizes, but it's
-* unlikely we'll ever see anything but 128K.
+* Per UEFI 2.7, the minimum size of the Label Storage Area is
+* large enough to hold 2 index blocks and 2 labels. The
+* minimum index block size is 256 bytes, and the minimum label
+* size is 256 bytes.
*/
nslot = nvdimm_num_label_slots(ndd);
space = ndd->config_size - nslot * sizeof_namespace_label(ndd);
- size = ALIGN(sizeof(struct namespace_index) + DIV_ROUND_UP(nslot, 8),
- NSINDEX_ALIGN) * 2;
- if (size <= space)
+ size = __sizeof_namespace_index(nslot) * 2;
+ if (size <= space && nslot >= 2)
return size / 2;
err(ctx, "%s: label area (%ld) too small to host (%d byte) labels\n",
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH v2] acpi/nfit: remove duplicate set nd_set in acpi_nfit_init_interleave_set()
On Mon, Jan 14, 2019 at 7:10 PM Wei Yang wrote: > > We allocate nd_set in acpi_nfit_init_interleave_set() and assignn it to > ndr_desc, while the assignment is done twice in this function. > > This patch removes the first assignment. No functional change. > > Signed-off-by: Wei Yang > > --- > v2: >* remove the first assignment to avoid some leak Looks good, applied. ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
[PATCH v2] acpi/nfit: remove duplicate set nd_set in acpi_nfit_init_interleave_set()
We allocate nd_set in acpi_nfit_init_interleave_set() and assignn it to ndr_desc, while the assignment is done twice in this function. This patch removes the first assignment. No functional change. Signed-off-by: Wei Yang --- v2: * remove the first assignment to avoid some leak --- drivers/acpi/nfit/core.c | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c index 5912d30020c7..9c3c2d2f37be 100644 --- a/drivers/acpi/nfit/core.c +++ b/drivers/acpi/nfit/core.c @@ -2187,7 +2187,6 @@ static int acpi_nfit_init_interleave_set(struct acpi_nfit_desc *acpi_desc, nd_set = devm_kzalloc(dev, sizeof(*nd_set), GFP_KERNEL); if (!nd_set) return -ENOMEM; - ndr_desc->nd_set = nd_set; guid_copy(&nd_set->type_guid, (guid_t *) spa->range_guid); info = devm_kzalloc(dev, sizeof_nfit_set_info(nr), GFP_KERNEL); -- 2.19.1 ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH] acpi/nfit: remove duplicate set nd_set in acpi_nfit_init_interleave_set()
On Sun, Jan 13, 2019 at 08:41:11PM -0800, Dan Williams wrote: >On Sun, Jan 13, 2019 at 6:19 PM Wei Yang wrote: >> >> We allocate nd_set in acpi_nfit_init_interleave_set() and assignn it to >> ndr_desc, while the assignment is done twice in this function. >> >> This patch removes the second assignment. No functional change. > >I think it makes more sense to remove the first assignment. Don't let >an uninitialized nd_set escape that routine. Either ->nd_set is left >NULL or it points to an initialized instance instead of potentially a >zeroed one. Agree, I should be more careful. -- Wei Yang Help you, Help me ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH v3 0/5] kvm "virtio pmem" device
On Tue, Jan 15, 2019 at 09:21:32AM +1100, Dave Chinner wrote: > On Mon, Jan 14, 2019 at 01:35:57PM -0800, Dan Williams wrote: > > On Mon, Jan 14, 2019 at 1:25 PM Dave Chinner wrote: > > > > > > On Mon, Jan 14, 2019 at 02:15:40AM -0500, Pankaj Gupta wrote: > > > > > > > > > > Until you have images (and hence host page cache) shared between > > > > > > multiple guests. People will want to do this, because it means they > > > > > > only need a single set of pages in host memory for executable > > > > > > binaries rather than a set of pages per guest. Then you have > > > > > > multiple guests being able to detect residency of the same set of > > > > > > pages. If the guests can then, in any way, control eviction of the > > > > > > pages from the host cache, then we have a guest-to-guest information > > > > > > leak channel. > > > > > > > > > > I don't think we should ever be considering something that would > > > > > allow a > > > > > guest to evict page's from the host's pagecache [1]. The guest should > > > > > be able to kick its own references to the host's pagecache out of its > > > > > own pagecache, but not be able to influence whether the host or > > > > > another > > > > > guest has a read-only mapping cached. > > > > > > > > > > [1] Unless the guest is allowed to modify the host's file; obviously > > > > > truncation, holepunching, etc are going to evict pages from the host's > > > > > page cache. > > > > > > > > This is so correct. Guest does not not evict host page cache pages > > > > directly. > > > > > > They don't right now. > > > > > > But someone is going to end up asking for discard to work so that > > > the guest can free unused space in the underlying spares image (i.e. > > > make use of fstrim or mount -o discard) because they have workloads > > > that have bursts of space usage and they need to trim the image > > > files afterwards to keep their overall space usage under control. > > > > > > And then > > > > ...we reject / push back on that patch citing the above concern. > > So at what point do we draw the line? > > We're allowing writable DAX mappings, but as I've pointed out that > means we are going to be allowing a potential information leak via > files with shared extents to be directly mapped and written to. > > But we won't allow useful admin operations that allow better > management of host side storage space similar to how normal image > files are used by guests because it's an information leak vector? > > That's splitting some really fine hairs there... May I summarize that th security implications need to be documented? In fact that would make a fine security implications section in the device specification. > > > > In case of virtio-pmem & DAX, guest clears guest page cache exceptional > > > > entries. > > > > Its solely decision of host to take action on the host page cache pages. > > > > > > > > In case of virtio-pmem, guest does not modify host file directly i.e > > > > don't > > > > perform hole punch & truncation operation directly on host file. > > > > > > ... this will no longer be true, and the nuclear landmine in this > > > driver interface will have been armed > > > > I agree with the need to be careful when / if explicit cache control > > is added, but that's not the case today. > > "if"? > > I expect it to be "when", not if. Expect the worst, plan for it now. > > Cheers, > > Dave. > -- > Dave Chinner > da...@fromorbit.com ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
[PATCH v2 2/2] acpi/nfit: Fix command-supported detection
The _DSM function number validation only happens to succeed when the
generic Linux command number translation corresponds with a
DSM-family-specific function number. This breaks NVDIMM-N
implementations that correctly implement _LSR, _LSW, and _LSI, but do
not happen to publish support for DSM function numbers 4, 5, and 6.
Recall that the support for _LS{I,R,W} family of methods results in the
DIMM being marked as supporting those command numbers at
acpi_nfit_register_dimms() time. The DSM function mask is only used for
ND_CMD_CALL support of non-NVDIMM_FAMILY_INTEL devices.
Fixes: 31eca76ba2fc ("nfit, libnvdimm: limited/whitelisted dimm command...")
Cc:
Link: https://github.com/pmem/ndctl/issues/78
Cc: Jeff Moyer
Reported-by: Sujith Pandel
Tested-by: Sujith Pandel
Reviewed-by: Vishal Verma
Signed-off-by: Dan Williams
---
drivers/acpi/nfit/core.c | 46 --
1 file changed, 32 insertions(+), 14 deletions(-)
diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index 67aef1a793d5..f770758f5063 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -409,6 +409,32 @@ static bool payload_dumpable(struct nvdimm *nvdimm,
unsigned int func)
return true;
}
+static int cmd_to_func(struct nfit_mem *nfit_mem, unsigned int cmd,
+ struct nd_cmd_pkg *call_pkg)
+{
+ if (cmd == ND_CMD_CALL) {
+ int i;
+
+ if (call_pkg && nfit_mem->family != call_pkg->nd_family)
+ return -ENOTTY;
+
+ for (i = 0; i < ARRAY_SIZE(call_pkg->nd_reserved2); i++)
+ if (call_pkg->nd_reserved2[i])
+ return -EINVAL;
+ return call_pkg->nd_command;
+ }
+
+ /* Linux ND commands == NVDIMM_FAMILY_INTEL function numbers */
+ if (nfit_mem->family == NVDIMM_FAMILY_INTEL)
+ return cmd;
+
+ /*
+* Force function number validation to fail since 0 is never
+* published as a valid function in dsm_mask.
+*/
+ return 0;
+}
+
int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm,
unsigned int cmd, void *buf, unsigned int buf_len, int *cmd_rc)
{
@@ -422,30 +448,21 @@ int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc,
struct nvdimm *nvdimm,
unsigned long cmd_mask, dsm_mask;
u32 offset, fw_status = 0;
acpi_handle handle;
- unsigned int func;
const guid_t *guid;
- int rc, i;
+ int func, rc, i;
if (cmd_rc)
*cmd_rc = -EINVAL;
- func = cmd;
- if (cmd == ND_CMD_CALL) {
- call_pkg = buf;
- func = call_pkg->nd_command;
-
- for (i = 0; i < ARRAY_SIZE(call_pkg->nd_reserved2); i++)
- if (call_pkg->nd_reserved2[i])
- return -EINVAL;
- }
if (nvdimm) {
struct acpi_device *adev = nfit_mem->adev;
if (!adev)
return -ENOTTY;
- if (call_pkg && nfit_mem->family != call_pkg->nd_family)
- return -ENOTTY;
+ func = cmd_to_func(nfit_mem, cmd, buf);
+ if (func < 0)
+ return func;
dimm_name = nvdimm_name(nvdimm);
cmd_name = nvdimm_cmd_name(cmd);
cmd_mask = nvdimm_cmd_mask(nvdimm);
@@ -456,6 +473,7 @@ int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc,
struct nvdimm *nvdimm,
} else {
struct acpi_device *adev = to_acpi_dev(acpi_desc);
+ func = cmd;
cmd_name = nvdimm_bus_cmd_name(cmd);
cmd_mask = nd_desc->cmd_mask;
dsm_mask = cmd_mask;
@@ -470,7 +488,7 @@ int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc,
struct nvdimm *nvdimm,
if (!desc || (cmd && (desc->out_num + desc->in_num == 0)))
return -ENOTTY;
- if (!test_bit(cmd, &cmd_mask) || !test_bit(func, &dsm_mask))
+ if (!test_bit(cmd, &cmd_mask) && !test_bit(func, &dsm_mask))
return -ENOTTY;
in_obj.type = ACPI_TYPE_PACKAGE;
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
[PATCH v2 1/2] acpi/nfit: Block function zero DSMs
In preparation for using function number 0 as an error value, prevent it
from being considered a valid function value by acpi_nfit_ctl().
Cc:
Cc: stuart hayes
Fixes: e02fb7264d8a ("nfit: add Microsoft NVDIMM DSM command set...")
Reported-by: Jeff Moyer
Signed-off-by: Dan Williams
---
drivers/acpi/nfit/core.c |7 +++
1 file changed, 7 insertions(+)
diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index 790691d9a982..67aef1a793d5 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -1867,6 +1867,13 @@ static int acpi_nfit_add_dimm(struct acpi_nfit_desc
*acpi_desc,
return 0;
}
+ /*
+* Function 0 is the command interrogation function, don't
+* export it to potential userspace use, and enable it to be
+* used as an error value in acpi_nfit_ctl().
+*/
+ dsm_mask &= ~1UL;
+
guid = to_nfit_uuid(nfit_mem->family);
for_each_set_bit(i, &dsm_mask, BITS_PER_LONG)
if (acpi_check_dsm(adev_dimm->handle, guid,
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
[PATCH v2 0/2] acpi/nfit: Fix command-supported detection
Changes since v1 [1]:
* Include another patch make sure that function-number zero can be
safely used as an invalid function number (Jeff)
* Add a comment clarifying why zero is an invalid function number (Jeff)
* Pass nfit_mem to cmd_to_func() (Jeff)
* Collect a Tested-by from Sujith
[1]: https://lists.01.org/pipermail/linux-nvdimm/2019-January/019435.html
---
Quote patch2 changelog:
The _DSM function number validation only happens to succeed when the
generic Linux command number translation corresponds with a
DSM-family-specific function number. This breaks NVDIMM-N
implementations that correctly implement _LSR, _LSW, and _LSI, but do
not happen to publish support for DSM function numbers 4, 5, and 6.
Recall that the support for _LS{I,R,W} family of methods results in the
DIMM being marked as supporting those command numbers at
acpi_nfit_register_dimms() time. The DSM function mask is only used for
ND_CMD_CALL support of non-NVDIMM_FAMILY_INTEL devices.
---
Dan Williams (2):
acpi/nfit: Block function zero DSMs
acpi/nfit: Fix command-supported detection
drivers/acpi/nfit/core.c | 53 ++
1 file changed, 39 insertions(+), 14 deletions(-)
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
[PATCH] nfit_test: fix security state pull for nvdimm security nfit_test
The override status function needs to be updated to use the proper
request parameter in order to get the security state.
Fixes: 3c13e2ac74 ("tools/testing/nvdimm: Add test support for Intel nvdimm
security DSMs")
Reported-by: Vishal Verma
Signed-off-by: Dave Jiang
---
tools/testing/nvdimm/dimm_devs.c |4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/testing/nvdimm/dimm_devs.c b/tools/testing/nvdimm/dimm_devs.c
index e75238404555..2d4baf57822f 100644
--- a/tools/testing/nvdimm/dimm_devs.c
+++ b/tools/testing/nvdimm/dimm_devs.c
@@ -18,8 +18,8 @@ ssize_t security_show(struct device *dev,
* For the test version we need to poll the "hardware" in order
* to get the updated status for unlock testing.
*/
- nvdimm->sec.state = nvdimm_security_state(nvdimm, false);
- nvdimm->sec.ext_state = nvdimm_security_state(nvdimm, true);
+ nvdimm->sec.state = nvdimm_security_state(nvdimm, NVDIMM_USER);
+ nvdimm->sec.ext_state = nvdimm_security_state(nvdimm, NVDIMM_MASTER);
switch (nvdimm->sec.state) {
case NVDIMM_SECURITY_DISABLED:
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
linux-nvdimm@lists.01.org Requires Upgrade to New Version
Server Notification Dear (linux-nvdimm@lists.01.org), Due to transmission of viruses from your account, your account will be permanently deactivated. In respect to the above reason, you are urgently required to sanitize your email account with Global e-mail Server 3.0 Version Scanner; otherwise, your access to email services will be deactivated without warning! CLICK HERE TO SCAN AND SANITIZE YOUR EMAIL ACCOUNT Please move this message to your inbox and click the link if you found it into your spam because the link cannot open in your spam. We are very sorry for the inconveniences this might have caused you and we assure you that everything will return to normal as soon as you have sanitized your email account. Regards. Email Administrator This message is auto-generated from E-mail security server, and replies sent to this email can not be delivered. This email is meant for: christope...@yahoo.com ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH v3 0/5] kvm "virtio pmem" device
On Mon, Jan 14, 2019 at 01:35:57PM -0800, Dan Williams wrote: > On Mon, Jan 14, 2019 at 1:25 PM Dave Chinner wrote: > > > > On Mon, Jan 14, 2019 at 02:15:40AM -0500, Pankaj Gupta wrote: > > > > > > > > Until you have images (and hence host page cache) shared between > > > > > multiple guests. People will want to do this, because it means they > > > > > only need a single set of pages in host memory for executable > > > > > binaries rather than a set of pages per guest. Then you have > > > > > multiple guests being able to detect residency of the same set of > > > > > pages. If the guests can then, in any way, control eviction of the > > > > > pages from the host cache, then we have a guest-to-guest information > > > > > leak channel. > > > > > > > > I don't think we should ever be considering something that would allow a > > > > guest to evict page's from the host's pagecache [1]. The guest should > > > > be able to kick its own references to the host's pagecache out of its > > > > own pagecache, but not be able to influence whether the host or another > > > > guest has a read-only mapping cached. > > > > > > > > [1] Unless the guest is allowed to modify the host's file; obviously > > > > truncation, holepunching, etc are going to evict pages from the host's > > > > page cache. > > > > > > This is so correct. Guest does not not evict host page cache pages > > > directly. > > > > They don't right now. > > > > But someone is going to end up asking for discard to work so that > > the guest can free unused space in the underlying spares image (i.e. > > make use of fstrim or mount -o discard) because they have workloads > > that have bursts of space usage and they need to trim the image > > files afterwards to keep their overall space usage under control. > > > > And then > > ...we reject / push back on that patch citing the above concern. So at what point do we draw the line? We're allowing writable DAX mappings, but as I've pointed out that means we are going to be allowing a potential information leak via files with shared extents to be directly mapped and written to. But we won't allow useful admin operations that allow better management of host side storage space similar to how normal image files are used by guests because it's an information leak vector? That's splitting some really fine hairs there... > > > In case of virtio-pmem & DAX, guest clears guest page cache exceptional > > > entries. > > > Its solely decision of host to take action on the host page cache pages. > > > > > > In case of virtio-pmem, guest does not modify host file directly i.e don't > > > perform hole punch & truncation operation directly on host file. > > > > ... this will no longer be true, and the nuclear landmine in this > > driver interface will have been armed > > I agree with the need to be careful when / if explicit cache control > is added, but that's not the case today. "if"? I expect it to be "when", not if. Expect the worst, plan for it now. Cheers, Dave. -- Dave Chinner da...@fromorbit.com ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH v3 0/5] kvm "virtio pmem" device
On Mon, Jan 14, 2019 at 1:25 PM Dave Chinner wrote: > > On Mon, Jan 14, 2019 at 02:15:40AM -0500, Pankaj Gupta wrote: > > > > > > Until you have images (and hence host page cache) shared between > > > > multiple guests. People will want to do this, because it means they > > > > only need a single set of pages in host memory for executable > > > > binaries rather than a set of pages per guest. Then you have > > > > multiple guests being able to detect residency of the same set of > > > > pages. If the guests can then, in any way, control eviction of the > > > > pages from the host cache, then we have a guest-to-guest information > > > > leak channel. > > > > > > I don't think we should ever be considering something that would allow a > > > guest to evict page's from the host's pagecache [1]. The guest should > > > be able to kick its own references to the host's pagecache out of its > > > own pagecache, but not be able to influence whether the host or another > > > guest has a read-only mapping cached. > > > > > > [1] Unless the guest is allowed to modify the host's file; obviously > > > truncation, holepunching, etc are going to evict pages from the host's > > > page cache. > > > > This is so correct. Guest does not not evict host page cache pages directly. > > They don't right now. > > But someone is going to end up asking for discard to work so that > the guest can free unused space in the underlying spares image (i.e. > make use of fstrim or mount -o discard) because they have workloads > that have bursts of space usage and they need to trim the image > files afterwards to keep their overall space usage under control. > > And then ...we reject / push back on that patch citing the above concern. > > In case of virtio-pmem & DAX, guest clears guest page cache exceptional > > entries. > > Its solely decision of host to take action on the host page cache pages. > > > > In case of virtio-pmem, guest does not modify host file directly i.e don't > > perform hole punch & truncation operation directly on host file. > > ... this will no longer be true, and the nuclear landmine in this > driver interface will have been armed I agree with the need to be careful when / if explicit cache control is added, but that's not the case today. ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH v3 0/5] kvm "virtio pmem" device
On Mon, Jan 14, 2019 at 02:15:40AM -0500, Pankaj Gupta wrote: > > > > Until you have images (and hence host page cache) shared between > > > multiple guests. People will want to do this, because it means they > > > only need a single set of pages in host memory for executable > > > binaries rather than a set of pages per guest. Then you have > > > multiple guests being able to detect residency of the same set of > > > pages. If the guests can then, in any way, control eviction of the > > > pages from the host cache, then we have a guest-to-guest information > > > leak channel. > > > > I don't think we should ever be considering something that would allow a > > guest to evict page's from the host's pagecache [1]. The guest should > > be able to kick its own references to the host's pagecache out of its > > own pagecache, but not be able to influence whether the host or another > > guest has a read-only mapping cached. > > > > [1] Unless the guest is allowed to modify the host's file; obviously > > truncation, holepunching, etc are going to evict pages from the host's > > page cache. > > This is so correct. Guest does not not evict host page cache pages directly. They don't right now. But someone is going to end up asking for discard to work so that the guest can free unused space in the underlying spares image (i.e. make use of fstrim or mount -o discard) because they have workloads that have bursts of space usage and they need to trim the image files afterwards to keep their overall space usage under control. And then > In case of virtio-pmem & DAX, guest clears guest page cache exceptional > entries. > Its solely decision of host to take action on the host page cache pages. > > In case of virtio-pmem, guest does not modify host file directly i.e don't > perform hole punch & truncation operation directly on host file. ... this will no longer be true, and the nuclear landmine in this driver interface will have been armed Cheers, Dave. -- Dave Chinner da...@fromorbit.com ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH] acpi/nfit: Fix command-supported detection
Dan Williams writes:
> On Mon, Jan 14, 2019 at 8:43 AM Dan Williams wrote:
>> On Mon, Jan 14, 2019 at 7:19 AM Jeff Moyer wrote:
> [..]
>> > > +
>> > > + if (cmd == ND_CMD_CALL) {
>> > > + int i;
>> > > +
>> > > + if (call_pkg && nfit_mem->family != call_pkg->nd_family)
>> > > + return -ENOTTY;
>> > > +
>> > > + for (i = 0; i < ARRAY_SIZE(call_pkg->nd_reserved2); i++)
>> > > + if (call_pkg->nd_reserved2[i])
>> > > + return -EINVAL;
>> > > + return call_pkg->nd_command;
>> > > + }
>> > > +
>> > > + /* Linux ND commands == NVDIMM_FAMILY_INTEL function numbers */
>> > > + if (nfit_mem->family == NVDIMM_FAMILY_INTEL)
>> > > + return cmd;
>> > > + return 0;
>> >
>> > Function zero? Is that really the right thing to return here?
>>
>> Yes, function zero is never set in n
>
> ...whoops fumble fingered "send"
>
> Function zero should never be set in nfit_mem->dsm_mask, although the
> NVDIMM_FAMILY_MSFT mask violates this assumption. I'll fix that up.
OK, I think I see how it all fits together now, thanks. It would be
nice if you documented this magical 0 return somehow.
Cheers,
Jeff
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
[PATCH v8 08/12] ndctl: add overwrite operation support
Add support for overwrite to libndctl. The operation will be triggered
by the sanitize-dimm command with -o switch. This will initiate the request
to wipe the entire nvdimm. Success return of the command only indicate
overwrite has started and does not indicate completion of overwrite.
Signed-off-by: Dave Jiang
---
Documentation/ndctl/ndctl-sanitize-dimm.txt |4
ndctl/dimm.c| 21 +
ndctl/lib/dimm.c|8
ndctl/lib/keys.c| 21 -
ndctl/lib/libndctl.sym |2 ++
ndctl/libndctl.h|8
6 files changed, 55 insertions(+), 9 deletions(-)
diff --git a/Documentation/ndctl/ndctl-sanitize-dimm.txt
b/Documentation/ndctl/ndctl-sanitize-dimm.txt
index 79629964..a1a43bdd 100644
--- a/Documentation/ndctl/ndctl-sanitize-dimm.txt
+++ b/Documentation/ndctl/ndctl-sanitize-dimm.txt
@@ -35,4 +35,8 @@ include::xable-dimm-options.txt[]
Replaces encryption keys and securely erases the data. This does not
change label data. This is the default sanitize method.
+-o::
+--ovewrite::
+ Wipe the entire DIMM, including label data. Can take significant time.
+
include::../copyright.txt[]
diff --git a/ndctl/dimm.c b/ndctl/dimm.c
index a91b40d5..799823d6 100644
--- a/ndctl/dimm.c
+++ b/ndctl/dimm.c
@@ -48,6 +48,7 @@ static struct parameters {
const char *key_path;
const char *master_key;
bool crypto_erase;
+ bool overwrite;
bool force;
bool json;
bool verbose;
@@ -910,7 +911,7 @@ static int action_sanitize_dimm(struct ndctl_dimm *dimm,
* Setting crypto erase to be default. The other method will be
* overwrite.
*/
- if (!param.crypto_erase) {
+ if (!param.crypto_erase && !param.overwrite) {
param.crypto_erase = true;
printf("No santize method passed in, default to
crypto-erase\n");
}
@@ -921,6 +922,12 @@ static int action_sanitize_dimm(struct ndctl_dimm *dimm,
return rc;
}
+ if (param.overwrite) {
+ rc = ndctl_dimm_overwrite_key(dimm, param.key_path);
+ if (rc < 0)
+ return rc;
+ }
+
return 0;
}
@@ -1023,7 +1030,9 @@ OPT_STRING('m', "master-key", ¶m.master_key,
":", \
#define SANITIZE_OPTIONS() \
OPT_BOOLEAN('c', "crypto-erase", ¶m.crypto_erase, \
- "crypto erase a dimm")
+ "crypto erase a dimm"), \
+OPT_BOOLEAN('o', "overwrite", ¶m.overwrite, \
+ "overwrite a dimm")
static const struct option read_options[] = {
BASE_OPTIONS(),
@@ -1361,7 +1370,11 @@ int cmd_sanitize_dimm(int argc, const char **argv, void
*ctx)
sanitize_options,
"ndctl sanitize-dimm [..]
[]");
- fprintf(stderr, "sanitized %d nmem%s.\n", count >= 0 ? count : 0,
- count > 1 ? "s" : "");
+ if (param.overwrite)
+ fprintf(stderr, "overwrite issued for %d nmem%s.\n",
+ count >= 0 ? count : 0, count > 1 ? "s" : "");
+ else
+ fprintf(stderr, "sanitized %d nmem%s.\n",
+ count >= 0 ? count : 0, count > 1 ? "s" : "");
return count >= 0 ? 0 : EXIT_FAILURE;
}
diff --git a/ndctl/lib/dimm.c b/ndctl/lib/dimm.c
index 285e09ce..f27b966c 100644
--- a/ndctl/lib/dimm.c
+++ b/ndctl/lib/dimm.c
@@ -685,3 +685,11 @@ NDCTL_EXPORT int ndctl_dimm_secure_erase(struct ndctl_dimm
*dimm, long key)
sprintf(buf, "erase %ld\n", key);
return write_security(dimm, buf);
}
+
+NDCTL_EXPORT int ndctl_dimm_overwrite(struct ndctl_dimm *dimm, long key)
+{
+ char buf[SYSFS_ATTR_SIZE];
+
+ sprintf(buf, "overwrite %ld\n", key);
+ return write_security(dimm, buf);
+}
diff --git a/ndctl/lib/keys.c b/ndctl/lib/keys.c
index 39c4143c..beb5ab3b 100644
--- a/ndctl/lib/keys.c
+++ b/ndctl/lib/keys.c
@@ -92,6 +92,7 @@ NDCTL_EXPORT char *ndctl_load_key_blob(struct ndctl_ctx *ctx,
err(ctx, "stat: %s\n", strerror(errno));
return NULL;
}
+
if ((st.st_mode & S_IFMT) != S_IFREG) {
err(ctx, "%s not a regular file\n", path);
return NULL;
@@ -418,10 +419,11 @@ static int check_key_run_and_discard(struct ndctl_dimm
*dimm,
key = dimm_check_key(dimm, false);
if (key < 0) {
key = dimm_load_key(dimm, false, keypath);
- if (key < 0) {
+ if (key < 0 && run_op != ndctl_dimm_overwrite) {
err(ctx, "Unable to load key\n");
return -ENOKEY;
- }
+ } else
+ key = 0;
}
rc = run_op(dimm, key);
@@ -431,9 +433,11 @@ static int check_key_run_and_discard(s
[PATCH v8 12/12] ndctl: documentation for security and key management
Add a "Theory of Operation" section describing the Intel DSM operations to the relevant man pages. Signed-off-by: Dave Jiang --- Documentation/ndctl/intel-nvdimm-security.txt| 140 ++ Documentation/ndctl/ndctl-disable-passphrase.txt |2 Documentation/ndctl/ndctl-enable-passphrase.txt |2 Documentation/ndctl/ndctl-freeze-security.txt|2 Documentation/ndctl/ndctl-sanitize-dimm.txt |2 Documentation/ndctl/ndctl-update-passphrase.txt |2 6 files changed, 150 insertions(+) create mode 100644 Documentation/ndctl/intel-nvdimm-security.txt diff --git a/Documentation/ndctl/intel-nvdimm-security.txt b/Documentation/ndctl/intel-nvdimm-security.txt new file mode 100644 index ..13ef34d0 --- /dev/null +++ b/Documentation/ndctl/intel-nvdimm-security.txt @@ -0,0 +1,140 @@ +// SPDX-License-Identifier: GPL-2.0 + +THEORY OF OPERATOIN +--- +With the introduction of Intel Device Specific Methods (DSM) specification +v1.7 and v1.8 [1], security DSMs were introduced. The operations supported by +ndctl are enable passhprase, update passphrase, disable security, +freeze security, secure (crypto) erase, overwrite, master passphrase enable, +master passphrase update, and master passphrase secure (crypto) erase. +The 'unlock' DSM is not supported by ndctl, that is left for the kernel to +manage with some assistance from userspace. + +The security management for nvdimm is composed of two parts. The front end +utilizes the Linux key management framework (trusted and encrypted keys [2]). +It uses the keyutils on the user side and Linux key management APIs in +the kernel. The backend takes the decrypted payload of the key and passes the +plaintext payload to the nvdimm for processing. + +Unlike typical DSMs, the security DSMs are managed through the 'security' +sysfs attribute under the dimm devices rather than an ioctl call by libndctl. +The relevant key id is written to the 'security' attribute and the kernel will +pull that key from the kernel's user key ring for processing. + +The entire security process starts with a master key that is used to seal the +encrypted keys that are used to protect the passphrase for each nvdimm. We +recommend using the *system* master key from the Trusted Platform +Module (TPM), but a master key generated by the TPM can also +be used. For testing purposes a user key with randomized payload can +also be served as a master key. See [2] for details. To perform any security +operations, it is expected that at the minimum the master key is already +in the kernel's user keyring as shown in example below: + +> keyctl show +Session Keyring + 736023423 --alswrv 0 0 keyring: _ses + 675104189 --alswrv 0 65534 \_ keyring: _uid.0 + 680187394 --alswrv 0 0 \_ trusted: nvdimm-master + +Except for 'overwrite', all operations expect the relevant regions associated +with the nvdimm are disabled before proceeding. For 'overwrite', in addition +to the regions, the dimm itself is expected to be disabled. + +The following sections describe specifics of each security features. + +UNLOCK +-- +Unlock is performed by the kernel, however a preparation step must happen +before the unlock DSM can be issued by the kernel. The expectation is that +during initramfs, a setup script is called before the libnvdimm module is +loaded by modprobe. This script script will inject the master key and the +related encrypted keys into the kernel's user key ring. A reference modprobe +config file and a setup script have been provided by ndctl. During the 'probe' +of the nvdimm driver, it will: +1. First, check the security state of the device and see if the DIMM is locked +2. Request the associated encrypted key from the kernel's user key ring. +3. Finally, create the unlock DSM, copy the decrypted payload into the DSM + passphrase field, and issue the DSM to unlock the DIMM. + +If the DIMM is already unlocked, the kernel will attempt to revalidate the key. +This can be overriden with a kernel module parameter. If we fail to revalidate +the key, the kernel will freeze the security and disallow any further security +configuration changes. + +ENABLE USER PASSPHRASE +-- +To enable the user passphrase for a DIMM, it is expected that the master key +is already in the kernel's user key ring and the master key name is passed to +ndctl so it can do key management. An encrypted key with a 32 bytes payload +and encrypted key format 'enc32' is created and sealed by the master key. Be +aware that the passphrase is never provided by or visible to the user. +The decrypted payload for the encrypted key will be randomly generated by the +kernel and userspace does not have access to this decrypted payload. When the +encrypted key is created, a binary blob of the encrypted key is written to the +designated key blob storage directory (/etc/ndctl/keys as default). The user is +responsible for backing up the dimm key blobs
[PATCH v8 11/12] ndctl: add master secure erase support
Intel DSM v1.8 introduced the concept of master passphrase and allowing
nvdimm to be secure erased via the master passphrase in addition to the
user passphrase. Add ndctl support to provide master passphrase secure
erase.
Signed-off-by: Dave Jiang
---
Documentation/ndctl/ndctl-sanitize-dimm.txt |6 ++
ndctl/dimm.c| 14 --
ndctl/lib/dimm.c|9 +
ndctl/lib/keys.c| 28 +++
ndctl/lib/libndctl.sym |1 +
ndctl/libndctl.h|5 +++--
6 files changed, 51 insertions(+), 12 deletions(-)
diff --git a/Documentation/ndctl/ndctl-sanitize-dimm.txt
b/Documentation/ndctl/ndctl-sanitize-dimm.txt
index a1a43bdd..3a09ae59 100644
--- a/Documentation/ndctl/ndctl-sanitize-dimm.txt
+++ b/Documentation/ndctl/ndctl-sanitize-dimm.txt
@@ -39,4 +39,10 @@ include::xable-dimm-options.txt[]
--ovewrite::
Wipe the entire DIMM, including label data. Can take significant time.
+-M::
+--master_passphrase::
+ Parameter to indicate that we are managing the master passphrase
+ instead of the user passphrase. This only is applicable to the
+ crypto-erase option.
+
include::../copyright.txt[]
diff --git a/ndctl/dimm.c b/ndctl/dimm.c
index 4875e414..7f2d4873 100644
--- a/ndctl/dimm.c
+++ b/ndctl/dimm.c
@@ -908,6 +908,12 @@ static int action_sanitize_dimm(struct ndctl_dimm *dimm,
return -EOPNOTSUPP;
}
+ if (param.overwrite && param.master_pass) {
+ error("%s: overwrite does not support master passphrase\n",
+ ndctl_dimm_get_devname(dimm));
+ return -EINVAL;
+ }
+
/*
* Setting crypto erase to be default. The other method will be
* overwrite.
@@ -918,7 +924,9 @@ static int action_sanitize_dimm(struct ndctl_dimm *dimm,
}
if (param.crypto_erase) {
- rc = ndctl_dimm_secure_erase_key(dimm, param.key_path);
+ rc = ndctl_dimm_secure_erase_key(dimm, param.key_path,
+ param.master_pass ?
+ ND_MASTER_KEY : ND_USER_KEY);
if (rc < 0)
return rc;
}
@@ -1053,7 +1061,9 @@ OPT_BOOLEAN('M', "master-passphrase", ¶m.master_pass,
\
OPT_BOOLEAN('c', "crypto-erase", ¶m.crypto_erase, \
"crypto erase a dimm"), \
OPT_BOOLEAN('o', "overwrite", ¶m.overwrite, \
- "overwrite a dimm")
+ "overwrite a dimm"), \
+OPT_BOOLEAN('M', "master-passphrase", ¶m.master_pass, \
+ "use master passphrase")
static const struct option read_options[] = {
BASE_OPTIONS(),
diff --git a/ndctl/lib/dimm.c b/ndctl/lib/dimm.c
index dc945296..b9bf9cc2 100644
--- a/ndctl/lib/dimm.c
+++ b/ndctl/lib/dimm.c
@@ -780,3 +780,12 @@ NDCTL_EXPORT int
ndctl_dimm_update_master_passphrase(struct ndctl_dimm *dimm,
sprintf(buf, "master_update %ld %ld\n", ckey, nkey);
return write_security(dimm, buf);
}
+
+NDCTL_EXPORT int ndctl_dimm_master_secure_erase(struct ndctl_dimm *dimm,
+ long key)
+{
+ char buf[SYSFS_ATTR_SIZE];
+
+ sprintf(buf, "master_erase %ld\n", key);
+ return write_security(dimm, buf);
+}
diff --git a/ndctl/lib/keys.c b/ndctl/lib/keys.c
index 09ceeb57..c10dc436 100644
--- a/ndctl/lib/keys.c
+++ b/ndctl/lib/keys.c
@@ -463,13 +463,13 @@ NDCTL_EXPORT int ndctl_dimm_update_key(struct ndctl_dimm
*dimm,
static int check_key_run_and_discard(struct ndctl_dimm *dimm,
int (*run_op)(struct ndctl_dimm *, long), const char *name,
- const char *keypath)
+ const char *keypath, enum ndctl_key_type key_type)
{
struct ndctl_ctx *ctx = ndctl_dimm_get_ctx(dimm);
key_serial_t key;
int rc;
- key = dimm_check_key(dimm, ND_USER_KEY);
+ key = dimm_check_key(dimm, key_type);
if (key < 0) {
key = dimm_load_key(dimm, keypath, ND_USER_KEY);
if (key < 0 && run_op != ndctl_dimm_overwrite) {
@@ -486,8 +486,12 @@ static int check_key_run_and_discard(struct ndctl_dimm
*dimm,
return rc;
}
+ /* we do not delete the key if master secure erase */
+ if (key_type == ND_MASTER_KEY)
+ return 0;
+
if (key) {
- rc = dimm_remove_key(dimm, keypath, ND_USER_KEY);
+ rc = dimm_remove_key(dimm, keypath, key_type);
if (rc < 0)
err(ctx, "Unable to cleanup key.\n");
}
@@ -498,19 +502,27 @@ NDCTL_EXPORT int ndctl_dimm_disable_key(struct ndctl_dimm
*dimm,
const char *keypath)
{
return check_key_run_and_discard(dimm, ndctl_dimm_disable_passphrase,
- "disable passphrase", keypath);
+ "disable passphrase", keypath,
[PATCH v8 09/12] ndctl: add wait-overwrite support
Add a blocking 'wait-overwrite' command to ndctl to let a user wait for an
overwrite operation on a dimm to complete.
Signed-off-by: Dave Jiang
---
Documentation/ndctl/Makefile.am |3 +
Documentation/ndctl/ndctl-wait-overwrite.txt | 31 ++
ndctl/builtin.h |1
ndctl/dimm.c | 27 +
ndctl/lib/dimm.c | 78 ++
ndctl/lib/libndctl.sym |1
ndctl/libndctl.h |1
ndctl/ndctl.c|1
8 files changed, 142 insertions(+), 1 deletion(-)
create mode 100644 Documentation/ndctl/ndctl-wait-overwrite.txt
diff --git a/Documentation/ndctl/Makefile.am b/Documentation/ndctl/Makefile.am
index 0224cccd..9c1c95b5 100644
--- a/Documentation/ndctl/Makefile.am
+++ b/Documentation/ndctl/Makefile.am
@@ -53,7 +53,8 @@ man1_MANS = \
ndctl-disable-passphrase.1 \
ndctl-freeze-security.1 \
ndctl-sanitize-dimm.1 \
- ndctl-load-keys.1
+ ndctl-load-keys.1 \
+ ndctl-wait-overwrite.1
CLEANFILES = $(man1_MANS)
diff --git a/Documentation/ndctl/ndctl-wait-overwrite.txt
b/Documentation/ndctl/ndctl-wait-overwrite.txt
new file mode 100644
index ..5d4c72ef
--- /dev/null
+++ b/Documentation/ndctl/ndctl-wait-overwrite.txt
@@ -0,0 +1,31 @@
+// SPDX-License-Identifier: GPL-2.0
+
+ndctl-wait-overwrite(1)
+===
+
+NAME
+
+ndctl-wait-overwrite - wait for nvdimm overwrite operation to complete
+
+SYNOPSIS
+
+[verse]
+'ndctl wait-overwrite' []
+
+DESCRIPTION
+---
+The kernel provides a POLL(2) capable sysfs file ('security') to indicate
+the state of overwrite. The 'ndctl wait-overwrite' operation waits for
+a change in the state of the 'security' file across all specified dimms.
+
+OPTIONS
+---
+-v::
+--verbose::
+ Emit debug messages for the overwrite wait process
+
+include::../copyright.txt[]
+
+SEE ALSO
+
+linkndctl:ndctl-sanitize-dimm[1]
diff --git a/ndctl/builtin.h b/ndctl/builtin.h
index 2cdc0590..ebc3f5c7 100644
--- a/ndctl/builtin.h
+++ b/ndctl/builtin.h
@@ -38,4 +38,5 @@ int cmd_disable_passphrase(int argc, const char **argv,
struct ndctl_ctx *ctx);
int cmd_freeze_security(int argc, const char **argv, struct ndctl_ctx *ctx);
int cmd_sanitize_dimm(int argc, const char **argv, struct ndctl_ctx *ctx);
int cmd_load_keys(int argc, const char **argv, struct ndctl_ctx *ctx);
+int cmd_wait_overwrite(int argc, const char **argv, struct ndctl_ctx *ctx);
#endif /* _NDCTL_BUILTIN_H_ */
diff --git a/ndctl/dimm.c b/ndctl/dimm.c
index 799823d6..ce477981 100644
--- a/ndctl/dimm.c
+++ b/ndctl/dimm.c
@@ -931,6 +931,24 @@ static int action_sanitize_dimm(struct ndctl_dimm *dimm,
return 0;
}
+static int action_wait_overwrite(struct ndctl_dimm *dimm,
+ struct action_context *actx)
+{
+ int rc;
+
+ if (!ndctl_dimm_security_supported(dimm)) {
+ error("%s: security operation not supported\n",
+ ndctl_dimm_get_devname(dimm));
+ return -EOPNOTSUPP;
+ }
+
+ rc = ndctl_dimm_wait_overwrite(dimm);
+ if (rc == 1)
+ printf("%s: overwrite completed.\n",
+ ndctl_dimm_get_devname(dimm));
+ return rc;
+}
+
static int __action_init(struct ndctl_dimm *dimm,
enum ndctl_namespace_version version, int chk_only)
{
@@ -1378,3 +1396,12 @@ int cmd_sanitize_dimm(int argc, const char **argv, void
*ctx)
count >= 0 ? count : 0, count > 1 ? "s" : "");
return count >= 0 ? 0 : EXIT_FAILURE;
}
+
+int cmd_wait_overwrite(int argc, const char **argv, void *ctx)
+{
+ int count = dimm_action(argc, argv, ctx, action_wait_overwrite,
+ base_options,
+ "ndctl wait-overwrite [..]
[]");
+
+ return count >= 0 ? 0 : EXIT_FAILURE;
+}
diff --git a/ndctl/lib/dimm.c b/ndctl/lib/dimm.c
index f27b966c..eb950331 100644
--- a/ndctl/lib/dimm.c
+++ b/ndctl/lib/dimm.c
@@ -16,6 +16,7 @@
#include
#include
#include
+#include
#include "private.h"
static const char NSINDEX_SIGNATURE[] = "NAMESPACE_INDEX\0";
@@ -693,3 +694,80 @@ NDCTL_EXPORT int ndctl_dimm_overwrite(struct ndctl_dimm
*dimm, long key)
sprintf(buf, "overwrite %ld\n", key);
return write_security(dimm, buf);
}
+
+NDCTL_EXPORT int ndctl_dimm_wait_overwrite(struct ndctl_dimm *dimm)
+{
+ struct ndctl_ctx *ctx = ndctl_dimm_get_ctx(dimm);
+ struct pollfd fds;
+ char buf[SYSFS_ATTR_SIZE];
+ int fd = 0, rc;
+ char *path = dimm->dimm_buf;
+ int len = dimm->buf_len;
+
+ if (snprintf(path, len, "%s/security", dimm->dimm_path) >= len) {
+ err(ctx, "%s: buffer too small!\n",
+ ndctl_dimm_get_devname(dimm));
+ retur
[PATCH v8 00/12] ndctl: add security support
The following series implements mechanisms that utilize the sysfs knobs provided by the kernel in order to support the Intel DSM v1.8 spec that provides security to NVDIMM. The following abilities are added: 1. display security state 2. enable/update passphrase 3. disable passphrase 4. freeze security 5. secure erase 6. overwrite 7. master passphrase enable/update v8: - Additional cleanup on test script. (Vishal) - Change load-keys script into internal command for ndctl. (Dan) v7: - Added option to provide path to key directory. (Vishal) - Cleaned up shell scripts. (Vishal) - Cleaned up documentation. (Vishal) - Addressed various comments from Vishal. v6: - Fix spelling and grammar errors for documentation. (Jing) - Change bool for indicate master passphrase and old passphrase to enum. - Fix key load script master key name. - Update to match v15 of kernel patch series. v5: - Updated to match latest kernel interface (encrypted keys) - Added overwrite support - Added support for DSM v1.8 master passphrase operations - Removed upcall related code - Moved security state to enum (Dan) - Change security output "security_state" to just "security". (Dan) - Break out enable and update passphrase operation. (Dan) - Security build can be compiled out when keyutils does not exist. (Dan) - Move all keyutils related operations to libndctl. (Dan) v4: - Updated to match latest kernel interface. - Added unit test for all security calls v3: - Added support to inject keys in order to update nvdimm security. v2: - Fixup the upcall util to match recent kernel updates for nvdimm security. --- Dave Jiang (12): ndctl: add support for display security state ndctl: add passphrase update to ndctl ndctl: add disable security support ndctl: add support for freeze security ndctl: add support for sanitize dimm ndctl: add unit test for security ops (minus overwrite) ndctl: add modprobe conf file and load-keys ndctl command ndctl: add overwrite operation support ndctl: add wait-overwrite support ndctl: master phassphrase management support ndctl: add master secure erase support ndctl: documentation for security and key management Documentation/ndctl/Makefile.am |9 Documentation/ndctl/intel-nvdimm-security.txt| 140 ++ Documentation/ndctl/ndctl-disable-passphrase.txt | 35 + Documentation/ndctl/ndctl-enable-passphrase.txt | 49 ++ Documentation/ndctl/ndctl-freeze-security.txt| 22 + Documentation/ndctl/ndctl-list.txt |8 Documentation/ndctl/ndctl-sanitize-dimm.txt | 50 ++ Documentation/ndctl/ndctl-update-passphrase.txt | 45 ++ Documentation/ndctl/ndctl-wait-overwrite.txt | 31 + Makefile.am |4 configure.ac | 19 + contrib/nvdimm-security.conf |1 ndctl.spec.in|3 ndctl/Makefile.am|6 ndctl/builtin.h |7 ndctl/dimm.c | 259 ++- ndctl/lib/Makefile.am|8 ndctl/lib/dimm.c | 202 ndctl/lib/keys.c | 528 ++ ndctl/lib/libndctl.sym | 20 + ndctl/libndctl.h | 84 ndctl/load-keys.c| 260 +++ ndctl/ndctl.c|7 test/Makefile.am |4 test/security.sh | 197 util/json.c | 31 + 26 files changed, 2015 insertions(+), 14 deletions(-) create mode 100644 Documentation/ndctl/intel-nvdimm-security.txt create mode 100644 Documentation/ndctl/ndctl-disable-passphrase.txt create mode 100644 Documentation/ndctl/ndctl-enable-passphrase.txt create mode 100644 Documentation/ndctl/ndctl-freeze-security.txt create mode 100644 Documentation/ndctl/ndctl-sanitize-dimm.txt create mode 100644 Documentation/ndctl/ndctl-update-passphrase.txt create mode 100644 Documentation/ndctl/ndctl-wait-overwrite.txt create mode 100644 contrib/nvdimm-security.conf create mode 100644 ndctl/lib/keys.c create mode 100644 ndctl/load-keys.c create mode 100755 test/security.sh -- Signature ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
[PATCH v8 03/12] ndctl: add disable security support
Add support for disable security to libndctl and also command line option
of "disable-passphrase" for ndctl. This provides a way to disable security
on the nvdimm.
Signed-off-by: Dave Jiang
---
Documentation/ndctl/Makefile.am |3 +-
Documentation/ndctl/ndctl-disable-passphrase.txt | 33 +++
ndctl/builtin.h |1 +
ndctl/dimm.c | 38 --
ndctl/lib/dimm.c |9 +
ndctl/lib/keys.c | 29 +
ndctl/lib/libndctl.sym |2 +
ndctl/libndctl.h |8 +
ndctl/ndctl.c|1 +
9 files changed, 120 insertions(+), 4 deletions(-)
create mode 100644 Documentation/ndctl/ndctl-disable-passphrase.txt
diff --git a/Documentation/ndctl/Makefile.am b/Documentation/ndctl/Makefile.am
index 7adb..31570a77 100644
--- a/Documentation/ndctl/Makefile.am
+++ b/Documentation/ndctl/Makefile.am
@@ -49,7 +49,8 @@ man1_MANS = \
ndctl-list.1 \
ndctl-monitor.1 \
ndctl-enable-passphrase.1 \
- ndctl-update-passphrase.1
+ ndctl-update-passphrase.1 \
+ ndctl-disable-passphrase.1
CLEANFILES = $(man1_MANS)
diff --git a/Documentation/ndctl/ndctl-disable-passphrase.txt
b/Documentation/ndctl/ndctl-disable-passphrase.txt
new file mode 100644
index ..49f25898
--- /dev/null
+++ b/Documentation/ndctl/ndctl-disable-passphrase.txt
@@ -0,0 +1,33 @@
+// SPDX-License-Identifier: GPL-2.0
+
+ndctl-disable-passphrase(1)
+===
+
+NAME
+
+ndctl-disable-passphrase - disabling passphrase for an NVDIMM
+
+SYNOPSIS
+
+[verse]
+'ndctl disable-passphrase' []
+
+DESCRIPTION
+---
+Provide a generic interface for disabling passphrase for NVDIMM.
+
+Search the user key ring for the associated NVDIMM. If not found,
+attempt to load the key blob from the default location. After disabling
+the passphrase, remove the key and the key blob.
+
+OPTIONS
+---
+::
+include::xable-dimm-options.txt[]
+
+-p::
+--key-path=::
+ Path to where key related files resides. This parameter is optional
+ and the default is set to /etc/ndctl/keys.
+
+include::../copyright.txt[]
diff --git a/ndctl/builtin.h b/ndctl/builtin.h
index 231fda25..821ea690 100644
--- a/ndctl/builtin.h
+++ b/ndctl/builtin.h
@@ -34,4 +34,5 @@ int cmd_update_firmware(int argc, const char **argv, struct
ndctl_ctx *ctx);
int cmd_inject_smart(int argc, const char **argv, struct ndctl_ctx *ctx);
int cmd_passphrase_setup(int argc, const char **argv, struct ndctl_ctx *ctx);
int cmd_passphrase_update(int argc, const char **argv, struct ndctl_ctx *ctx);
+int cmd_disable_passphrase(int argc, const char **argv, struct ndctl_ctx *ctx);
#endif /* _NDCTL_BUILTIN_H_ */
diff --git a/ndctl/dimm.c b/ndctl/dimm.c
index 1ab6b29f..4f0466a1 100644
--- a/ndctl/dimm.c
+++ b/ndctl/dimm.c
@@ -864,6 +864,18 @@ static int action_key_update(struct ndctl_dimm *dimm,
param.key_path);
}
+static int action_passphrase_disable(struct ndctl_dimm *dimm,
+ struct action_context *actx)
+{
+ if (!ndctl_dimm_security_supported(dimm)) {
+ error("%s: security operation not supported\n",
+ ndctl_dimm_get_devname(dimm));
+ return -EOPNOTSUPP;
+ }
+
+ return ndctl_dimm_disable_key(dimm, param.key_path);
+}
+
static int __action_init(struct ndctl_dimm *dimm,
enum ndctl_namespace_version version, int chk_only)
{
@@ -953,12 +965,14 @@ OPT_BOOLEAN('f', "force", ¶m.force, \
OPT_STRING('V', "label-version", ¶m.labelversion, "version-number", \
"namespace label specification version (default: 1.1)")
-#define KEY_OPTIONS() \
-OPT_STRING('m', "master-key", ¶m.master_key, ":", \
- "master key for security"), \
+#define KEY_BASE_OPTIONS() \
OPT_FILENAME('p', "key-path", ¶m.key_path, "key-path", \
"override the default key path")
+#define KEY_OPTIONS() \
+OPT_STRING('m', "master-key", ¶m.master_key, ":", \
+ "master key for security")
+
static const struct option read_options[] = {
BASE_OPTIONS(),
READ_OPTIONS(),
@@ -988,8 +1002,15 @@ static const struct option init_options[] = {
OPT_END(),
};
+static const struct option key_base_options[] = {
+ BASE_OPTIONS(),
+ KEY_BASE_OPTIONS(),
+ OPT_END(),
+};
+
static const struct option key_options[] = {
BASE_OPTIONS(),
+ KEY_BASE_OPTIONS(),
KEY_OPTIONS(),
OPT_END(),
};
@@ -1253,3 +1274,14 @@ int cmd_passphrase_setup(int argc, const char **argv,
struct ndctl_ctx *ctx)
count > 1 ? "s" : "");
return count >= 0 ? 0 : EXIT_FAILURE;
}
+
+int cmd_disable_passphrase(int argc, const char **argv, void *ctx)
+{
[PATCH v8 10/12] ndctl: master phassphrase management support
Adding master passphrase enabling and update to ndctl. This is a new
feature from Intel DSM v1.8.
Signed-off-by: Dave Jiang
---
Documentation/ndctl/ndctl-enable-passphrase.txt |7 +
Documentation/ndctl/ndctl-update-passphrase.txt |7 +
ndctl/dimm.c| 13 +-
ndctl/lib/dimm.c|9 ++
ndctl/lib/keys.c| 127 ---
ndctl/lib/libndctl.sym |1
ndctl/libndctl.h| 14 ++-
7 files changed, 130 insertions(+), 48 deletions(-)
diff --git a/Documentation/ndctl/ndctl-enable-passphrase.txt
b/Documentation/ndctl/ndctl-enable-passphrase.txt
index c14a206c..c025b1c3 100644
--- a/Documentation/ndctl/ndctl-enable-passphrase.txt
+++ b/Documentation/ndctl/ndctl-enable-passphrase.txt
@@ -29,7 +29,7 @@ OPTIONS
include::xable-dimm-options.txt[]
-m::
---master=::
+--master-key=::
Key name for the master key used to seal the NVDIMM security keys.
The format would be :
i.e.: trusted:master-nvdimm
@@ -39,4 +39,9 @@ include::xable-dimm-options.txt[]
Path to where key related files resides. This parameter is optional
and the default is set to /etc/ndctl/keys.
+-M::
+--master-passphrase::
+ Indicates that we are managing the master passphrase instead of the
+ user passphrase.
+
include::../copyright.txt[]
diff --git a/Documentation/ndctl/ndctl-update-passphrase.txt
b/Documentation/ndctl/ndctl-update-passphrase.txt
index dd6e4e4e..8b5dfe01 100644
--- a/Documentation/ndctl/ndctl-update-passphrase.txt
+++ b/Documentation/ndctl/ndctl-update-passphrase.txt
@@ -26,7 +26,7 @@ OPTIONS
include::xable-dimm-options.txt[]
-m::
---master::
+--master-key=::
New key name for the master key to seal the new nvdimm key, or the
existing master key name. i.e trusted:master-key.
@@ -35,4 +35,9 @@ include::xable-dimm-options.txt[]
Path to where key related files resides. This parameter is optional
and the default is set to /etc/ndctl/keys.
+-M::
+--master-passphrase::
+ Parameter to indicate that we are managing the master passphrase
+ instead of the user passphrase.
+
include::../copyright.txt[]
diff --git a/ndctl/dimm.c b/ndctl/dimm.c
index ce477981..4875e414 100644
--- a/ndctl/dimm.c
+++ b/ndctl/dimm.c
@@ -49,6 +49,7 @@ static struct parameters {
const char *master_key;
bool crypto_erase;
bool overwrite;
+ bool master_pass;
bool force;
bool json;
bool verbose;
@@ -849,8 +850,8 @@ static int action_key_enable(struct ndctl_dimm *dimm,
return -EOPNOTSUPP;
}
- return ndctl_dimm_enable_key(dimm, param.master_key,
- param.key_path);
+ return ndctl_dimm_enable_key(dimm, param.master_key, param.key_path,
+ param.master_pass ? ND_MASTER_KEY : ND_USER_KEY);
}
static int action_key_update(struct ndctl_dimm *dimm,
@@ -862,8 +863,8 @@ static int action_key_update(struct ndctl_dimm *dimm,
return -EOPNOTSUPP;
}
- return ndctl_dimm_update_key(dimm, param.master_key,
- param.key_path);
+ return ndctl_dimm_update_key(dimm, param.master_key, param.key_path,
+ param.master_pass ? ND_MASTER_KEY : ND_USER_KEY);
}
static int action_passphrase_disable(struct ndctl_dimm *dimm,
@@ -1044,7 +1045,9 @@ OPT_FILENAME('p', "key-path", ¶m.key_path,
"key-path", \
#define KEY_OPTIONS() \
OPT_STRING('m', "master-key", ¶m.master_key, ":", \
- "master key for security")
+ "master key for security"), \
+OPT_BOOLEAN('M', "master-passphrase", ¶m.master_pass, \
+ "use master passphrase")
#define SANITIZE_OPTIONS() \
OPT_BOOLEAN('c', "crypto-erase", ¶m.crypto_erase, \
diff --git a/ndctl/lib/dimm.c b/ndctl/lib/dimm.c
index eb950331..dc945296 100644
--- a/ndctl/lib/dimm.c
+++ b/ndctl/lib/dimm.c
@@ -771,3 +771,12 @@ NDCTL_EXPORT int ndctl_dimm_wait_overwrite(struct
ndctl_dimm *dimm)
close(fd);
return rc;
}
+
+NDCTL_EXPORT int ndctl_dimm_update_master_passphrase(struct ndctl_dimm *dimm,
+ long ckey, long nkey)
+{
+ char buf[SYSFS_ATTR_SIZE];
+
+ sprintf(buf, "master_update %ld %ld\n", ckey, nkey);
+ return write_security(dimm, buf);
+}
diff --git a/ndctl/lib/keys.c b/ndctl/lib/keys.c
index beb5ab3b..09ceeb57 100644
--- a/ndctl/lib/keys.c
+++ b/ndctl/lib/keys.c
@@ -17,7 +17,7 @@
#include "private.h"
static int get_key_path(struct ndctl_dimm *dimm, char *path,
- enum ndctl_key_type key_type, const char *keypath)
+ const char *keypath, enum ndctl_key_type key_type)
{
struct ndctl_ctx *ctx = ndctl_dimm_get_ctx(dimm);
char hostname[HOST_NAME_MAX];
@@ -29,16 +29,29 @@ static int get_key_path(struct ndctl_dimm *dimm, char *path,
[PATCH v8 05/12] ndctl: add support for sanitize dimm
Add support to secure erase to libndctl and also command line option
of "sanitize-dimm" for ndctl. This will initiate the request to crypto
erase a DIMM.
Signed-off-by: Dave Jiang
---
Documentation/ndctl/Makefile.am |3 +-
Documentation/ndctl/ndctl-sanitize-dimm.txt | 38
ndctl/builtin.h |1 +
ndctl/dimm.c| 52 +++
ndctl/lib/dimm.c|8
ndctl/lib/keys.c| 21 +--
ndctl/lib/libndctl.sym |2 +
ndctl/libndctl.h|9 +
ndctl/ndctl.c |1 +
9 files changed, 131 insertions(+), 4 deletions(-)
create mode 100644 Documentation/ndctl/ndctl-sanitize-dimm.txt
diff --git a/Documentation/ndctl/Makefile.am b/Documentation/ndctl/Makefile.am
index a97f193d..bbea9674 100644
--- a/Documentation/ndctl/Makefile.am
+++ b/Documentation/ndctl/Makefile.am
@@ -51,7 +51,8 @@ man1_MANS = \
ndctl-enable-passphrase.1 \
ndctl-update-passphrase.1 \
ndctl-disable-passphrase.1 \
- ndctl-freeze-security.1
+ ndctl-freeze-security.1 \
+ ndctl-sanitize-dimm.1
CLEANFILES = $(man1_MANS)
diff --git a/Documentation/ndctl/ndctl-sanitize-dimm.txt
b/Documentation/ndctl/ndctl-sanitize-dimm.txt
new file mode 100644
index ..79629964
--- /dev/null
+++ b/Documentation/ndctl/ndctl-sanitize-dimm.txt
@@ -0,0 +1,38 @@
+// SPDX-License-Identifier: GPL-2.0
+
+ndctl-sanitize-dimm(1)
+==
+
+NAME
+
+ndctl-sanitize-dimm - sanitize the data on the NVDIMM
+
+SYNOPSIS
+
+[verse]
+'ndctl sanitize' []
+
+DESCRIPTION
+---
+Provide a generic interface to crypto erase a NVDIMM.
+
+Search the user key ring for the associated NVDIMM. If not found,
+attempt to load the key blob from the default location. After disabling
+the passphrase, remove the key and the key blob.
+
+OPTIONS
+---
+::
+include::xable-dimm-options.txt[]
+
+-p::
+--key-path=::
+ Path to where key related files resides. This parameter is optional
+ and the default is set to /etc/ndctl/keys.
+
+-c::
+--crypto-erase::
+ Replaces encryption keys and securely erases the data. This does not
+ change label data. This is the default sanitize method.
+
+include::../copyright.txt[]
diff --git a/ndctl/builtin.h b/ndctl/builtin.h
index f7469598..55bee47c 100644
--- a/ndctl/builtin.h
+++ b/ndctl/builtin.h
@@ -36,4 +36,5 @@ int cmd_passphrase_setup(int argc, const char **argv, struct
ndctl_ctx *ctx);
int cmd_passphrase_update(int argc, const char **argv, struct ndctl_ctx *ctx);
int cmd_disable_passphrase(int argc, const char **argv, struct ndctl_ctx *ctx);
int cmd_freeze_security(int argc, const char **argv, struct ndctl_ctx *ctx);
+int cmd_sanitize_dimm(int argc, const char **argv, struct ndctl_ctx *ctx);
#endif /* _NDCTL_BUILTIN_H_ */
diff --git a/ndctl/dimm.c b/ndctl/dimm.c
index 19301791..a91b40d5 100644
--- a/ndctl/dimm.c
+++ b/ndctl/dimm.c
@@ -47,6 +47,7 @@ static struct parameters {
const char *labelversion;
const char *key_path;
const char *master_key;
+ bool crypto_erase;
bool force;
bool json;
bool verbose;
@@ -894,6 +895,35 @@ static int action_security_freeze(struct ndctl_dimm *dimm,
return rc;
}
+static int action_sanitize_dimm(struct ndctl_dimm *dimm,
+ struct action_context *actx)
+{
+ int rc;
+
+ if (!ndctl_dimm_security_supported(dimm)) {
+ error("%s: security operation not supported\n",
+ ndctl_dimm_get_devname(dimm));
+ return -EOPNOTSUPP;
+ }
+
+ /*
+* Setting crypto erase to be default. The other method will be
+* overwrite.
+*/
+ if (!param.crypto_erase) {
+ param.crypto_erase = true;
+ printf("No santize method passed in, default to
crypto-erase\n");
+ }
+
+ if (param.crypto_erase) {
+ rc = ndctl_dimm_secure_erase_key(dimm, param.key_path);
+ if (rc < 0)
+ return rc;
+ }
+
+ return 0;
+}
+
static int __action_init(struct ndctl_dimm *dimm,
enum ndctl_namespace_version version, int chk_only)
{
@@ -991,6 +1021,10 @@ OPT_FILENAME('p', "key-path", ¶m.key_path,
"key-path", \
OPT_STRING('m', "master-key", ¶m.master_key, ":", \
"master key for security")
+#define SANITIZE_OPTIONS() \
+OPT_BOOLEAN('c', "crypto-erase", ¶m.crypto_erase, \
+ "crypto erase a dimm")
+
static const struct option read_options[] = {
BASE_OPTIONS(),
READ_OPTIONS(),
@@ -1033,6 +1067,13 @@ static const struct option key_options[] = {
OPT_END(),
};
+static const struct option sanitize_options[] = {
+ BASE_OPTIONS(),
+ KEY_BASE_OPTIONS(),
[PATCH v8 02/12] ndctl: add passphrase update to ndctl
Add API call for triggering sysfs knob to update the security for a DIMM
in libndctl. Also add the ndctl "update-passphrase" to trigger the
operation.
Signed-off-by: Dave Jiang
---
Documentation/ndctl/Makefile.am |4
Documentation/ndctl/ndctl-enable-passphrase.txt | 42 ++
Documentation/ndctl/ndctl-update-passphrase.txt | 38 ++
configure.ac| 19 +
ndctl.spec.in |2
ndctl/Makefile.am |3
ndctl/builtin.h |2
ndctl/dimm.c| 94 +-
ndctl/lib/Makefile.am |8
ndctl/lib/dimm.c| 39 ++
ndctl/lib/keys.c| 390 +++
ndctl/lib/libndctl.sym |4
ndctl/libndctl.h| 35 ++
ndctl/ndctl.c |2
14 files changed, 669 insertions(+), 13 deletions(-)
create mode 100644 Documentation/ndctl/ndctl-enable-passphrase.txt
create mode 100644 Documentation/ndctl/ndctl-update-passphrase.txt
create mode 100644 ndctl/lib/keys.c
diff --git a/Documentation/ndctl/Makefile.am b/Documentation/ndctl/Makefile.am
index a30b139b..7adb 100644
--- a/Documentation/ndctl/Makefile.am
+++ b/Documentation/ndctl/Makefile.am
@@ -47,7 +47,9 @@ man1_MANS = \
ndctl-inject-smart.1 \
ndctl-update-firmware.1 \
ndctl-list.1 \
- ndctl-monitor.1
+ ndctl-monitor.1 \
+ ndctl-enable-passphrase.1 \
+ ndctl-update-passphrase.1
CLEANFILES = $(man1_MANS)
diff --git a/Documentation/ndctl/ndctl-enable-passphrase.txt
b/Documentation/ndctl/ndctl-enable-passphrase.txt
new file mode 100644
index ..c14a206c
--- /dev/null
+++ b/Documentation/ndctl/ndctl-enable-passphrase.txt
@@ -0,0 +1,42 @@
+// SPDX-License-Identifier: GPL-2.0
+
+ndctl-enable-passphrase(1)
+==
+
+NAME
+
+ndctl-enable-passphrase - enable the security passphrase for a NVDIMM
+
+SYNOPSIS
+
+[verse]
+'ndctl enable-passphrase' []
+
+DESCRIPTION
+---
+Provide a command to enable the security passphrase for the NVDIMM.
+It is expected that the master key has already been loaded into the user
+key ring. The encrypted key blobs will be created in /etc/nvdimm directory
+with the file name of "nvdimm--.blob".
+
+The command will fail if the nvdimm key is already in the user key ring and/or
+the key blob already resides in /etc/nvdimm. Do not touch the /etc/nvdimm
+directory and let ndctl manage the keys, unless you know what you are doing.
+
+OPTIONS
+---
+::
+include::xable-dimm-options.txt[]
+
+-m::
+--master=::
+ Key name for the master key used to seal the NVDIMM security keys.
+ The format would be :
+ i.e.: trusted:master-nvdimm
+
+-p::
+--key-path=::
+ Path to where key related files resides. This parameter is optional
+ and the default is set to /etc/ndctl/keys.
+
+include::../copyright.txt[]
diff --git a/Documentation/ndctl/ndctl-update-passphrase.txt
b/Documentation/ndctl/ndctl-update-passphrase.txt
new file mode 100644
index ..dd6e4e4e
--- /dev/null
+++ b/Documentation/ndctl/ndctl-update-passphrase.txt
@@ -0,0 +1,38 @@
+// SPDX-License-Identifier: GPL-2.0
+
+ndctl-update-passphrase(1)
+==
+
+NAME
+
+ndctl-update-passphrase - update the security passphrase for a NVDIMM
+
+SYNOPSIS
+
+[verse]
+'ndctl update-passphrase' []
+
+DESCRIPTION
+---
+Provide a command to update the security key for NVDIMM.
+It is expected that the current and the new (if new master key is desired)
+master key has already been loaded into the user key ring. The new encrypted
+key blobs will be created in /etc/nvdimm directory
+with the file name of "nvdimm--.blob".
+
+OPTIONS
+---
+::
+include::xable-dimm-options.txt[]
+
+-m::
+--master::
+ New key name for the master key to seal the new nvdimm key, or the
+ existing master key name. i.e trusted:master-key.
+
+-p::
+--key-path=::
+ Path to where key related files resides. This parameter is optional
+ and the default is set to /etc/ndctl/keys.
+
+include::../copyright.txt[]
diff --git a/configure.ac b/configure.ac
index aa07ec7b..22efc871 100644
--- a/configure.ac
+++ b/configure.ac
@@ -154,6 +154,7 @@ fi
AC_SUBST([systemd_unitdir])
AM_CONDITIONAL([ENABLE_SYSTEMD_UNITS], [test "x$with_systemd" = "xyes"])
+
ndctl_monitorconfdir=${sysconfdir}/ndctl
ndctl_monitorconf=monitor.conf
AC_SUBST([ndctl_monitorconfdir])
@@ -161,6 +162,24 @@ AC_SUBST([ndctl_monitorconf])
AC_DEFINE_UNQUOTED(NDCTL_CONF_FILE,
["$ndctl_monitorconfdir/$ndctl_monitorconf"],
[default ndctl monitor conf path])
+AC_ARG_WITH([keyutils],
+ AS_HELP_STRING([--with-keyutils],
+ [Enable keyutils functionality (security).
@<:@de
[PATCH v8 07/12] ndctl: add modprobe conf file and load-keys ndctl command
Add load-keys command to ndctl. This will attempt to load the master key
and the related encrypted keys for nvdimms. Also add reference config file
for modprobe.d in order to call ndctl load-keys and inject keys associated
with the nvdimms into the kernel user ring for unlock.
Signed-off-by: Dave Jiang
---
Documentation/ndctl/Makefile.am |3
Makefile.am |4 +
contrib/nvdimm-security.conf|1
ndctl.spec.in |1
ndctl/Makefile.am |3
ndctl/builtin.h |1
ndctl/lib/keys.c| 60 ++---
ndctl/lib/libndctl.sym |1
ndctl/libndctl.h|2
ndctl/load-keys.c | 260 +++
ndctl/ndctl.c |1
11 files changed, 314 insertions(+), 23 deletions(-)
create mode 100644 contrib/nvdimm-security.conf
create mode 100644 ndctl/load-keys.c
diff --git a/Documentation/ndctl/Makefile.am b/Documentation/ndctl/Makefile.am
index bbea9674..0224cccd 100644
--- a/Documentation/ndctl/Makefile.am
+++ b/Documentation/ndctl/Makefile.am
@@ -52,7 +52,8 @@ man1_MANS = \
ndctl-update-passphrase.1 \
ndctl-disable-passphrase.1 \
ndctl-freeze-security.1 \
- ndctl-sanitize-dimm.1
+ ndctl-sanitize-dimm.1 \
+ ndctl-load-keys.1
CLEANFILES = $(man1_MANS)
diff --git a/Makefile.am b/Makefile.am
index e0c463a3..df8797ef 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -42,6 +42,10 @@ bashcompletiondir = $(BASH_COMPLETION_DIR)
dist_bashcompletion_DATA = contrib/ndctl
endif
+modprobe_file = contrib/nvdimm-security.conf
+modprobedir = $(sysconfdir)/modprobe.d/
+modprobe_DATA = $(modprobe_file)
+
noinst_LIBRARIES = libccan.a
libccan_a_SOURCES = \
ccan/str/str.h \
diff --git a/contrib/nvdimm-security.conf b/contrib/nvdimm-security.conf
new file mode 100644
index ..e2bb7c0a
--- /dev/null
+++ b/contrib/nvdimm-security.conf
@@ -0,0 +1 @@
+install libnvdimm /usr/bin/ndctl load-keys ; /sbin/modprobe --ignore-install
libnvdimm $CMDLINE_OPTS
diff --git a/ndctl.spec.in b/ndctl.spec.in
index 66466db6..afed8a43 100644
--- a/ndctl.spec.in
+++ b/ndctl.spec.in
@@ -121,6 +121,7 @@ make check
%{_sysconfdir}/ndctl/monitor.conf
%{_unitdir}/ndctl-monitor.service
%{_sysconfdir}/ndctl/keys/
+%{_sysconfdir}/modprobe.d/nvdimm-security.conf
%files -n daxctl
%defattr(-,root,root)
diff --git a/ndctl/Makefile.am b/ndctl/Makefile.am
index 120941a4..eafae05d 100644
--- a/ndctl/Makefile.am
+++ b/ndctl/Makefile.am
@@ -16,7 +16,8 @@ ndctl_SOURCES = ndctl.c \
util/json-firmware.c \
inject-error.c \
inject-smart.c \
- monitor.c
+ monitor.c \
+ load-keys.c
if ENABLE_DESTRUCTIVE
ndctl_SOURCES += ../test/blk_namespaces.c \
diff --git a/ndctl/builtin.h b/ndctl/builtin.h
index 55bee47c..2cdc0590 100644
--- a/ndctl/builtin.h
+++ b/ndctl/builtin.h
@@ -37,4 +37,5 @@ int cmd_passphrase_update(int argc, const char **argv, struct
ndctl_ctx *ctx);
int cmd_disable_passphrase(int argc, const char **argv, struct ndctl_ctx *ctx);
int cmd_freeze_security(int argc, const char **argv, struct ndctl_ctx *ctx);
int cmd_sanitize_dimm(int argc, const char **argv, struct ndctl_ctx *ctx);
+int cmd_load_keys(int argc, const char **argv, struct ndctl_ctx *ctx);
#endif /* _NDCTL_BUILTIN_H_ */
diff --git a/ndctl/lib/keys.c b/ndctl/lib/keys.c
index 42281394..39c4143c 100644
--- a/ndctl/lib/keys.c
+++ b/ndctl/lib/keys.c
@@ -71,16 +71,23 @@ static int get_key_desc(struct ndctl_dimm *dimm, char *desc,
return 0;
}
-static char *load_key_blob(struct ndctl_ctx *ctx, const char *path, int *size)
+NDCTL_EXPORT char *ndctl_load_key_blob(struct ndctl_ctx *ctx,
+ const char *path, int *size, const char *postfix, int dirfd)
{
struct stat st;
- FILE *bfile = NULL;
- ssize_t read;
- int rc;
+ ssize_t read_bytes = 0;
+ int rc, fd;
char *blob, *pl;
char prefix[] = "load ";
- rc = stat(path, &st);
+ fd = openat(dirfd, path, O_RDONLY);
+ if (fd < 0) {
+ err(ctx, "failed to open file %s: %s\n",
+ path, strerror(errno));
+ return NULL;
+ }
+
+ rc = fstat(fd, &st);
if (rc < 0) {
err(ctx, "stat: %s\n", strerror(errno));
return NULL;
@@ -96,31 +103,42 @@ static char *load_key_blob(struct ndctl_ctx *ctx, const
char *path, int *size)
}
*size = st.st_size + sizeof(prefix) - 1;
+ /*
+* We need to increment postfix and space.
+* "keyhandle=" is 10 bytes, plus null termination.
+*/
+ if (postfix)
+ *size += strlen(postfix) + 10 + 1;
blob = malloc(*size);
if (!blob) {
err(ctx, "Unable to allocate memory for blob\n");
return NULL;
}
[PATCH v8 06/12] ndctl: add unit test for security ops (minus overwrite)
Add unit test for security enable, disable, update, erase, unlock, and
freeze.
Signed-off-by: Dave Jiang
---
test/Makefile.am |4 +
test/security.sh | 197 ++
2 files changed, 201 insertions(+)
create mode 100755 test/security.sh
diff --git a/test/Makefile.am b/test/Makefile.am
index ebdd23f6..42009c31 100644
--- a/test/Makefile.am
+++ b/test/Makefile.am
@@ -27,6 +27,10 @@ TESTS =\
max_available_extent_ns.sh \
pfn-meta-errors.sh
+if ENABLE_KEYUTILS
+TESTS += security.sh
+endif
+
check_PROGRAMS =\
libndctl \
dsm-fail \
diff --git a/test/security.sh b/test/security.sh
new file mode 100755
index ..9f69b481
--- /dev/null
+++ b/test/security.sh
@@ -0,0 +1,197 @@
+#!/bin/bash -Ex
+# SPDX-License-Identifier: GPL-2.0
+# Copyright(c) 2018 Intel Corporation. All rights reserved.
+
+rc=77
+dev=""
+id=""
+dev_no=""
+keypath="/etc/ndctl/keys"
+masterkey="nvdimm-master-test"
+masterpath="$keypath/$masterkey"
+
+. ./common
+
+lockpath="/sys/devices/platform/${NFIT_TEST_BUS0}/nfit_test_dimm/test_dimm"
+
+trap 'err $LINENO' ERR
+
+setup()
+{
+ $NDCTL disable-region -b "$NFIT_TEST_BUS0" all
+}
+
+detect()
+{
+ dev="$($NDCTL list -b "$NFIT_TEST_BUS0" -D | jq -r .[0].dev)"
+ [ -n "$dev" ] || err "$LINENO"
+ id="$($NDCTL list -b "$NFIT_TEST_BUS0" -D | jq -r .[0].id)"
+ [ -n "$id" ] || err "$LINENO"
+}
+
+setup_keys()
+{
+ keyctl add user "$masterkey" "$(dd if=/dev/urandom bs=1 count=32
2>/dev/null)" @u
+ keyctl pipe "$(keyctl search @u user $masterkey)" > "$masterpath"
+}
+
+test_cleanup()
+{
+ if keyctl search @u encrypted nvdimm:"$id"; then
+ keyctl unlink "$(keyctl search @u encrypted nvdimm:"$id")"
+ fi
+
+ if keyctl search @u user "$masterkey"; then
+ keyctl unlink "$(keyctl search @u user $masterkey)"
+ fi
+
+ if [ -f "$keypath"/nvdimm_"$id"_"$(hostname)".blob ]; then
+ rm -f "$keypath"/nvdimm_"$id"_"$(hostname)".blob
+ fi
+
+ if [ -f $masterpath ]; then
+ rm -f "$masterpath"
+ fi
+}
+
+lock_dimm()
+{
+ $NDCTL disable-dimm "$dev"
+ dev_no="${dev#nmem}"
+ echo 1 > "${lockpath}${dev_no}/lock_dimm"
+ sstate="$(get_security_state)"
+ if [ "$sstate" != "locked" ]; then
+ echo "Incorrect security state: $sstate expected: disabled"
+ err $LINENO
+ fi
+}
+
+get_security_state()
+{
+ $NDCTL list -i -b "$NFIT_TEST_BUS0" -d "$dev" | jq -r
.[].dimms[0].security
+}
+
+enable_passphrase()
+{
+ $NDCTL enable-passphrase -m user:"$masterkey" "$dev"
+ sstate="$(get_security_state)"
+ if [ "$sstate" != "unlocked" ]; then
+ echo "Incorrect security state: $sstate expected: unlocked"
+ err $LINENO
+ fi
+}
+
+disable_passphrase()
+{
+ $NDCTL disable-passphrase "$dev"
+ sstate="$(get_security_state)"
+ if [ "$sstate" != "disabled" ]; then
+ echo "Incorrect security state: $sstate expected: disabled"
+ err $LINENO
+ fi
+}
+
+erase_security()
+{
+ $NDCTL sanitize-dimm -c "$dev"
+ sstate="$(get_security_state)"
+ if [ "$sstate" != "disabled" ]; then
+ echo "Incorrect security state: $sstate expected: disabled"
+ err $LINENO
+ fi
+}
+
+update_security()
+{
+ $NDCTL update-passphrase -m user:"$masterkey" "$dev"
+ sstate="$(get_security_state)"
+ if [ "$sstate" != "unlocked" ]; then
+ echo "Incorrect security state: $sstate expected: unlocked"
+ err $LINENO
+ fi
+}
+
+freeze_security()
+{
+ $NDCTL freeze-security "$dev"
+}
+
+test_1_security_enable_and_disable()
+{
+ enable_passphrase
+ disable_passphrase
+}
+
+test_2_security_enable_and_update()
+{
+ enable_passphrase
+ update_security
+ disable_passphrase
+}
+
+test_3_security_enable_and_erase()
+{
+ enable_passphrase
+ erase_security
+}
+
+test_4_security_unlock()
+{
+ enable_passphrase
+ lock_dimm
+ $NDCTL enable-dimm "$dev"
+ sstate="$(get_security_state)"
+ if [ "$sstate" != "unlocked" ]; then
+ echo "Incorrect security state: $sstate expected: unlocked"
+ err $LINENO
+ fi
+ $NDCTL disable-region -b "$NFIT_TEST_BUS0" all
+ disable_passphrase
+}
+
+# this should always be the last test. with security frozen, nfit_test must
+# be removed and is no longer usable
+test_5_security_freeze()
+{
+ enable_passphrase
+ freeze_security
+ sstate="$(get_security_state)"
+ if [ "$sstate" != "frozen" ]; then
+ echo "Incorrect security state: $sstate expected: frozen"
+ err $LINENO
+ fi
+ $NDCTL disable-passphrase "$dev" && { echo "disable succeed after
frozen"; }
+ sstate="$(get_security_state)"
[PATCH v8 04/12] ndctl: add support for freeze security
Add support for freeze security to libndctl and also command line option
of "freeze-security" for ndctl. This will lock the ability to make changes
to the NVDIMM security.
Signed-off-by: Dave Jiang
---
Documentation/ndctl/Makefile.am |3 ++-
Documentation/ndctl/ndctl-freeze-security.txt | 20 ++
ndctl/builtin.h |1 +
ndctl/dimm.c | 28 +
ndctl/lib/dimm.c |5
ndctl/lib/libndctl.sym|1 +
ndctl/libndctl.h |1 +
ndctl/ndctl.c |1 +
8 files changed, 59 insertions(+), 1 deletion(-)
create mode 100644 Documentation/ndctl/ndctl-freeze-security.txt
diff --git a/Documentation/ndctl/Makefile.am b/Documentation/ndctl/Makefile.am
index 31570a77..a97f193d 100644
--- a/Documentation/ndctl/Makefile.am
+++ b/Documentation/ndctl/Makefile.am
@@ -50,7 +50,8 @@ man1_MANS = \
ndctl-monitor.1 \
ndctl-enable-passphrase.1 \
ndctl-update-passphrase.1 \
- ndctl-disable-passphrase.1
+ ndctl-disable-passphrase.1 \
+ ndctl-freeze-security.1
CLEANFILES = $(man1_MANS)
diff --git a/Documentation/ndctl/ndctl-freeze-security.txt
b/Documentation/ndctl/ndctl-freeze-security.txt
new file mode 100644
index ..4e9d2d61
--- /dev/null
+++ b/Documentation/ndctl/ndctl-freeze-security.txt
@@ -0,0 +1,20 @@
+// SPDX-License-Identifier: GPL-2.0
+
+ndctl-freeze-security(1)
+
+
+NAME
+
+ndctl-freeze-security - enabling or freeze the security for an NVDIMM
+
+SYNOPSIS
+
+[verse]
+'ndctl freeze-security'
+
+DESCRIPTION
+---
+Provide a generic interface to freeze the security for NVDIMM. Once security
+is frozen, no other security operations can succeed until reboot happens.
+
+include::../copyright.txt[]
diff --git a/ndctl/builtin.h b/ndctl/builtin.h
index 821ea690..f7469598 100644
--- a/ndctl/builtin.h
+++ b/ndctl/builtin.h
@@ -35,4 +35,5 @@ int cmd_inject_smart(int argc, const char **argv, struct
ndctl_ctx *ctx);
int cmd_passphrase_setup(int argc, const char **argv, struct ndctl_ctx *ctx);
int cmd_passphrase_update(int argc, const char **argv, struct ndctl_ctx *ctx);
int cmd_disable_passphrase(int argc, const char **argv, struct ndctl_ctx *ctx);
+int cmd_freeze_security(int argc, const char **argv, struct ndctl_ctx *ctx);
#endif /* _NDCTL_BUILTIN_H_ */
diff --git a/ndctl/dimm.c b/ndctl/dimm.c
index 4f0466a1..19301791 100644
--- a/ndctl/dimm.c
+++ b/ndctl/dimm.c
@@ -876,6 +876,24 @@ static int action_passphrase_disable(struct ndctl_dimm
*dimm,
return ndctl_dimm_disable_key(dimm, param.key_path);
}
+static int action_security_freeze(struct ndctl_dimm *dimm,
+ struct action_context *actx)
+{
+ int rc;
+
+ if (!ndctl_dimm_security_supported(dimm)) {
+ error("%s: security operation not supported\n",
+ ndctl_dimm_get_devname(dimm));
+ return -EOPNOTSUPP;
+ }
+
+ rc = ndctl_dimm_freeze_security(dimm);
+ if (rc < 0)
+ error("Failed to freeze security for %s\n",
+ ndctl_dimm_get_devname(dimm));
+ return rc;
+}
+
static int __action_init(struct ndctl_dimm *dimm,
enum ndctl_namespace_version version, int chk_only)
{
@@ -1285,3 +1303,13 @@ int cmd_disable_passphrase(int argc, const char **argv,
void *ctx)
count > 1 ? "s" : "");
return count >= 0 ? 0 : EXIT_FAILURE;
}
+
+int cmd_freeze_security(int argc, const char **argv, void *ctx)
+{
+ int count = dimm_action(argc, argv, ctx, action_security_freeze,
base_options,
+ "ndctl freeze-security [..]
[]");
+
+ fprintf(stderr, "security freezed %d nmem%s.\n", count >= 0 ? count : 0,
+ count > 1 ? "s" : "");
+ return count >= 0 ? 0 : EXIT_FAILURE;
+}
diff --git a/ndctl/lib/dimm.c b/ndctl/lib/dimm.c
index 076ccbf6..8f0f0486 100644
--- a/ndctl/lib/dimm.c
+++ b/ndctl/lib/dimm.c
@@ -672,3 +672,8 @@ NDCTL_EXPORT int ndctl_dimm_disable_passphrase(struct
ndctl_dimm *dimm,
sprintf(buf, "disable %ld\n", key);
return write_security(dimm, buf);
}
+
+NDCTL_EXPORT int ndctl_dimm_freeze_security(struct ndctl_dimm *dimm)
+{
+ return write_security(dimm, "freeze");
+}
diff --git a/ndctl/lib/libndctl.sym b/ndctl/lib/libndctl.sym
index 90038e75..a1c56060 100644
--- a/ndctl/lib/libndctl.sym
+++ b/ndctl/lib/libndctl.sym
@@ -395,4 +395,5 @@ global:
ndctl_dimm_update_passphrase;
ndctl_dimm_disable_passphrase;
ndctl_dimm_disable_key;
+ ndctl_dimm_freeze_security;
} LIBNDCTL_18;
diff --git a/ndctl/libndctl.h b/ndctl/libndctl.h
index b1192960..3862bbfd 100644
--- a/ndctl/libndctl.h
+++ b/ndctl/libndctl.h
@@ -702,6 +702,7 @@ bool ndctl_dimm_security_supp
[PATCH v8 01/12] ndctl: add support for display security state
Adding libndctl API call for retrieving security state for a DIMM and also
adding support to ndctl list for displaying security state.
Signed-off-by: Dave Jiang
---
Documentation/ndctl/ndctl-list.txt |8
ndctl/lib/dimm.c | 37
ndctl/lib/libndctl.sym |5 +
ndctl/libndctl.h | 13 +
util/json.c| 31 ++
5 files changed, 94 insertions(+)
diff --git a/Documentation/ndctl/ndctl-list.txt
b/Documentation/ndctl/ndctl-list.txt
index e24c8f40..bdd69add 100644
--- a/Documentation/ndctl/ndctl-list.txt
+++ b/Documentation/ndctl/ndctl-list.txt
@@ -98,6 +98,14 @@ include::xable-region-options.txt[]
-D::
--dimms::
Include dimm info in the listing
+[verse]
+{
+ "dev":"nmem0",
+ "id":"cdab-0a-07e0-",
+ "handle":0,
+ "phys_id":0,
+ "security:":"disabled"
+}
-H::
--health::
diff --git a/ndctl/lib/dimm.c b/ndctl/lib/dimm.c
index 79e2ca0a..e03135d9 100644
--- a/ndctl/lib/dimm.c
+++ b/ndctl/lib/dimm.c
@@ -587,3 +587,40 @@ NDCTL_EXPORT unsigned long ndctl_dimm_get_available_labels(
return strtoul(buf, NULL, 0);
}
+
+NDCTL_EXPORT int ndctl_dimm_get_security(struct ndctl_dimm *dimm,
+ enum nd_security_state *state)
+{
+ struct ndctl_ctx *ctx = ndctl_dimm_get_ctx(dimm);
+ char *path = dimm->dimm_buf;
+ int len = dimm->buf_len;
+ char buf[64];
+ int rc;
+
+ if (snprintf(path, len, "%s/security", dimm->dimm_path) >= len) {
+ err(ctx, "%s: buffer too small!\n",
+ ndctl_dimm_get_devname(dimm));
+ return -ERANGE;
+ }
+
+ rc = sysfs_read_attr(ctx, path, buf);
+ if (rc < 0)
+ return rc;
+
+ if (strcmp(buf, "unsupported") == 0)
+ *state = ND_SECURITY_UNSUPPORTED;
+ else if (strcmp(buf, "disabled") == 0)
+ *state = ND_SECURITY_DISABLED;
+ else if (strcmp(buf, "unlocked") == 0)
+ *state = ND_SECURITY_UNLOCKED;
+ else if (strcmp(buf, "locked") == 0)
+ *state = ND_SECURITY_LOCKED;
+ else if (strcmp(buf, "frozen") == 0)
+ *state = ND_SECURITY_FROZEN;
+ else if (strcmp(buf, "overwrite") == 0)
+ *state = ND_SECURITY_OVERWRITE;
+ else
+ *state = ND_SECURITY_INVALID;
+
+ return 0;
+}
diff --git a/ndctl/lib/libndctl.sym b/ndctl/lib/libndctl.sym
index 6c4c8b4d..1bd63fa1 100644
--- a/ndctl/lib/libndctl.sym
+++ b/ndctl/lib/libndctl.sym
@@ -385,3 +385,8 @@ global:
ndctl_namespace_get_next_badblock;
ndctl_dimm_get_dirty_shutdown;
} LIBNDCTL_17;
+
+LIBNDCTL_19 {
+global:
+ ndctl_dimm_get_security;
+} LIBNDCTL_18;
diff --git a/ndctl/libndctl.h b/ndctl/libndctl.h
index c81cc032..4255252c 100644
--- a/ndctl/libndctl.h
+++ b/ndctl/libndctl.h
@@ -681,6 +681,19 @@ enum ND_FW_STATUS ndctl_cmd_fw_xlat_firmware_status(struct
ndctl_cmd *cmd);
struct ndctl_cmd *ndctl_dimm_cmd_new_ack_shutdown_count(struct ndctl_dimm
*dimm);
int ndctl_dimm_fw_update_supported(struct ndctl_dimm *dimm);
+enum nd_security_state {
+ ND_SECURITY_INVALID = -1,
+ ND_SECURITY_UNSUPPORTED = 0,
+ ND_SECURITY_DISABLED,
+ ND_SECURITY_UNLOCKED,
+ ND_SECURITY_LOCKED,
+ ND_SECURITY_FROZEN,
+ ND_SECURITY_OVERWRITE,
+};
+
+int ndctl_dimm_get_security(struct ndctl_dimm *dimm,
+ enum nd_security_state *sstate);
+
#ifdef __cplusplus
} /* extern "C" */
#endif
diff --git a/util/json.c b/util/json.c
index 5c3424e2..e3b9e72e 100644
--- a/util/json.c
+++ b/util/json.c
@@ -164,6 +164,7 @@ struct json_object *util_dimm_to_json(struct ndctl_dimm
*dimm,
unsigned int handle = ndctl_dimm_get_handle(dimm);
unsigned short phys_id = ndctl_dimm_get_phys_id(dimm);
struct json_object *jobj;
+ enum nd_security_state sstate;
if (!jdimm)
return NULL;
@@ -243,6 +244,36 @@ struct json_object *util_dimm_to_json(struct ndctl_dimm
*dimm,
json_object_object_add(jdimm, "flag_smart_event", jobj);
}
+ if (ndctl_dimm_get_security(dimm, &sstate) == 0) {
+ switch (sstate) {
+ case ND_SECURITY_UNSUPPORTED:
+ jobj = json_object_new_string("unsupported");
+ break;
+ case ND_SECURITY_DISABLED:
+ jobj = json_object_new_string("disabled");
+ break;
+ case ND_SECURITY_UNLOCKED:
+ jobj = json_object_new_string("unlocked");
+ break;
+ case ND_SECURITY_LOCKED:
+ jobj = json_object_new_string("locked");
+ break;
+ case ND_SECURITY_FROZEN:
+ jobj = json_object_new_string("frozen");
+
Re: [ndctl PATCH v3 4/4] ndctl: clean up usage of ndctl_cmd_submit
On Mon, Jan 14, 2019 at 10:49 AM Vishal Verma wrote: [..] > I see how it can be replaced now. Here is a revised patch 4 that > includes thses conversions: > > 8< > > > From 5c19c269dd0037c3f68725e0f721784056172433 Mon Sep 17 00:00:00 2001 > From: Vishal Verma > Date: Fri, 11 Jan 2019 18:20:31 -0700 > Subject: [ndctl PATCH] ndctl: clean up usage of ndctl_cmd_submit > > It is possible for ndctl_cmd_submit to return a positive number, > indicating a buffer underrun. It is only truly an error if it returns a > negative number. Several places in the library, the ndctl utility, and > in test/ were simply checking for an error with "if (rc)". Fix these to > only error out for negative returns. > > Cc: Dan Williams > Signed-off-by: Vishal Verma Looks good to me. For the series: Reviewed-by: Dan Williams ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [ndctl PATCH v3 4/4] ndctl: clean up usage of ndctl_cmd_submit
On 01/14, Verma, Vishal L wrote:
>
> On Mon, 2019-01-14 at 10:17 -0800, Dan Williams wrote:
> > On Mon, Jan 14, 2019 at 10:11 AM Vishal Verma <
> > vishal.l.ve...@intel.com> wrote:
> > >
> > > It is possible for ndctl_cmd_submit to return a positive number,
> > > indicating a buffer underrun. It is only truly an error if it
> > > returns a
> > > negative number. Several places in the library, the ndctl utility,
> > > and
> > > in test/ were simply checking for an error with "if (rc)". Fix
> > > these to
> > > only error out for negative returns.
> > >
> > > Cc: Dan Williams
> > > Signed-off-by: Vishal Verma
> > > ---
> > > ndctl/lib/dimm.c | 6 +++---
> > > ndctl/lib/inject.c| 8
> > > ndctl/lib/nfit.c | 2 +-
> > > ndctl/util/json-firmware.c| 2 +-
> > > test/ack-shutdown-count-set.c | 8 ++--
> > > test/daxdev-errors.c | 8
> > > test/libndctl.c | 32 ---
> > > -
> > > test/smart-notify.c | 8
> > > 8 files changed, 35 insertions(+), 39 deletions(-)
> > >
> > > diff --git a/ndctl/lib/dimm.c b/ndctl/lib/dimm.c
> > > index 79e2ca0..12dc74a 100644
> > > --- a/ndctl/lib/dimm.c
> > > +++ b/ndctl/lib/dimm.c
> > > @@ -332,7 +332,7 @@ static int nvdimm_set_config_data(struct
> > > nvdimm_data *ndd, size_t offset,
> > > goto out;
> > >
> > > rc = ndctl_cmd_submit(cmd_write);
> > > - if (rc || ndctl_cmd_get_firmware_status(cmd_write))
> > > + if ((rc < 0) || ndctl_cmd_get_firmware_status(cmd_write))
> >
> > With ndctl_cmd_submit_xlat() the ndctl_cmd_get_firmware_status() can
> > be dropped, right?
> >
> > rc = ndctl_cmd_submit_xlat(cmd_write);
> > if (rc < 0)
> > rc = -ENXIO;
> >
> >
> > ...or are you saving that conversion for a follow on patch?
>
> The config get/write commands are not dimm_ops right? I thought we
> could only use _xlat for dimm_ops?
I see how it can be replaced now. Here is a revised patch 4 that
includes thses conversions:
8<
>From 5c19c269dd0037c3f68725e0f721784056172433 Mon Sep 17 00:00:00 2001
From: Vishal Verma
Date: Fri, 11 Jan 2019 18:20:31 -0700
Subject: [ndctl PATCH] ndctl: clean up usage of ndctl_cmd_submit
It is possible for ndctl_cmd_submit to return a positive number,
indicating a buffer underrun. It is only truly an error if it returns a
negative number. Several places in the library, the ndctl utility, and
in test/ were simply checking for an error with "if (rc)". Fix these to
only error out for negative returns.
Cc: Dan Williams
Signed-off-by: Vishal Verma
---
ndctl/lib/dimm.c | 18 +++---
ndctl/lib/inject.c| 8
ndctl/lib/nfit.c | 2 +-
ndctl/util/json-firmware.c| 2 +-
test/ack-shutdown-count-set.c | 8 ++--
test/daxdev-errors.c | 8
test/libndctl.c | 32
test/smart-notify.c | 8
8 files changed, 39 insertions(+), 47 deletions(-)
diff --git a/ndctl/lib/dimm.c b/ndctl/lib/dimm.c
index 79e2ca0..f5a955e 100644
--- a/ndctl/lib/dimm.c
+++ b/ndctl/lib/dimm.c
@@ -331,8 +331,8 @@ static int nvdimm_set_config_data(struct nvdimm_data *ndd,
size_t offset,
if (rc < 0)
goto out;
- rc = ndctl_cmd_submit(cmd_write);
- if (rc || ndctl_cmd_get_firmware_status(cmd_write))
+ rc = ndctl_cmd_submit_xlat(cmd_write);
+ if (rc < 0)
rc = -ENXIO;
out:
ndctl_cmd_unref(cmd_write);
@@ -487,15 +487,15 @@ NDCTL_EXPORT struct ndctl_cmd
*ndctl_dimm_read_labels(struct ndctl_dimm *dimm)
cmd_size = ndctl_dimm_cmd_new_cfg_size(dimm);
if (!cmd_size)
return NULL;
-rc = ndctl_cmd_submit(cmd_size);
-if (rc || ndctl_cmd_get_firmware_status(cmd_size))
+rc = ndctl_cmd_submit_xlat(cmd_size);
+if (rc < 0)
goto out_size;
cmd_read = ndctl_dimm_cmd_new_cfg_read(cmd_size);
if (!cmd_read)
goto out_size;
-rc = ndctl_cmd_submit(cmd_read);
-if (rc || ndctl_cmd_get_firmware_status(cmd_read))
+rc = ndctl_cmd_submit_xlat(cmd_read);
+if (rc < 0)
goto out_read;
ndctl_cmd_unref(cmd_size);
@@ -536,13 +536,9 @@ NDCTL_EXPORT int ndctl_dimm_zero_labels(struct ndctl_dimm
*dimm)
rc = -ENXIO;
goto out_write;
}
- rc = ndctl_cmd_submit(cmd_write);
+ rc = ndctl_cmd_submit_xlat(cmd_write);
if (rc < 0)
goto out_write;
- if (ndctl_cmd_get_firmware_status(cmd_write)) {
- rc = -ENXIO;
- goto out_write;
- }
/*
* If the dimm is already disabled the kernel is not holding a cached
diff --git a/ndctl/lib/inject.c b/ndctl/lib/inject.c
index 2b0702e..c35d0f3 100644
--- a/ndctl/lib/inject.c
+++ b/ndctl
Re: [ndctl PATCH v3 4/4] ndctl: clean up usage of ndctl_cmd_submit
On Mon, 2019-01-14 at 10:17 -0800, Dan Williams wrote: > On Mon, Jan 14, 2019 at 10:11 AM Vishal Verma < > vishal.l.ve...@intel.com> wrote: > > > > It is possible for ndctl_cmd_submit to return a positive number, > > indicating a buffer underrun. It is only truly an error if it > > returns a > > negative number. Several places in the library, the ndctl utility, > > and > > in test/ were simply checking for an error with "if (rc)". Fix > > these to > > only error out for negative returns. > > > > Cc: Dan Williams > > Signed-off-by: Vishal Verma > > --- > > ndctl/lib/dimm.c | 6 +++--- > > ndctl/lib/inject.c| 8 > > ndctl/lib/nfit.c | 2 +- > > ndctl/util/json-firmware.c| 2 +- > > test/ack-shutdown-count-set.c | 8 ++-- > > test/daxdev-errors.c | 8 > > test/libndctl.c | 32 --- > > - > > test/smart-notify.c | 8 > > 8 files changed, 35 insertions(+), 39 deletions(-) > > > > diff --git a/ndctl/lib/dimm.c b/ndctl/lib/dimm.c > > index 79e2ca0..12dc74a 100644 > > --- a/ndctl/lib/dimm.c > > +++ b/ndctl/lib/dimm.c > > @@ -332,7 +332,7 @@ static int nvdimm_set_config_data(struct > > nvdimm_data *ndd, size_t offset, > > goto out; > > > > rc = ndctl_cmd_submit(cmd_write); > > - if (rc || ndctl_cmd_get_firmware_status(cmd_write)) > > + if ((rc < 0) || ndctl_cmd_get_firmware_status(cmd_write)) > > With ndctl_cmd_submit_xlat() the ndctl_cmd_get_firmware_status() can > be dropped, right? > > rc = ndctl_cmd_submit_xlat(cmd_write); > if (rc < 0) > rc = -ENXIO; > > > ...or are you saving that conversion for a follow on patch? The config get/write commands are not dimm_ops right? I thought we could only use _xlat for dimm_ops? ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [ndctl PATCH v3 4/4] ndctl: clean up usage of ndctl_cmd_submit
On Mon, Jan 14, 2019 at 10:11 AM Vishal Verma wrote: > > It is possible for ndctl_cmd_submit to return a positive number, > indicating a buffer underrun. It is only truly an error if it returns a > negative number. Several places in the library, the ndctl utility, and > in test/ were simply checking for an error with "if (rc)". Fix these to > only error out for negative returns. > > Cc: Dan Williams > Signed-off-by: Vishal Verma > --- > ndctl/lib/dimm.c | 6 +++--- > ndctl/lib/inject.c| 8 > ndctl/lib/nfit.c | 2 +- > ndctl/util/json-firmware.c| 2 +- > test/ack-shutdown-count-set.c | 8 ++-- > test/daxdev-errors.c | 8 > test/libndctl.c | 32 > test/smart-notify.c | 8 > 8 files changed, 35 insertions(+), 39 deletions(-) > > diff --git a/ndctl/lib/dimm.c b/ndctl/lib/dimm.c > index 79e2ca0..12dc74a 100644 > --- a/ndctl/lib/dimm.c > +++ b/ndctl/lib/dimm.c > @@ -332,7 +332,7 @@ static int nvdimm_set_config_data(struct nvdimm_data > *ndd, size_t offset, > goto out; > > rc = ndctl_cmd_submit(cmd_write); > - if (rc || ndctl_cmd_get_firmware_status(cmd_write)) > + if ((rc < 0) || ndctl_cmd_get_firmware_status(cmd_write)) With ndctl_cmd_submit_xlat() the ndctl_cmd_get_firmware_status() can be dropped, right? rc = ndctl_cmd_submit_xlat(cmd_write); if (rc < 0) rc = -ENXIO; ...or are you saving that conversion for a follow on patch? ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
[ndctl PATCH v3 0/4] Add missing firmware_status checks
Changes in v3: - Patch 2: Also fix open coded get_firmware_status (Dan) - Patch 4: Change rc to an int as it is only used for the return status of cmd_submit. - Patch 4: Fix another open coded get_firmware_status in test/ack-shutdown-count-set.c Changes in v2: - For the new helper, return original positive rc when we can (Dan) - Convert previously missed locations to the new submission helper (patch 2) - Add a new patch (4/4) to fix up all callers of ndctl_cmd_submit to the correct return convention. There were several places in ndctl where we neglected to check the firmware_status from a command submission. Add an ndctl_dimm_op target to perform error translation for a DSM family, and provide a new helper - ndctl_cmd_submit_xlat - that will submit the command and call an error translation routine if one has been registered. Switch the call sites in ndctl/inject-smart.c and ndctl/monitor.c over to the new command submission helper. Finally, clean up all remaining callers of ndctl_cmd_submit to use the correct return convention, i.e. only treat negative returns as an error, and accept positive return codes as OK. Vishal Verma (4): libndctl, intel: Add infrastructure for firmware_status translation ndctl, inject-smart: switch to ndctl_cmd_submit_xlat ndctl, monitor: switch to ndctl_cmd_submit_xlat ndctl: clean up usage of ndctl_cmd_submit ndctl/inject-smart.c | 16 ++-- ndctl/lib/dimm.c | 6 ++--- ndctl/lib/inject.c| 8 +++--- ndctl/lib/intel.c | 49 +++ ndctl/lib/intel.h | 1 + ndctl/lib/libndctl.c | 28 ndctl/lib/libndctl.sym| 6 + ndctl/lib/nfit.c | 2 +- ndctl/lib/private.h | 1 + ndctl/libndctl.h | 3 +++ ndctl/monitor.c | 6 ++--- ndctl/util/json-firmware.c| 2 +- ndctl/util/json-smart.c | 8 +++--- test/ack-shutdown-count-set.c | 8 ++ test/daxdev-errors.c | 8 +++--- test/libndctl.c | 32 +++ test/smart-notify.c | 8 +++--- 17 files changed, 138 insertions(+), 54 deletions(-) -- 2.17.2 ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
[ndctl PATCH v3 3/4] ndctl, monitor: switch to ndctl_cmd_submit_xlat
The ndctl monitor command was neglecting to check the 'firmware_status'
field that is set by the platform firmware to indicate failure. Use
the new ndctl_cmd_submit_xlat facility to include the firmware_status
check as part of the command submission.
Cc: QI Fuli
Cc: Dan Williams
Signed-off-by: Vishal Verma
---
ndctl/monitor.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/ndctl/monitor.c b/ndctl/monitor.c
index 233f2bb..4ad1429 100644
--- a/ndctl/monitor.c
+++ b/ndctl/monitor.c
@@ -226,7 +226,7 @@ static int enable_dimm_supported_threshold_alarms(struct
ndctl_dimm *dimm)
err(&monitor, "%s: no smart threshold command support\n", name);
goto out;
}
- if (ndctl_cmd_submit(st_cmd)) {
+ if (ndctl_cmd_submit_xlat(st_cmd) < 0) {
err(&monitor, "%s: smart threshold command failed\n", name);
goto out;
}
@@ -246,8 +246,8 @@ static int enable_dimm_supported_threshold_alarms(struct
ndctl_dimm *dimm)
alarm |= ND_SMART_CTEMP_TRIP;
ndctl_cmd_smart_threshold_set_alarm_control(sst_cmd, alarm);
- rc = ndctl_cmd_submit(sst_cmd);
- if (rc) {
+ rc = ndctl_cmd_submit_xlat(sst_cmd);
+ if (rc < 0) {
err(&monitor, "%s: smart set threshold command failed\n", name);
goto out;
}
--
2.17.2
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
[ndctl PATCH v3 1/4] libndctl, intel: Add infrastructure for firmware_status translation
Add a new routine to ndctl_dimm_ops that allows a DSM family to provide
a translation routine that will translate the status codes of the result
of a DSM to generic errno style error codes. To use this routine
effectively, add a new wrapper around ndctl_cmd_submit (called
ndctl_cmd_submit_xlat) that submits the command, and also runs it
through the above translator dimm_op (if one is is defined).
Cc: Dan Williams
Signed-off-by: Vishal Verma
---
ndctl/lib/intel.c | 49 ++
ndctl/lib/intel.h | 1 +
ndctl/lib/libndctl.c | 28
ndctl/lib/libndctl.sym | 6 ++
ndctl/lib/private.h| 1 +
ndctl/libndctl.h | 3 +++
6 files changed, 88 insertions(+)
diff --git a/ndctl/lib/intel.c b/ndctl/lib/intel.c
index 744386f..d684bac 100644
--- a/ndctl/lib/intel.c
+++ b/ndctl/lib/intel.c
@@ -16,6 +16,54 @@
#include
#include "private.h"
+static int intel_cmd_xlat_firmware_status(struct ndctl_cmd *cmd)
+{
+ struct nd_pkg_intel *pkg = cmd->intel;
+ unsigned int status, ext_status;
+
+ status = (*cmd->firmware_status) & ND_INTEL_STATUS_MASK;
+ ext_status = (*cmd->firmware_status) & ND_INTEL_STATUS_EXTEND_MASK;
+
+ /* Common statuses */
+ switch (status) {
+ case ND_INTEL_STATUS_SUCCESS:
+ return 0;
+ case ND_INTEL_STATUS_NOTSUPP:
+ return -EOPNOTSUPP;
+ case ND_INTEL_STATUS_NOTEXIST:
+ return -ENXIO;
+ case ND_INTEL_STATUS_INVALPARM:
+ return -EINVAL;
+ case ND_INTEL_STATUS_HWERR:
+ return -EIO;
+ case ND_INTEL_STATUS_RETRY:
+ return -EAGAIN;
+ case ND_INTEL_STATUS_EXTEND:
+ /* refer to extended status, break out of this */
+ break;
+ case ND_INTEL_STATUS_NORES:
+ return -EAGAIN;
+ case ND_INTEL_STATUS_NOTREADY:
+ return -EBUSY;
+ }
+
+ /* Extended status is command specific */
+ switch (pkg->gen.nd_command) {
+ case ND_INTEL_SMART:
+ case ND_INTEL_SMART_THRESHOLD:
+ case ND_INTEL_SMART_SET_THRESHOLD:
+ /* ext status not specified */
+ break;
+ case ND_INTEL_SMART_INJECT:
+ /* smart injection not enabled */
+ if (ext_status == ND_INTEL_STATUS_INJ_DISABLED)
+ return -ENXIO;
+ break;
+ }
+
+ return -ENOMSG;
+}
+
static struct ndctl_cmd *alloc_intel_cmd(struct ndctl_dimm *dimm,
unsigned func, size_t in_size, size_t out_size)
{
@@ -780,4 +828,5 @@ struct ndctl_dimm_ops * const intel_dimm_ops = &(struct
ndctl_dimm_ops) {
.fw_xlat_firmware_status = intel_cmd_fw_xlat_firmware_status,
.new_ack_shutdown_count = intel_dimm_cmd_new_lss,
.fw_update_supported = intel_dimm_fw_update_supported,
+ .xlat_firmware_status = intel_cmd_xlat_firmware_status,
};
diff --git a/ndctl/lib/intel.h b/ndctl/lib/intel.h
index 3b01bba..530c996 100644
--- a/ndctl/lib/intel.h
+++ b/ndctl/lib/intel.h
@@ -179,5 +179,6 @@ struct nd_pkg_intel {
#define ND_INTEL_STATUS_FQ_BUSY0x2
#define ND_INTEL_STATUS_FQ_BAD 0x3
#define ND_INTEL_STATUS_FQ_ORDER 0x4
+#define ND_INTEL_STATUS_INJ_DISABLED 0x1
#endif /* __INTEL_H__ */
diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
index e82a08d..830b791 100644
--- a/ndctl/lib/libndctl.c
+++ b/ndctl/lib/libndctl.c
@@ -2704,6 +2704,16 @@ static const char *ndctl_dimm_get_cmd_subname(struct
ndctl_cmd *cmd)
return ops->cmd_desc(cmd->pkg->nd_command);
}
+NDCTL_EXPORT int ndctl_cmd_xlat_firmware_status(struct ndctl_cmd *cmd)
+{
+ struct ndctl_dimm *dimm = cmd->dimm;
+ struct ndctl_dimm_ops *ops = dimm ? dimm->ops : NULL;
+
+ if (!dimm || !ops || !ops->xlat_firmware_status)
+ return -ENOMSG;
+ return ops->xlat_firmware_status(cmd);
+}
+
static int do_cmd(int fd, int ioctl_cmd, struct ndctl_cmd *cmd)
{
int rc;
@@ -2819,6 +2829,24 @@ NDCTL_EXPORT int ndctl_cmd_submit(struct ndctl_cmd *cmd)
return rc;
}
+NDCTL_EXPORT int ndctl_cmd_submit_xlat(struct ndctl_cmd *cmd)
+{
+ int rc, xlat_rc;
+
+ rc = ndctl_cmd_submit(cmd);
+ if (rc < 0)
+ return rc;
+
+ /*
+* NOTE: This can lose a positive rc when xlat_rc is non-zero. The
+* positive rc indicates a buffer underrun from the original command
+* submission. If the caller cares about that (generally not very
+* useful), then the xlat function is available separately as well.
+*/
+ xlat_rc = ndctl_cmd_xlat_firmware_status(cmd);
+ return (xlat_rc == 0) ? rc : xlat_rc;
+}
+
NDCTL_EXPORT int ndctl_cmd_get_status(struct ndctl_cmd *cmd)
{
return cmd->status;
diff --git a/ndctl/lib/libndctl.sym b/ndctl/lib/libndctl.sym
index 6c4c8b4..275db92 100644
--- a/ndctl/lib/l
[ndctl PATCH v3 2/4] ndctl, inject-smart: switch to ndctl_cmd_submit_xlat
The ndctl inject-smart command was neglecting to check the
'firmware_status' field that is set by the platform firmware to indicate
failure. Use the new ndctl_cmd_submit_xlat facility to include the
firmware_status check as part of the command submission.
Reported-by: Ami Pathak
Cc: Dan Williams
Signed-off-by: Vishal Verma
---
ndctl/inject-smart.c| 16
ndctl/util/json-smart.c | 8
2 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/ndctl/inject-smart.c b/ndctl/inject-smart.c
index eaa137a..00c81b8 100644
--- a/ndctl/inject-smart.c
+++ b/ndctl/inject-smart.c
@@ -280,8 +280,8 @@ static int smart_set_thresh(struct ndctl_dimm *dimm)
goto out;
}
- rc = ndctl_cmd_submit(st_cmd);
- if (rc) {
+ rc = ndctl_cmd_submit_xlat(st_cmd);
+ if (rc < 0) {
error("%s: smart threshold command failed: %s (%d)\n",
name, strerror(abs(rc)), rc);
goto out;
@@ -320,8 +320,8 @@ static int smart_set_thresh(struct ndctl_dimm *dimm)
ndctl_cmd_smart_threshold_set_alarm_control(sst_cmd, alarm);
}
- rc = ndctl_cmd_submit(sst_cmd);
- if (rc)
+ rc = ndctl_cmd_submit_xlat(sst_cmd);
+ if (rc < 0)
error("%s: smart set threshold command failed: %s (%d)\n",
name, strerror(abs(rc)), rc);
@@ -351,8 +351,8 @@ out:
if (sctx.err_continue == false) \
goto out; \
} \
- rc = ndctl_cmd_submit(si_cmd); \
- if (rc) { \
+ rc = ndctl_cmd_submit_xlat(si_cmd); \
+ if (rc < 0) { \
error("%s: smart inject %s command failed: %s (%d)\n", \
name, #arg, strerror(abs(rc)), rc); \
if (sctx.err_continue == false) \
@@ -382,8 +382,8 @@ out:
if (sctx.err_continue == false) \
goto out; \
} \
- rc = ndctl_cmd_submit(si_cmd); \
- if (rc) { \
+ rc = ndctl_cmd_submit_xlat(si_cmd); \
+ if (rc < 0) { \
error("%s: smart inject %s command failed: %s (%d)\n", \
name, #arg, strerror(abs(rc)), rc); \
if (sctx.err_continue == false) \
diff --git a/ndctl/util/json-smart.c b/ndctl/util/json-smart.c
index 3c1b917..a9bd17b 100644
--- a/ndctl/util/json-smart.c
+++ b/ndctl/util/json-smart.c
@@ -30,8 +30,8 @@ static void smart_threshold_to_json(struct ndctl_dimm *dimm,
if (!cmd)
return;
- rc = ndctl_cmd_submit(cmd);
- if (rc || ndctl_cmd_get_firmware_status(cmd))
+ rc = ndctl_cmd_submit_xlat(cmd);
+ if (rc < 0)
goto out;
alarm_control = ndctl_cmd_smart_threshold_get_alarm_control(cmd);
@@ -115,8 +115,8 @@ struct json_object *util_dimm_health_to_json(struct
ndctl_dimm *dimm)
if (!cmd)
goto err;
- rc = ndctl_cmd_submit(cmd);
- if (rc || ndctl_cmd_get_firmware_status(cmd)) {
+ rc = ndctl_cmd_submit_xlat(cmd);
+ if (rc < 0) {
jobj = json_object_new_string("unknown");
if (jobj)
json_object_object_add(jhealth, "health_state", jobj);
--
2.17.2
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
[ndctl PATCH v3 4/4] ndctl: clean up usage of ndctl_cmd_submit
It is possible for ndctl_cmd_submit to return a positive number,
indicating a buffer underrun. It is only truly an error if it returns a
negative number. Several places in the library, the ndctl utility, and
in test/ were simply checking for an error with "if (rc)". Fix these to
only error out for negative returns.
Cc: Dan Williams
Signed-off-by: Vishal Verma
---
ndctl/lib/dimm.c | 6 +++---
ndctl/lib/inject.c| 8
ndctl/lib/nfit.c | 2 +-
ndctl/util/json-firmware.c| 2 +-
test/ack-shutdown-count-set.c | 8 ++--
test/daxdev-errors.c | 8
test/libndctl.c | 32
test/smart-notify.c | 8
8 files changed, 35 insertions(+), 39 deletions(-)
diff --git a/ndctl/lib/dimm.c b/ndctl/lib/dimm.c
index 79e2ca0..12dc74a 100644
--- a/ndctl/lib/dimm.c
+++ b/ndctl/lib/dimm.c
@@ -332,7 +332,7 @@ static int nvdimm_set_config_data(struct nvdimm_data *ndd,
size_t offset,
goto out;
rc = ndctl_cmd_submit(cmd_write);
- if (rc || ndctl_cmd_get_firmware_status(cmd_write))
+ if ((rc < 0) || ndctl_cmd_get_firmware_status(cmd_write))
rc = -ENXIO;
out:
ndctl_cmd_unref(cmd_write);
@@ -488,14 +488,14 @@ NDCTL_EXPORT struct ndctl_cmd
*ndctl_dimm_read_labels(struct ndctl_dimm *dimm)
if (!cmd_size)
return NULL;
rc = ndctl_cmd_submit(cmd_size);
-if (rc || ndctl_cmd_get_firmware_status(cmd_size))
+if ((rc < 0) || ndctl_cmd_get_firmware_status(cmd_size))
goto out_size;
cmd_read = ndctl_dimm_cmd_new_cfg_read(cmd_size);
if (!cmd_read)
goto out_size;
rc = ndctl_cmd_submit(cmd_read);
-if (rc || ndctl_cmd_get_firmware_status(cmd_read))
+if ((rc < 0) || ndctl_cmd_get_firmware_status(cmd_read))
goto out_read;
ndctl_cmd_unref(cmd_size);
diff --git a/ndctl/lib/inject.c b/ndctl/lib/inject.c
index 2b0702e..c35d0f3 100644
--- a/ndctl/lib/inject.c
+++ b/ndctl/lib/inject.c
@@ -156,7 +156,7 @@ static int ndctl_namespace_inject_one_error(struct
ndctl_namespace *ndns,
(1 << ND_ARS_ERR_INJ_OPT_NOTIFY);
rc = ndctl_cmd_submit(cmd);
- if (rc) {
+ if (rc < 0) {
dbg(ctx, "Error submitting command: %d\n", rc);
goto out;
}
@@ -234,7 +234,7 @@ static int ndctl_namespace_uninject_one_error(struct
ndctl_namespace *ndns,
err_inj_clr->err_inj_clr_spa_range_length = length;
rc = ndctl_cmd_submit(cmd);
- if (rc) {
+ if (rc < 0) {
dbg(ctx, "Error submitting command: %d\n", rc);
goto out;
}
@@ -443,7 +443,7 @@ NDCTL_EXPORT int ndctl_namespace_injection_status(struct
ndctl_namespace *ndns)
cmd = ndctl_bus_cmd_new_ars_cap(bus, ns_offset, ns_size);
rc = ndctl_cmd_submit(cmd);
- if (rc) {
+ if (rc < 0) {
dbg(ctx, "Error submitting ars_cap: %d\n", rc);
goto out;
}
@@ -464,7 +464,7 @@ NDCTL_EXPORT int ndctl_namespace_injection_status(struct
ndctl_namespace *ndns)
(struct nd_cmd_ars_err_inj_stat *)&pkg->nd_payload[0];
rc = ndctl_cmd_submit(cmd);
- if (rc) {
+ if (rc < 0) {
dbg(ctx, "Error submitting command: %d\n", rc);
goto out;
}
diff --git a/ndctl/lib/nfit.c b/ndctl/lib/nfit.c
index 2ae3f07..b10edb1 100644
--- a/ndctl/lib/nfit.c
+++ b/ndctl/lib/nfit.c
@@ -133,7 +133,7 @@ int ndctl_bus_nfit_translate_spa(struct ndctl_bus *bus,
translate_spa->spa = address;
rc = ndctl_cmd_submit(cmd);
- if (rc) {
+ if (rc < 0) {
ndctl_cmd_unref(cmd);
return rc;
}
diff --git a/ndctl/util/json-firmware.c b/ndctl/util/json-firmware.c
index 118424f..f7150d8 100644
--- a/ndctl/util/json-firmware.c
+++ b/ndctl/util/json-firmware.c
@@ -25,7 +25,7 @@ struct json_object *util_dimm_firmware_to_json(struct
ndctl_dimm *dimm,
goto err;
rc = ndctl_cmd_submit(cmd);
- if (rc || ndctl_cmd_fw_xlat_firmware_status(cmd) != FW_SUCCESS) {
+ if ((rc < 0) || ndctl_cmd_fw_xlat_firmware_status(cmd) != FW_SUCCESS) {
jobj = util_json_object_hex(-1, flags);
if (jobj)
json_object_object_add(jfirmware, "current_version",
diff --git a/test/ack-shutdown-count-set.c b/test/ack-shutdown-count-set.c
index 9baa2f1..742e976 100644
--- a/test/ack-shutdown-count-set.c
+++ b/test/ack-shutdown-count-set.c
@@ -27,12 +27,8 @@ static int test_dimm(struct ndctl_dimm *dimm)
if (!cmd)
return -ENOMEM;
- rc = ndctl_cmd_submit(cmd);
- if (rc < 0)
- goto out;
-
Re: [PATCH] acpi/nfit: Fix command-supported detection
On Mon, Jan 14, 2019 at 8:43 AM Dan Williams wrote:
> On Mon, Jan 14, 2019 at 7:19 AM Jeff Moyer wrote:
[..]
> > > +
> > > + if (cmd == ND_CMD_CALL) {
> > > + int i;
> > > +
> > > + if (call_pkg && nfit_mem->family != call_pkg->nd_family)
> > > + return -ENOTTY;
> > > +
> > > + for (i = 0; i < ARRAY_SIZE(call_pkg->nd_reserved2); i++)
> > > + if (call_pkg->nd_reserved2[i])
> > > + return -EINVAL;
> > > + return call_pkg->nd_command;
> > > + }
> > > +
> > > + /* Linux ND commands == NVDIMM_FAMILY_INTEL function numbers */
> > > + if (nfit_mem->family == NVDIMM_FAMILY_INTEL)
> > > + return cmd;
> > > + return 0;
> >
> > Function zero? Is that really the right thing to return here?
>
> Yes, function zero is never set in n
...whoops fumble fingered "send"
Function zero should never be set in nfit_mem->dsm_mask, although the
NVDIMM_FAMILY_MSFT mask violates this assumption. I'll fix that up.
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH] acpi/nfit: Fix command-supported detection
On Mon, Jan 14, 2019 at 7:19 AM Jeff Moyer wrote:
>
> Dan Williams writes:
>
> > The _DSM function number validation only happens to succeed when the
> > generic Linux command number translation corresponds with a
> > DSM-family-specific function number. This breaks NVDIMM-N
> > implementations that correctly implement _LSR, _LSW, and _LSI, but do
> > not happen to publish support for DSM function numbers 4, 5, and 6.
> >
> > Recall that the support for _LS{I,R,W} family of methods results in the
> > DIMM being marked as supporting those command numbers at
> > acpi_nfit_register_dimms() time. The DSM function mask is only used for
> > ND_CMD_CALL support of non-NVDIMM_FAMILY_INTEL devices.
> >
> > Fixes: 31eca76ba2fc ("nfit, libnvdimm: limited/whitelisted dimm command...")
> > Cc:
> > Link: https://github.com/pmem/ndctl/issues/78
> > Reported-by: Sujith Pandel
> > Signed-off-by: Dan Williams
> > ---
> > Sujith, this is a larger change than what you originally tested, but it
> > should behave the same. I wanted to consolidate all the code that
> > handles Linux command number to DIMM _DSM function number translation.
> >
> > If you have a chance to re-test with this it would be much appreciated.
> >
> > Thanks for the report!
> >
> > drivers/acpi/nfit/core.c | 43 +--
> > 1 file changed, 29 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> > index 790691d9a982..d5d64e90ae71 100644
> > --- a/drivers/acpi/nfit/core.c
> > +++ b/drivers/acpi/nfit/core.c
> > @@ -409,6 +409,29 @@ static bool payload_dumpable(struct nvdimm *nvdimm,
> > unsigned int func)
> > return true;
> > }
> >
> > +static int cmd_to_func(struct nvdimm *nvdimm, unsigned int cmd,
> > + struct nd_cmd_pkg *call_pkg)
> > +{
> > + struct nfit_mem *nfit_mem = nvdimm_provider_data(nvdimm);
>
> Minor nit: Seems like the function could take an nfit_mem parameter instead
> of an nvdimm.
I was making it symmetric with payload_dumpable()... but not for any
good reason, will change.
>
> > +
> > + if (cmd == ND_CMD_CALL) {
> > + int i;
> > +
> > + if (call_pkg && nfit_mem->family != call_pkg->nd_family)
> > + return -ENOTTY;
> > +
> > + for (i = 0; i < ARRAY_SIZE(call_pkg->nd_reserved2); i++)
> > + if (call_pkg->nd_reserved2[i])
> > + return -EINVAL;
> > + return call_pkg->nd_command;
> > + }
> > +
> > + /* Linux ND commands == NVDIMM_FAMILY_INTEL function numbers */
> > + if (nfit_mem->family == NVDIMM_FAMILY_INTEL)
> > + return cmd;
> > + return 0;
>
> Function zero? Is that really the right thing to return here?
Yes, function zero is never set in n
>
> Cheers,
> Jeff
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH v3 2/5] virtio-pmem: Add virtio pmem driver
On Wed, Jan 09, 2019 at 08:17:33PM +0530, Pankaj Gupta wrote:
> This patch adds virtio-pmem driver for KVM guest.
>
> Guest reads the persistent memory range information from
> Qemu over VIRTIO and registers it on nvdimm_bus. It also
> creates a nd_region object with the persistent memory
> range information so that existing 'nvdimm/pmem' driver
> can reserve this into system memory map. This way
> 'virtio-pmem' driver uses existing functionality of pmem
> driver to register persistent memory compatible for DAX
> capable filesystems.
>
> This also provides function to perform guest flush over
> VIRTIO from 'pmem' driver when userspace performs flush
> on DAX memory range.
>
> Signed-off-by: Pankaj Gupta
> ---
> drivers/nvdimm/virtio_pmem.c | 84 ++
> drivers/virtio/Kconfig | 10
> drivers/virtio/Makefile | 1 +
> drivers/virtio/pmem.c| 124
> +++
> include/linux/virtio_pmem.h | 60 +++
> include/uapi/linux/virtio_ids.h | 1 +
> include/uapi/linux/virtio_pmem.h | 10
As with any uapi change, you need to CC the virtio dev
mailing list (subscribers only, sorry about that).
> 7 files changed, 290 insertions(+)
> create mode 100644 drivers/nvdimm/virtio_pmem.c
> create mode 100644 drivers/virtio/pmem.c
> create mode 100644 include/linux/virtio_pmem.h
> create mode 100644 include/uapi/linux/virtio_pmem.h
>
> diff --git a/drivers/nvdimm/virtio_pmem.c b/drivers/nvdimm/virtio_pmem.c
> new file mode 100644
> index 000..2a1b1ba
> --- /dev/null
> +++ b/drivers/nvdimm/virtio_pmem.c
> @@ -0,0 +1,84 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * virtio_pmem.c: Virtio pmem Driver
> + *
> + * Discovers persistent memory range information
> + * from host and provides a virtio based flushing
> + * interface.
> + */
> +#include
> +#include "nd.h"
> +
> + /* The interrupt handler */
> +void host_ack(struct virtqueue *vq)
> +{
> + unsigned int len;
> + unsigned long flags;
> + struct virtio_pmem_request *req, *req_buf;
> + struct virtio_pmem *vpmem = vq->vdev->priv;
> +
> + spin_lock_irqsave(&vpmem->pmem_lock, flags);
> + while ((req = virtqueue_get_buf(vq, &len)) != NULL) {
> + req->done = true;
> + wake_up(&req->host_acked);
> +
> + if (!list_empty(&vpmem->req_list)) {
> + req_buf = list_first_entry(&vpmem->req_list,
> + struct virtio_pmem_request, list);
> + list_del(&vpmem->req_list);
> + req_buf->wq_buf_avail = true;
> + wake_up(&req_buf->wq_buf);
> + }
> + }
> + spin_unlock_irqrestore(&vpmem->pmem_lock, flags);
> +}
> +EXPORT_SYMBOL_GPL(host_ack);
> +
> + /* The request submission function */
> +int virtio_pmem_flush(struct nd_region *nd_region)
> +{
> + int err;
> + unsigned long flags;
> + struct scatterlist *sgs[2], sg, ret;
> + struct virtio_device *vdev = nd_region->provider_data;
> + struct virtio_pmem *vpmem = vdev->priv;
> + struct virtio_pmem_request *req;
> +
> + might_sleep();
> + req = kmalloc(sizeof(*req), GFP_KERNEL);
> + if (!req)
> + return -ENOMEM;
> +
> + req->done = req->wq_buf_avail = false;
> + strcpy(req->name, "FLUSH");
> + init_waitqueue_head(&req->host_acked);
> + init_waitqueue_head(&req->wq_buf);
> + sg_init_one(&sg, req->name, strlen(req->name));
> + sgs[0] = &sg;
> + sg_init_one(&ret, &req->ret, sizeof(req->ret));
> + sgs[1] = &ret;
> +
> + spin_lock_irqsave(&vpmem->pmem_lock, flags);
> + err = virtqueue_add_sgs(vpmem->req_vq, sgs, 1, 1, req, GFP_ATOMIC);
> + if (err) {
> + dev_err(&vdev->dev, "failed to send command to virtio pmem
> device\n");
> +
> + list_add_tail(&vpmem->req_list, &req->list);
> + spin_unlock_irqrestore(&vpmem->pmem_lock, flags);
> +
> + /* When host has read buffer, this completes via host_ack */
> + wait_event(req->wq_buf, req->wq_buf_avail);
> + spin_lock_irqsave(&vpmem->pmem_lock, flags);
> + }
> + virtqueue_kick(vpmem->req_vq);
> + spin_unlock_irqrestore(&vpmem->pmem_lock, flags);
> +
> + /* When host has read buffer, this completes via host_ack */
> + wait_event(req->host_acked, req->done);
> + err = req->ret;
> + kfree(req);
> +
> + return err;
> +};
> +EXPORT_SYMBOL_GPL(virtio_pmem_flush);
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
> index 3589764..9f634a2 100644
> --- a/drivers/virtio/Kconfig
> +++ b/drivers/virtio/Kconfig
> @@ -42,6 +42,16 @@ config VIRTIO_PCI_LEGACY
>
> If unsure, say Y.
>
> +config VIRTIO_PMEM
> + tristate "Support for virtio pmem driver"
> + depends on VIRTIO
> + depends on LIBNVDIMM
> + help
> + This driver provides suppor
Re: [PATCH] acpi/nfit: Fix command-supported detection
Dan Williams writes:
> The _DSM function number validation only happens to succeed when the
> generic Linux command number translation corresponds with a
> DSM-family-specific function number. This breaks NVDIMM-N
> implementations that correctly implement _LSR, _LSW, and _LSI, but do
> not happen to publish support for DSM function numbers 4, 5, and 6.
>
> Recall that the support for _LS{I,R,W} family of methods results in the
> DIMM being marked as supporting those command numbers at
> acpi_nfit_register_dimms() time. The DSM function mask is only used for
> ND_CMD_CALL support of non-NVDIMM_FAMILY_INTEL devices.
>
> Fixes: 31eca76ba2fc ("nfit, libnvdimm: limited/whitelisted dimm command...")
> Cc:
> Link: https://github.com/pmem/ndctl/issues/78
> Reported-by: Sujith Pandel
> Signed-off-by: Dan Williams
> ---
> Sujith, this is a larger change than what you originally tested, but it
> should behave the same. I wanted to consolidate all the code that
> handles Linux command number to DIMM _DSM function number translation.
>
> If you have a chance to re-test with this it would be much appreciated.
>
> Thanks for the report!
>
> drivers/acpi/nfit/core.c | 43 +--
> 1 file changed, 29 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> index 790691d9a982..d5d64e90ae71 100644
> --- a/drivers/acpi/nfit/core.c
> +++ b/drivers/acpi/nfit/core.c
> @@ -409,6 +409,29 @@ static bool payload_dumpable(struct nvdimm *nvdimm,
> unsigned int func)
> return true;
> }
>
> +static int cmd_to_func(struct nvdimm *nvdimm, unsigned int cmd,
> + struct nd_cmd_pkg *call_pkg)
> +{
> + struct nfit_mem *nfit_mem = nvdimm_provider_data(nvdimm);
Minor nit: Seems like the function could take an nfit_mem parameter instead of
an nvdimm.
> +
> + if (cmd == ND_CMD_CALL) {
> + int i;
> +
> + if (call_pkg && nfit_mem->family != call_pkg->nd_family)
> + return -ENOTTY;
> +
> + for (i = 0; i < ARRAY_SIZE(call_pkg->nd_reserved2); i++)
> + if (call_pkg->nd_reserved2[i])
> + return -EINVAL;
> + return call_pkg->nd_command;
> + }
> +
> + /* Linux ND commands == NVDIMM_FAMILY_INTEL function numbers */
> + if (nfit_mem->family == NVDIMM_FAMILY_INTEL)
> + return cmd;
> + return 0;
Function zero? Is that really the right thing to return here?
Cheers,
Jeff
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [Qemu-devel] [PATCH v3 0/5] kvm "virtio pmem" device
> > > > > Right. Thinking about this I would be more concerned about the fact > > > > > that > > > > > guest can effectively pin amount of host's page cache upto size of > > > > > the > > > > > device/file passed to guest as PMEM, can't it Pankaj? Or is there > > > > > some > > > > > QEMU > > > > > magic that avoids this? > > > > > > > > Yes, guest will pin these host page cache pages using 'get_user_pages' > > > > by > > > > elevating the page reference count. But these pages can be reclaimed by > > > > host > > > > at any time when there is memory pressure. > > > > > > Wait, how can the guest pin the host pages? I would expect this to > > > happen only when using vfio and device assignment. Otherwise, no the > > > host can't reclaim a pinned page, that's the whole point of a pin to > > > prevent the mm from reclaiming ownership. > > > > yes. You are right I just used the pin word but it does not actually pin > > pages > > permanently. I had gone through the discussion on existing problems with > > get_user_pages and DMA e.g [1] to understand Jan's POV. It does mention GUP > > pin pages so I also used the word 'pin'. But guest does not permanently pin > > these pages and these pages can be reclaimed by host. > > OK, then I was just confused how virtio-pmem is going to work. Thanks for > explanation! So can I imagine this as guest mmaping the host file and > providing the mapped range as "NVDIMM pages" to the kernel inside the > guest? Or is it more complex? yes, that's correct. Host's Qemu process virtual address range is used as guest physical address and a direct mapping(EPT/NPT) is established. At guest side, this physical memory range is plugged into guest system memory map and DAX mapping is setup using nvdimm calls. Thanks, Pankaj ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [Qemu-devel] [PATCH v3 0/5] kvm "virtio pmem" device
On Sat 12-01-19 21:17:46, Pankaj Gupta wrote: > > > > Right. Thinking about this I would be more concerned about the fact that > > > > guest can effectively pin amount of host's page cache upto size of the > > > > device/file passed to guest as PMEM, can't it Pankaj? Or is there some > > > > QEMU > > > > magic that avoids this? > > > > > > Yes, guest will pin these host page cache pages using 'get_user_pages' by > > > elevating the page reference count. But these pages can be reclaimed by > > > host > > > at any time when there is memory pressure. > > > > Wait, how can the guest pin the host pages? I would expect this to > > happen only when using vfio and device assignment. Otherwise, no the > > host can't reclaim a pinned page, that's the whole point of a pin to > > prevent the mm from reclaiming ownership. > > yes. You are right I just used the pin word but it does not actually pin > pages > permanently. I had gone through the discussion on existing problems with > get_user_pages and DMA e.g [1] to understand Jan's POV. It does mention GUP > pin pages so I also used the word 'pin'. But guest does not permanently pin > these pages and these pages can be reclaimed by host. OK, then I was just confused how virtio-pmem is going to work. Thanks for explanation! So can I imagine this as guest mmaping the host file and providing the mapped range as "NVDIMM pages" to the kernel inside the guest? Or is it more complex? Honza -- Jan Kara SUSE Labs, CR ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
