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 <[email protected]>
Reviewed-by: Davidlohr Bueso <[email protected]>
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
