diff mbox series

[4/5] nvdimm/region: Move cache management to the region driver

Message ID 166993221550.1995348.16843505129579060258.stgit@dwillia2-xfh.jf.intel.com (mailing list archive)
State Accepted
Commit dc370b28c8425669e7ed5af4c01540645cfb00ec
Headers show
Series cxl, nvdimm: Move CPU cache management to region drivers | expand

Commit Message

Dan Williams Dec. 1, 2022, 10:03 p.m. UTC 
Now that cpu_cache_invalidate_memregion() is generically available, use
it to centralize CPU cache management in the nvdimm region driver.

This trades off removing redundant per-dimm CPU cache flushing with an
opportunistic flush on every region disable event to cover the case of
sensitive dirty data in the cache being written back to media after a
secure erase / overwrite event.

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/acpi/nfit/intel.c    |   25 ---------------------
 drivers/nvdimm/region.c      |   11 +++++++++
 drivers/nvdimm/region_devs.c |   49 +++++++++++++++++++++++++++++++++++++++++-
 drivers/nvdimm/security.c    |    6 +++++
 include/linux/libnvdimm.h    |    5 ++++
 5 files changed, 70 insertions(+), 26 deletions(-)

Comments

Dave Jiang Dec. 1, 2022, 11 p.m. UTC | #1 
On 12/1/2022 3:03 PM, Dan Williams wrote:
> Now that cpu_cache_invalidate_memregion() is generically available, use
> it to centralize CPU cache management in the nvdimm region driver.
> 
> This trades off removing redundant per-dimm CPU cache flushing with an
> opportunistic flush on every region disable event to cover the case of
> sensitive dirty data in the cache being written back to media after a
> secure erase / overwrite event.
> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

One minor bit below, otherwise
Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> ---
>   drivers/acpi/nfit/intel.c    |   25 ---------------------
>   drivers/nvdimm/region.c      |   11 +++++++++
>   drivers/nvdimm/region_devs.c |   49 +++++++++++++++++++++++++++++++++++++++++-
>   drivers/nvdimm/security.c    |    6 +++++
>   include/linux/libnvdimm.h    |    5 ++++
>   5 files changed, 70 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/acpi/nfit/intel.c b/drivers/acpi/nfit/intel.c
> index fa0e57e35162..3902759abcba 100644
> --- a/drivers/acpi/nfit/intel.c
> +++ b/drivers/acpi/nfit/intel.c
> @@ -212,9 +212,6 @@ static int __maybe_unused intel_security_unlock(struct nvdimm *nvdimm,
>   	if (!test_bit(NVDIMM_INTEL_UNLOCK_UNIT, &nfit_mem->dsm_mask))
>   		return -ENOTTY;
>   
> -	if (!cpu_cache_has_invalidate_memregion())
> -		return -EINVAL;
> -
>   	memcpy(nd_cmd.cmd.passphrase, key_data->data,
>   			sizeof(nd_cmd.cmd.passphrase));
>   	rc = nvdimm_ctl(nvdimm, ND_CMD_CALL, &nd_cmd, sizeof(nd_cmd), NULL);
> @@ -229,9 +226,6 @@ static int __maybe_unused intel_security_unlock(struct nvdimm *nvdimm,
>   		return -EIO;
>   	}
>   
> -	/* DIMM unlocked, invalidate all CPU caches before we read it */
> -	cpu_cache_invalidate_memregion(IORES_DESC_PERSISTENT_MEMORY);
> -
>   	return 0;
>   }
>   
> @@ -299,11 +293,6 @@ static int __maybe_unused intel_security_erase(struct nvdimm *nvdimm,
>   	if (!test_bit(cmd, &nfit_mem->dsm_mask))
>   		return -ENOTTY;
>   
> -	if (!cpu_cache_has_invalidate_memregion())
> -		return -EINVAL;
> -
> -	/* flush all cache before we erase DIMM */
> -	cpu_cache_invalidate_memregion(IORES_DESC_PERSISTENT_MEMORY);
>   	memcpy(nd_cmd.cmd.passphrase, key->data,
>   			sizeof(nd_cmd.cmd.passphrase));
>   	rc = nvdimm_ctl(nvdimm, ND_CMD_CALL, &nd_cmd, sizeof(nd_cmd), NULL);
> @@ -322,8 +311,6 @@ static int __maybe_unused intel_security_erase(struct nvdimm *nvdimm,
>   		return -ENXIO;
>   	}
>   
> -	/* DIMM erased, invalidate all CPU caches before we read it */
> -	cpu_cache_invalidate_memregion(IORES_DESC_PERSISTENT_MEMORY);
>   	return 0;
>   }
>   
> @@ -346,9 +333,6 @@ static int __maybe_unused intel_security_query_overwrite(struct nvdimm *nvdimm)
>   	if (!test_bit(NVDIMM_INTEL_QUERY_OVERWRITE, &nfit_mem->dsm_mask))
>   		return -ENOTTY;
>   
> -	if (!cpu_cache_has_invalidate_memregion())
> -		return -EINVAL;
> -
>   	rc = nvdimm_ctl(nvdimm, ND_CMD_CALL, &nd_cmd, sizeof(nd_cmd), NULL);
>   	if (rc < 0)
>   		return rc;
> @@ -362,8 +346,6 @@ static int __maybe_unused intel_security_query_overwrite(struct nvdimm *nvdimm)
>   		return -ENXIO;
>   	}
>   
> -	/* flush all cache before we make the nvdimms available */
> -	cpu_cache_invalidate_memregion(IORES_DESC_PERSISTENT_MEMORY);
>   	return 0;
>   }
>   
> @@ -388,11 +370,6 @@ static int __maybe_unused intel_security_overwrite(struct nvdimm *nvdimm,
>   	if (!test_bit(NVDIMM_INTEL_OVERWRITE, &nfit_mem->dsm_mask))
>   		return -ENOTTY;
>   
> -	if (!cpu_cache_has_invalidate_memregion())
> -		return -EINVAL;
> -
> -	/* flush all cache before we erase DIMM */
> -	cpu_cache_invalidate_memregion(IORES_DESC_PERSISTENT_MEMORY);
>   	memcpy(nd_cmd.cmd.passphrase, nkey->data,
>   			sizeof(nd_cmd.cmd.passphrase));
>   	rc = nvdimm_ctl(nvdimm, ND_CMD_CALL, &nd_cmd, sizeof(nd_cmd), NULL);
> @@ -770,5 +747,3 @@ static const struct nvdimm_fw_ops __intel_fw_ops = {
>   };
>   
>   const struct nvdimm_fw_ops *intel_fw_ops = &__intel_fw_ops;
> -
> -MODULE_IMPORT_NS(DEVMEM);
> diff --git a/drivers/nvdimm/region.c b/drivers/nvdimm/region.c
> index 390123d293ea..88dc062af5f8 100644
> --- a/drivers/nvdimm/region.c
> +++ b/drivers/nvdimm/region.c
> @@ -2,6 +2,7 @@
>   /*
>    * Copyright(c) 2013-2015 Intel Corporation. All rights reserved.
>    */
> +#include <linux/memregion.h>
>   #include <linux/cpumask.h>
>   #include <linux/module.h>
>   #include <linux/device.h>
> @@ -100,6 +101,16 @@ static void nd_region_remove(struct device *dev)
>   	 */
>   	sysfs_put(nd_region->bb_state);
>   	nd_region->bb_state = NULL;
> +
> +	/*
> +	 * Try to flush caches here since a disabled region may be subject to
> +	 * secure erase while disabled, and previous dirty data should not be
> +	 * written back to a new instance of the region. This only matters on
> +	 * bare metal where security commands are available, so silent failure
> +	 * here is ok.
> +	 */
> +	if (cpu_cache_has_invalidate_memregion())
> +		cpu_cache_invalidate_memregion(IORES_DESC_PERSISTENT_MEMORY);
>   }
>   
>   static int child_notify(struct device *dev, void *data)
> diff --git a/drivers/nvdimm/region_devs.c b/drivers/nvdimm/region_devs.c
> index e0875d369762..c73e3b1fd0a6 100644
> --- a/drivers/nvdimm/region_devs.c
> +++ b/drivers/nvdimm/region_devs.c
> @@ -59,13 +59,57 @@ static int nvdimm_map_flush(struct device *dev, struct nvdimm *nvdimm, int dimm,
>   	return 0;
>   }
>   
> +static int nd_region_invalidate_memregion(struct nd_region *nd_region)
> +{
> +	int i, incoherent = 0;
> +
> +	for (i = 0; i < nd_region->ndr_mappings; i++) {
> +		struct nd_mapping *nd_mapping = &nd_region->mapping[i];
> +		struct nvdimm *nvdimm = nd_mapping->nvdimm;
> +
> +		if (test_bit(NDD_INCOHERENT, &nvdimm->flags))
> +			incoherent++;
> +	}
> +
> +	if (!incoherent)
> +		return 0;
> +
> +	if (!cpu_cache_has_invalidate_memregion()) {
> +		if (IS_ENABLED(CONFIG_NVDIMM_SECURITY_TEST)) {
> +			dev_warn(
> +				&nd_region->dev,
> +				"Bypassing cpu_cache_invalidate_memergion() for testing!\n");
> +			goto out;
> +		} else {
> +			dev_err(&nd_region->dev,
> +				"Failed to synchronize CPU cache state\n");
> +			return -ENXIO;
> +		}
> +	}
> +
> +	cpu_cache_invalidate_memregion(IORES_DESC_PERSISTENT_MEMORY);
> +out:
> +	for (i = 0; i < nd_region->ndr_mappings; i++) {
> +		struct nd_mapping *nd_mapping = &nd_region->mapping[i];
> +		struct nvdimm *nvdimm = nd_mapping->nvdimm;
> +
> +		clear_bit(NDD_INCOHERENT, &nvdimm->flags);
> +	}
> +
> +	return 0;
> +}
> +
>   int nd_region_activate(struct nd_region *nd_region)
>   {
> -	int i, j, num_flush = 0;
> +	int i, j, rc, num_flush = 0;
>   	struct nd_region_data *ndrd;
>   	struct device *dev = &nd_region->dev;
>   	size_t flush_data_size = sizeof(void *);
>   
> +	rc = nd_region_invalidate_memregion(nd_region);
> +	if (rc)
> +		return rc;
> +
>   	nvdimm_bus_lock(&nd_region->dev);
>   	for (i = 0; i < nd_region->ndr_mappings; i++) {
>   		struct nd_mapping *nd_mapping = &nd_region->mapping[i];
> @@ -85,6 +129,7 @@ int nd_region_activate(struct nd_region *nd_region)
>   	}
>   	nvdimm_bus_unlock(&nd_region->dev);
>   
> +

Extraneous blankline

DJ

>   	ndrd = devm_kzalloc(dev, sizeof(*ndrd) + flush_data_size, GFP_KERNEL);
>   	if (!ndrd)
>   		return -ENOMEM;
> @@ -1222,3 +1267,5 @@ int nd_region_conflict(struct nd_region *nd_region, resource_size_t start,
>   
>   	return device_for_each_child(&nvdimm_bus->dev, &ctx, region_conflict);
>   }
> +
> +MODULE_IMPORT_NS(DEVMEM);
> diff --git a/drivers/nvdimm/security.c b/drivers/nvdimm/security.c
> index 6814339b3dab..a03e3c45f297 100644
> --- a/drivers/nvdimm/security.c
> +++ b/drivers/nvdimm/security.c
> @@ -208,6 +208,8 @@ static int __nvdimm_security_unlock(struct nvdimm *nvdimm)
>   	rc = nvdimm->sec.ops->unlock(nvdimm, data);
>   	dev_dbg(dev, "key: %d unlock: %s\n", key_serial(key),
>   			rc == 0 ? "success" : "fail");
> +	if (rc == 0)
> +		set_bit(NDD_INCOHERENT, &nvdimm->flags);
>   
>   	nvdimm_put_key(key);
>   	nvdimm->sec.flags = nvdimm_security_flags(nvdimm, NVDIMM_USER);
> @@ -374,6 +376,8 @@ static int security_erase(struct nvdimm *nvdimm, unsigned int keyid,
>   		return -ENOKEY;
>   
>   	rc = nvdimm->sec.ops->erase(nvdimm, data, pass_type);
> +	if (rc == 0)
> +		set_bit(NDD_INCOHERENT, &nvdimm->flags);
>   	dev_dbg(dev, "key: %d erase%s: %s\n", key_serial(key),
>   			pass_type == NVDIMM_MASTER ? "(master)" : "(user)",
>   			rc == 0 ? "success" : "fail");
> @@ -408,6 +412,8 @@ static int security_overwrite(struct nvdimm *nvdimm, unsigned int keyid)
>   		return -ENOKEY;
>   
>   	rc = nvdimm->sec.ops->overwrite(nvdimm, data);
> +	if (rc == 0)
> +		set_bit(NDD_INCOHERENT, &nvdimm->flags);
>   	dev_dbg(dev, "key: %d overwrite submission: %s\n", key_serial(key),
>   			rc == 0 ? "success" : "fail");
>   
> diff --git a/include/linux/libnvdimm.h b/include/linux/libnvdimm.h
> index 3bf658a74ccb..af38252ad704 100644
> --- a/include/linux/libnvdimm.h
> +++ b/include/linux/libnvdimm.h
> @@ -35,6 +35,11 @@ enum {
>   	NDD_WORK_PENDING = 4,
>   	/* dimm supports namespace labels */
>   	NDD_LABELING = 6,
> +	/*
> +	 * dimm contents have changed requiring invalidation of CPU caches prior
> +	 * to activation of a region that includes this device
> +	 */
> +	NDD_INCOHERENT = 7,
>   
>   	/* need to set a limit somewhere, but yes, this is likely overkill */
>   	ND_IOCTL_MAX_BUFLEN = SZ_4M,
>
Davidlohr Bueso Dec. 2, 2022, 3:21 a.m. UTC | #2 
On Thu, 01 Dec 2022, Dan Williams wrote:

>Now that cpu_cache_invalidate_memregion() is generically available, use
>it to centralize CPU cache management in the nvdimm region driver.
>
>This trades off removing redundant per-dimm CPU cache flushing with an
>opportunistic flush on every region disable event to cover the case of
>sensitive dirty data in the cache being written back to media after a
>secure erase / overwrite event.

Very nifty.

>Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>

with a few notes below.

>+static int nd_region_invalidate_memregion(struct nd_region *nd_region)
>+{
>+	int i, incoherent = 0;
>+
>+	for (i = 0; i < nd_region->ndr_mappings; i++) {
>+		struct nd_mapping *nd_mapping = &nd_region->mapping[i];
>+		struct nvdimm *nvdimm = nd_mapping->nvdimm;
>+
>+		if (test_bit(NDD_INCOHERENT, &nvdimm->flags))
>+			incoherent++;

No need to compute the rest, just break out here?

>+	}
>+
>+	if (!incoherent)
>+		return 0;
>+
>+	if (!cpu_cache_has_invalidate_memregion()) {
>+		if (IS_ENABLED(CONFIG_NVDIMM_SECURITY_TEST)) {
>+			dev_warn(
>+				&nd_region->dev,
>+				"Bypassing cpu_cache_invalidate_memergion() for testing!\n");
>+			goto out;
>+		} else {
>+			dev_err(&nd_region->dev,
>+				"Failed to synchronize CPU cache state\n");
>+			return -ENXIO;
>+		}
>+	}
>+
>+	cpu_cache_invalidate_memregion(IORES_DESC_PERSISTENT_MEMORY);
>+out:
>+	for (i = 0; i < nd_region->ndr_mappings; i++) {
>+		struct nd_mapping *nd_mapping = &nd_region->mapping[i];
>+		struct nvdimm *nvdimm = nd_mapping->nvdimm;
>+
>+		clear_bit(NDD_INCOHERENT, &nvdimm->flags);
>+	}
>+
>+	return 0;
>+}
>+
> int nd_region_activate(struct nd_region *nd_region)
> {
>-	int i, j, num_flush = 0;
>+	int i, j, rc, num_flush = 0;
>	struct nd_region_data *ndrd;
>	struct device *dev = &nd_region->dev;
>	size_t flush_data_size = sizeof(void *);
>
>+	rc = nd_region_invalidate_memregion(nd_region);
>+	if (rc)
>+		return rc;
>+
>	nvdimm_bus_lock(&nd_region->dev);
>	for (i = 0; i < nd_region->ndr_mappings; i++) {
>		struct nd_mapping *nd_mapping = &nd_region->mapping[i];
>@@ -85,6 +129,7 @@ int nd_region_activate(struct nd_region *nd_region)
>	}
>	nvdimm_bus_unlock(&nd_region->dev);
>
>+
>	ndrd = devm_kzalloc(dev, sizeof(*ndrd) + flush_data_size, GFP_KERNEL);
>	if (!ndrd)
>		return -ENOMEM;
>@@ -1222,3 +1267,5 @@ int nd_region_conflict(struct nd_region *nd_region, resource_size_t start,
>
>	return device_for_each_child(&nvdimm_bus->dev, &ctx, region_conflict);
> }
>+
>+MODULE_IMPORT_NS(DEVMEM);
>diff --git a/drivers/nvdimm/security.c b/drivers/nvdimm/security.c
>index 6814339b3dab..a03e3c45f297 100644
>--- a/drivers/nvdimm/security.c
>+++ b/drivers/nvdimm/security.c
>@@ -208,6 +208,8 @@ static int __nvdimm_security_unlock(struct nvdimm *nvdimm)
>	rc = nvdimm->sec.ops->unlock(nvdimm, data);
>	dev_dbg(dev, "key: %d unlock: %s\n", key_serial(key),
>			rc == 0 ? "success" : "fail");
>+	if (rc == 0)
>+		set_bit(NDD_INCOHERENT, &nvdimm->flags);
>
>	nvdimm_put_key(key);
>	nvdimm->sec.flags = nvdimm_security_flags(nvdimm, NVDIMM_USER);
>@@ -374,6 +376,8 @@ static int security_erase(struct nvdimm *nvdimm, unsigned int keyid,
>		return -ENOKEY;
>
>	rc = nvdimm->sec.ops->erase(nvdimm, data, pass_type);
>+	if (rc == 0)
>+		set_bit(NDD_INCOHERENT, &nvdimm->flags);
>	dev_dbg(dev, "key: %d erase%s: %s\n", key_serial(key),
>			pass_type == NVDIMM_MASTER ? "(master)" : "(user)",
>			rc == 0 ? "success" : "fail");
>@@ -408,6 +412,8 @@ static int security_overwrite(struct nvdimm *nvdimm, unsigned int keyid)
>		return -ENOKEY;
>
>	rc = nvdimm->sec.ops->overwrite(nvdimm, data);
>+	if (rc == 0)
>+		set_bit(NDD_INCOHERENT, &nvdimm->flags);

Are you relying on hw preventing an incoming region_activate() while the overwrite
operation is in progress to ensure that the flags are stable throughout the whole
op? Currently query-overwrite also provides the flushing guarantees for when the
command is actually complete (at least from a user pov).

Thanks,
Davidlohr
Dan Williams Dec. 3, 2022, 8:01 a.m. UTC | #3 
Davidlohr Bueso wrote:
> On Thu, 01 Dec 2022, Dan Williams wrote:
> 
> >Now that cpu_cache_invalidate_memregion() is generically available, use
> >it to centralize CPU cache management in the nvdimm region driver.
> >
> >This trades off removing redundant per-dimm CPU cache flushing with an
> >opportunistic flush on every region disable event to cover the case of
> >sensitive dirty data in the cache being written back to media after a
> >secure erase / overwrite event.
> 
> Very nifty.
> 
> >Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> 
> Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>
> 
> with a few notes below.
> 
> >+static int nd_region_invalidate_memregion(struct nd_region *nd_region)
> >+{
> >+	int i, incoherent = 0;
> >+
> >+	for (i = 0; i < nd_region->ndr_mappings; i++) {
> >+		struct nd_mapping *nd_mapping = &nd_region->mapping[i];
> >+		struct nvdimm *nvdimm = nd_mapping->nvdimm;
> >+
> >+		if (test_bit(NDD_INCOHERENT, &nvdimm->flags))
> >+			incoherent++;
> 
> No need to compute the rest, just break out here?

Sure, makes sense.

> 
> >+	}
> >+
> >+	if (!incoherent)
> >+		return 0;
> >+
> >+	if (!cpu_cache_has_invalidate_memregion()) {
> >+		if (IS_ENABLED(CONFIG_NVDIMM_SECURITY_TEST)) {
> >+			dev_warn(
> >+				&nd_region->dev,
> >+				"Bypassing cpu_cache_invalidate_memergion() for testing!\n");
> >+			goto out;
> >+		} else {
> >+			dev_err(&nd_region->dev,
> >+				"Failed to synchronize CPU cache state\n");
> >+			return -ENXIO;
> >+		}
> >+	}
> >+
> >+	cpu_cache_invalidate_memregion(IORES_DESC_PERSISTENT_MEMORY);
> >+out:
> >+	for (i = 0; i < nd_region->ndr_mappings; i++) {
> >+		struct nd_mapping *nd_mapping = &nd_region->mapping[i];
> >+		struct nvdimm *nvdimm = nd_mapping->nvdimm;
> >+
> >+		clear_bit(NDD_INCOHERENT, &nvdimm->flags);
> >+	}
> >+
> >+	return 0;
> >+}
> >+
> > int nd_region_activate(struct nd_region *nd_region)
> > {
> >-	int i, j, num_flush = 0;
> >+	int i, j, rc, num_flush = 0;
> >	struct nd_region_data *ndrd;
> >	struct device *dev = &nd_region->dev;
> >	size_t flush_data_size = sizeof(void *);
> >
> >+	rc = nd_region_invalidate_memregion(nd_region);
> >+	if (rc)
> >+		return rc;
> >+
> >	nvdimm_bus_lock(&nd_region->dev);
> >	for (i = 0; i < nd_region->ndr_mappings; i++) {
> >		struct nd_mapping *nd_mapping = &nd_region->mapping[i];
> >@@ -85,6 +129,7 @@ int nd_region_activate(struct nd_region *nd_region)
> >	}
> >	nvdimm_bus_unlock(&nd_region->dev);
> >
> >+
> >	ndrd = devm_kzalloc(dev, sizeof(*ndrd) + flush_data_size, GFP_KERNEL);
> >	if (!ndrd)
> >		return -ENOMEM;
> >@@ -1222,3 +1267,5 @@ int nd_region_conflict(struct nd_region *nd_region, resource_size_t start,
> >
> >	return device_for_each_child(&nvdimm_bus->dev, &ctx, region_conflict);
> > }
> >+
> >+MODULE_IMPORT_NS(DEVMEM);
> >diff --git a/drivers/nvdimm/security.c b/drivers/nvdimm/security.c
> >index 6814339b3dab..a03e3c45f297 100644
> >--- a/drivers/nvdimm/security.c
> >+++ b/drivers/nvdimm/security.c
> >@@ -208,6 +208,8 @@ static int __nvdimm_security_unlock(struct nvdimm *nvdimm)
> >	rc = nvdimm->sec.ops->unlock(nvdimm, data);
> >	dev_dbg(dev, "key: %d unlock: %s\n", key_serial(key),
> >			rc == 0 ? "success" : "fail");
> >+	if (rc == 0)
> >+		set_bit(NDD_INCOHERENT, &nvdimm->flags);
> >
> >	nvdimm_put_key(key);
> >	nvdimm->sec.flags = nvdimm_security_flags(nvdimm, NVDIMM_USER);
> >@@ -374,6 +376,8 @@ static int security_erase(struct nvdimm *nvdimm, unsigned int keyid,
> >		return -ENOKEY;
> >
> >	rc = nvdimm->sec.ops->erase(nvdimm, data, pass_type);
> >+	if (rc == 0)
> >+		set_bit(NDD_INCOHERENT, &nvdimm->flags);
> >	dev_dbg(dev, "key: %d erase%s: %s\n", key_serial(key),
> >			pass_type == NVDIMM_MASTER ? "(master)" : "(user)",
> >			rc == 0 ? "success" : "fail");
> >@@ -408,6 +412,8 @@ static int security_overwrite(struct nvdimm *nvdimm, unsigned int keyid)
> >		return -ENOKEY;
> >
> >	rc = nvdimm->sec.ops->overwrite(nvdimm, data);
> >+	if (rc == 0)
> >+		set_bit(NDD_INCOHERENT, &nvdimm->flags);
> 
> Are you relying on hw preventing an incoming region_activate() while the overwrite
> operation is in progress to ensure that the flags are stable throughout the whole
> op? Currently query-overwrite also provides the flushing guarantees for when the
> command is actually complete (at least from a user pov).

The driver handles this in nd_region_activate(), but hey, look at that,
nd_region_invalidate_memregion() is too early. I.e. it's before this check:

                if (test_bit(NDD_SECURITY_OVERWRITE, &nvdimm->flags)) {
                        nvdimm_bus_unlock(&nd_region->dev);
                        return -EBUSY;
                }

...which means that the cache could be invalidated too early while the
overwrite is still happening. Will move the cache invalidate below that
check. Thanks for poking at it!

Folded the following:

diff --git a/drivers/nvdimm/region_devs.c b/drivers/nvdimm/region_devs.c
index c73e3b1fd0a6..83dbf398ea84 100644
--- a/drivers/nvdimm/region_devs.c
+++ b/drivers/nvdimm/region_devs.c
@@ -67,8 +67,10 @@ static int nd_region_invalidate_memregion(struct nd_region *nd_region)
                struct nd_mapping *nd_mapping = &nd_region->mapping[i];
                struct nvdimm *nvdimm = nd_mapping->nvdimm;
 
-               if (test_bit(NDD_INCOHERENT, &nvdimm->flags))
+               if (test_bit(NDD_INCOHERENT, &nvdimm->flags)) {
                        incoherent++;
+                       break;
+               }
        }
 
        if (!incoherent)
@@ -106,10 +108,6 @@ int nd_region_activate(struct nd_region *nd_region)
        struct device *dev = &nd_region->dev;
        size_t flush_data_size = sizeof(void *);
 
-       rc = nd_region_invalidate_memregion(nd_region);
-       if (rc)
-               return rc;
-
        nvdimm_bus_lock(&nd_region->dev);
        for (i = 0; i < nd_region->ndr_mappings; i++) {
                struct nd_mapping *nd_mapping = &nd_region->mapping[i];
@@ -129,6 +127,9 @@ int nd_region_activate(struct nd_region *nd_region)
        }
        nvdimm_bus_unlock(&nd_region->dev);
 
+       rc = nd_region_invalidate_memregion(nd_region);
+       if (rc)
+               return rc;
 
        ndrd = devm_kzalloc(dev, sizeof(*ndrd) + flush_data_size, GFP_KERNEL);
        if (!ndrd)
diff mbox series

Patch

diff --git a/drivers/acpi/nfit/intel.c b/drivers/acpi/nfit/intel.c
index fa0e57e35162..3902759abcba 100644
--- a/drivers/acpi/nfit/intel.c
+++ b/drivers/acpi/nfit/intel.c
@@ -212,9 +212,6 @@  static int __maybe_unused intel_security_unlock(struct nvdimm *nvdimm,
 	if (!test_bit(NVDIMM_INTEL_UNLOCK_UNIT, &nfit_mem->dsm_mask))
 		return -ENOTTY;
 
-	if (!cpu_cache_has_invalidate_memregion())
-		return -EINVAL;
-
 	memcpy(nd_cmd.cmd.passphrase, key_data->data,
 			sizeof(nd_cmd.cmd.passphrase));
 	rc = nvdimm_ctl(nvdimm, ND_CMD_CALL, &nd_cmd, sizeof(nd_cmd), NULL);
@@ -229,9 +226,6 @@  static int __maybe_unused intel_security_unlock(struct nvdimm *nvdimm,
 		return -EIO;
 	}
 
-	/* DIMM unlocked, invalidate all CPU caches before we read it */
-	cpu_cache_invalidate_memregion(IORES_DESC_PERSISTENT_MEMORY);
-
 	return 0;
 }
 
@@ -299,11 +293,6 @@  static int __maybe_unused intel_security_erase(struct nvdimm *nvdimm,
 	if (!test_bit(cmd, &nfit_mem->dsm_mask))
 		return -ENOTTY;
 
-	if (!cpu_cache_has_invalidate_memregion())
-		return -EINVAL;
-
-	/* flush all cache before we erase DIMM */
-	cpu_cache_invalidate_memregion(IORES_DESC_PERSISTENT_MEMORY);
 	memcpy(nd_cmd.cmd.passphrase, key->data,
 			sizeof(nd_cmd.cmd.passphrase));
 	rc = nvdimm_ctl(nvdimm, ND_CMD_CALL, &nd_cmd, sizeof(nd_cmd), NULL);
@@ -322,8 +311,6 @@  static int __maybe_unused intel_security_erase(struct nvdimm *nvdimm,
 		return -ENXIO;
 	}
 
-	/* DIMM erased, invalidate all CPU caches before we read it */
-	cpu_cache_invalidate_memregion(IORES_DESC_PERSISTENT_MEMORY);
 	return 0;
 }
 
@@ -346,9 +333,6 @@  static int __maybe_unused intel_security_query_overwrite(struct nvdimm *nvdimm)
 	if (!test_bit(NVDIMM_INTEL_QUERY_OVERWRITE, &nfit_mem->dsm_mask))
 		return -ENOTTY;
 
-	if (!cpu_cache_has_invalidate_memregion())
-		return -EINVAL;
-
 	rc = nvdimm_ctl(nvdimm, ND_CMD_CALL, &nd_cmd, sizeof(nd_cmd), NULL);
 	if (rc < 0)
 		return rc;
@@ -362,8 +346,6 @@  static int __maybe_unused intel_security_query_overwrite(struct nvdimm *nvdimm)
 		return -ENXIO;
 	}
 
-	/* flush all cache before we make the nvdimms available */
-	cpu_cache_invalidate_memregion(IORES_DESC_PERSISTENT_MEMORY);
 	return 0;
 }
 
@@ -388,11 +370,6 @@  static int __maybe_unused intel_security_overwrite(struct nvdimm *nvdimm,
 	if (!test_bit(NVDIMM_INTEL_OVERWRITE, &nfit_mem->dsm_mask))
 		return -ENOTTY;
 
-	if (!cpu_cache_has_invalidate_memregion())
-		return -EINVAL;
-
-	/* flush all cache before we erase DIMM */
-	cpu_cache_invalidate_memregion(IORES_DESC_PERSISTENT_MEMORY);
 	memcpy(nd_cmd.cmd.passphrase, nkey->data,
 			sizeof(nd_cmd.cmd.passphrase));
 	rc = nvdimm_ctl(nvdimm, ND_CMD_CALL, &nd_cmd, sizeof(nd_cmd), NULL);
@@ -770,5 +747,3 @@  static const struct nvdimm_fw_ops __intel_fw_ops = {
 };
 
 const struct nvdimm_fw_ops *intel_fw_ops = &__intel_fw_ops;
-
-MODULE_IMPORT_NS(DEVMEM);
diff --git a/drivers/nvdimm/region.c b/drivers/nvdimm/region.c
index 390123d293ea..88dc062af5f8 100644
--- a/drivers/nvdimm/region.c
+++ b/drivers/nvdimm/region.c
@@ -2,6 +2,7 @@ 
 /*
  * Copyright(c) 2013-2015 Intel Corporation. All rights reserved.
  */
+#include <linux/memregion.h>
 #include <linux/cpumask.h>
 #include <linux/module.h>
 #include <linux/device.h>
@@ -100,6 +101,16 @@  static void nd_region_remove(struct device *dev)
 	 */
 	sysfs_put(nd_region->bb_state);
 	nd_region->bb_state = NULL;
+
+	/*
+	 * Try to flush caches here since a disabled region may be subject to
+	 * secure erase while disabled, and previous dirty data should not be
+	 * written back to a new instance of the region. This only matters on
+	 * bare metal where security commands are available, so silent failure
+	 * here is ok.
+	 */
+	if (cpu_cache_has_invalidate_memregion())
+		cpu_cache_invalidate_memregion(IORES_DESC_PERSISTENT_MEMORY);
 }
 
 static int child_notify(struct device *dev, void *data)
diff --git a/drivers/nvdimm/region_devs.c b/drivers/nvdimm/region_devs.c
index e0875d369762..c73e3b1fd0a6 100644
--- a/drivers/nvdimm/region_devs.c
+++ b/drivers/nvdimm/region_devs.c
@@ -59,13 +59,57 @@  static int nvdimm_map_flush(struct device *dev, struct nvdimm *nvdimm, int dimm,
 	return 0;
 }
 
+static int nd_region_invalidate_memregion(struct nd_region *nd_region)
+{
+	int i, incoherent = 0;
+
+	for (i = 0; i < nd_region->ndr_mappings; i++) {
+		struct nd_mapping *nd_mapping = &nd_region->mapping[i];
+		struct nvdimm *nvdimm = nd_mapping->nvdimm;
+
+		if (test_bit(NDD_INCOHERENT, &nvdimm->flags))
+			incoherent++;
+	}
+
+	if (!incoherent)
+		return 0;
+
+	if (!cpu_cache_has_invalidate_memregion()) {
+		if (IS_ENABLED(CONFIG_NVDIMM_SECURITY_TEST)) {
+			dev_warn(
+				&nd_region->dev,
+				"Bypassing cpu_cache_invalidate_memergion() for testing!\n");
+			goto out;
+		} else {
+			dev_err(&nd_region->dev,
+				"Failed to synchronize CPU cache state\n");
+			return -ENXIO;
+		}
+	}
+
+	cpu_cache_invalidate_memregion(IORES_DESC_PERSISTENT_MEMORY);
+out:
+	for (i = 0; i < nd_region->ndr_mappings; i++) {
+		struct nd_mapping *nd_mapping = &nd_region->mapping[i];
+		struct nvdimm *nvdimm = nd_mapping->nvdimm;
+
+		clear_bit(NDD_INCOHERENT, &nvdimm->flags);
+	}
+
+	return 0;
+}
+
 int nd_region_activate(struct nd_region *nd_region)
 {
-	int i, j, num_flush = 0;
+	int i, j, rc, num_flush = 0;
 	struct nd_region_data *ndrd;
 	struct device *dev = &nd_region->dev;
 	size_t flush_data_size = sizeof(void *);
 
+	rc = nd_region_invalidate_memregion(nd_region);
+	if (rc)
+		return rc;
+
 	nvdimm_bus_lock(&nd_region->dev);
 	for (i = 0; i < nd_region->ndr_mappings; i++) {
 		struct nd_mapping *nd_mapping = &nd_region->mapping[i];
@@ -85,6 +129,7 @@  int nd_region_activate(struct nd_region *nd_region)
 	}
 	nvdimm_bus_unlock(&nd_region->dev);
 
+
 	ndrd = devm_kzalloc(dev, sizeof(*ndrd) + flush_data_size, GFP_KERNEL);
 	if (!ndrd)
 		return -ENOMEM;
@@ -1222,3 +1267,5 @@  int nd_region_conflict(struct nd_region *nd_region, resource_size_t start,
 
 	return device_for_each_child(&nvdimm_bus->dev, &ctx, region_conflict);
 }
+
+MODULE_IMPORT_NS(DEVMEM);
diff --git a/drivers/nvdimm/security.c b/drivers/nvdimm/security.c
index 6814339b3dab..a03e3c45f297 100644
--- a/drivers/nvdimm/security.c
+++ b/drivers/nvdimm/security.c
@@ -208,6 +208,8 @@  static int __nvdimm_security_unlock(struct nvdimm *nvdimm)
 	rc = nvdimm->sec.ops->unlock(nvdimm, data);
 	dev_dbg(dev, "key: %d unlock: %s\n", key_serial(key),
 			rc == 0 ? "success" : "fail");
+	if (rc == 0)
+		set_bit(NDD_INCOHERENT, &nvdimm->flags);
 
 	nvdimm_put_key(key);
 	nvdimm->sec.flags = nvdimm_security_flags(nvdimm, NVDIMM_USER);
@@ -374,6 +376,8 @@  static int security_erase(struct nvdimm *nvdimm, unsigned int keyid,
 		return -ENOKEY;
 
 	rc = nvdimm->sec.ops->erase(nvdimm, data, pass_type);
+	if (rc == 0)
+		set_bit(NDD_INCOHERENT, &nvdimm->flags);
 	dev_dbg(dev, "key: %d erase%s: %s\n", key_serial(key),
 			pass_type == NVDIMM_MASTER ? "(master)" : "(user)",
 			rc == 0 ? "success" : "fail");
@@ -408,6 +412,8 @@  static int security_overwrite(struct nvdimm *nvdimm, unsigned int keyid)
 		return -ENOKEY;
 
 	rc = nvdimm->sec.ops->overwrite(nvdimm, data);
+	if (rc == 0)
+		set_bit(NDD_INCOHERENT, &nvdimm->flags);
 	dev_dbg(dev, "key: %d overwrite submission: %s\n", key_serial(key),
 			rc == 0 ? "success" : "fail");
 
diff --git a/include/linux/libnvdimm.h b/include/linux/libnvdimm.h
index 3bf658a74ccb..af38252ad704 100644
--- a/include/linux/libnvdimm.h
+++ b/include/linux/libnvdimm.h
@@ -35,6 +35,11 @@  enum {
 	NDD_WORK_PENDING = 4,
 	/* dimm supports namespace labels */
 	NDD_LABELING = 6,
+	/*
+	 * dimm contents have changed requiring invalidation of CPU caches prior
+	 * to activation of a region that includes this device
+	 */
+	NDD_INCOHERENT = 7,
 
 	/* need to set a limit somewhere, but yes, this is likely overkill */
 	ND_IOCTL_MAX_BUFLEN = SZ_4M,