* [PATCH v2 2/3] libnvdimm/security: the 'security' attr never show 'overwrite' state
2020-08-03 22:41 [PATCH v2 1/3] libnvdimm/security: fix a typo Jane Chu
@ 2020-08-03 22:41 ` Jane Chu
2020-08-06 19:26 ` Ira Weiny
2020-08-03 22:41 ` [PATCH v2 3/3] libnvdimm/security: ensure sysfs poll thread woke up and fetch updated attr Jane Chu
2020-08-06 19:25 ` [PATCH v2 1/3] libnvdimm/security: fix a typo Ira Weiny
2 siblings, 1 reply; 6+ messages in thread
From: Jane Chu @ 2020-08-03 22:41 UTC (permalink / raw)
To: dan.j.williams, vishal.l.verma, dave.jiang, ira.weiny, jmoyer,
linux-nvdimm, linux-kernel
Cc: jane.chu
'security' attribute displays the security state of an nvdimm.
During normal operation, the nvdimm state maybe one of 'disabled',
'unlocked' or 'locked'. When an admin issues
# ndctl sanitize-dimm nmem0 --overwrite
the attribute is expected to change to 'overwrite' until the overwrite
operation completes.
But tests on our systems show that 'overwrite' is never shown during
the overwrite operation. i.e.
# cat /sys/devices/LNXSYSTM:00/LNXSYBUS:00/ACPI0012:00/ndbus0/nmem0/security
unlocked
the attribute remain 'unlocked' through out the operation, consequently
"ndctl wait-overwrite nmem0" command doesn't wait at all.
The driver tracks the state in 'nvdimm->sec.flags': when the operation
starts, it adds an overwrite bit to the flags; and when the operation
completes, it removes the bit. Hence security_show() should check the
'overwrite' bit first, in order to indicate the actual state when multiple
bits are set in the flags.
Signed-off-by: Jane Chu <jane.chu@oracle.com>
Reviewed-by: Dave Jiang <dave.jiang@intel.com>
---
drivers/nvdimm/dimm_devs.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/nvdimm/dimm_devs.c b/drivers/nvdimm/dimm_devs.c
index b7b77e8..5d72026 100644
--- a/drivers/nvdimm/dimm_devs.c
+++ b/drivers/nvdimm/dimm_devs.c
@@ -363,14 +363,14 @@ __weak ssize_t security_show(struct device *dev,
{
struct nvdimm *nvdimm = to_nvdimm(dev);
+ if (test_bit(NVDIMM_SECURITY_OVERWRITE, &nvdimm->sec.flags))
+ return sprintf(buf, "overwrite\n");
if (test_bit(NVDIMM_SECURITY_DISABLED, &nvdimm->sec.flags))
return sprintf(buf, "disabled\n");
if (test_bit(NVDIMM_SECURITY_UNLOCKED, &nvdimm->sec.flags))
return sprintf(buf, "unlocked\n");
if (test_bit(NVDIMM_SECURITY_LOCKED, &nvdimm->sec.flags))
return sprintf(buf, "locked\n");
- if (test_bit(NVDIMM_SECURITY_OVERWRITE, &nvdimm->sec.flags))
- return sprintf(buf, "overwrite\n");
return -ENOTTY;
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v2 2/3] libnvdimm/security: the 'security' attr never show 'overwrite' state
2020-08-03 22:41 ` [PATCH v2 2/3] libnvdimm/security: the 'security' attr never show 'overwrite' state Jane Chu
@ 2020-08-06 19:26 ` Ira Weiny
0 siblings, 0 replies; 6+ messages in thread
From: Ira Weiny @ 2020-08-06 19:26 UTC (permalink / raw)
To: Jane Chu
Cc: dan.j.williams, vishal.l.verma, dave.jiang, jmoyer, linux-nvdimm,
linux-kernel
On Mon, Aug 03, 2020 at 04:41:38PM -0600, Jane Chu wrote:
> 'security' attribute displays the security state of an nvdimm.
> During normal operation, the nvdimm state maybe one of 'disabled',
> 'unlocked' or 'locked'. When an admin issues
> # ndctl sanitize-dimm nmem0 --overwrite
> the attribute is expected to change to 'overwrite' until the overwrite
> operation completes.
>
> But tests on our systems show that 'overwrite' is never shown during
> the overwrite operation. i.e.
> # cat /sys/devices/LNXSYSTM:00/LNXSYBUS:00/ACPI0012:00/ndbus0/nmem0/security
> unlocked
> the attribute remain 'unlocked' through out the operation, consequently
> "ndctl wait-overwrite nmem0" command doesn't wait at all.
>
> The driver tracks the state in 'nvdimm->sec.flags': when the operation
> starts, it adds an overwrite bit to the flags; and when the operation
> completes, it removes the bit. Hence security_show() should check the
> 'overwrite' bit first, in order to indicate the actual state when multiple
> bits are set in the flags.
>
> Signed-off-by: Jane Chu <jane.chu@oracle.com>
> Reviewed-by: Dave Jiang <dave.jiang@intel.com>
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
> ---
> drivers/nvdimm/dimm_devs.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/nvdimm/dimm_devs.c b/drivers/nvdimm/dimm_devs.c
> index b7b77e8..5d72026 100644
> --- a/drivers/nvdimm/dimm_devs.c
> +++ b/drivers/nvdimm/dimm_devs.c
> @@ -363,14 +363,14 @@ __weak ssize_t security_show(struct device *dev,
> {
> struct nvdimm *nvdimm = to_nvdimm(dev);
>
> + if (test_bit(NVDIMM_SECURITY_OVERWRITE, &nvdimm->sec.flags))
> + return sprintf(buf, "overwrite\n");
> if (test_bit(NVDIMM_SECURITY_DISABLED, &nvdimm->sec.flags))
> return sprintf(buf, "disabled\n");
> if (test_bit(NVDIMM_SECURITY_UNLOCKED, &nvdimm->sec.flags))
> return sprintf(buf, "unlocked\n");
> if (test_bit(NVDIMM_SECURITY_LOCKED, &nvdimm->sec.flags))
> return sprintf(buf, "locked\n");
> - if (test_bit(NVDIMM_SECURITY_OVERWRITE, &nvdimm->sec.flags))
> - return sprintf(buf, "overwrite\n");
> return -ENOTTY;
> }
>
> --
> 1.8.3.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 3/3] libnvdimm/security: ensure sysfs poll thread woke up and fetch updated attr
2020-08-03 22:41 [PATCH v2 1/3] libnvdimm/security: fix a typo Jane Chu
2020-08-03 22:41 ` [PATCH v2 2/3] libnvdimm/security: the 'security' attr never show 'overwrite' state Jane Chu
@ 2020-08-03 22:41 ` Jane Chu
2020-08-06 19:26 ` Ira Weiny
2020-08-06 19:25 ` [PATCH v2 1/3] libnvdimm/security: fix a typo Ira Weiny
2 siblings, 1 reply; 6+ messages in thread
From: Jane Chu @ 2020-08-03 22:41 UTC (permalink / raw)
To: dan.j.williams, vishal.l.verma, dave.jiang, ira.weiny, jmoyer,
linux-nvdimm, linux-kernel
Cc: jane.chu
commit 7d988097c546 ("acpi/nfit, libnvdimm/security: Add security DSM overwrite support")
adds a sysfs_notify_dirent() to wake up userspace poll thread when the "overwrite"
operation has completed. But the notification is issued before the internal
dimm security state and flags have been updated, so the userspace poll thread
wakes up and fetches the not-yet-updated attr and falls back to sleep, forever.
But if user from another terminal issue "ndctl wait-overwrite nmemX" again,
the command returns instantly.
Cc: Dave Jiang <dave.jiang@intel.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Fixes: 7d988097c546 ("acpi/nfit, libnvdimm/security: Add security DSM overwrite support")
Signed-off-by: Jane Chu <jane.chu@oracle.com>
Reviewed-by: Dave Jiang <dave.jiang@intel.com>
---
drivers/nvdimm/security.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/drivers/nvdimm/security.c b/drivers/nvdimm/security.c
index 8f3971c..4b80150 100644
--- a/drivers/nvdimm/security.c
+++ b/drivers/nvdimm/security.c
@@ -450,14 +450,19 @@ void __nvdimm_security_overwrite_query(struct nvdimm *nvdimm)
else
dev_dbg(&nvdimm->dev, "overwrite completed\n");
- if (nvdimm->sec.overwrite_state)
- sysfs_notify_dirent(nvdimm->sec.overwrite_state);
+ /*
+ * Mark the overwrite work done and update dimm security flags,
+ * then send a sysfs event notification to wake up userspace
+ * poll threads to picked up the changed state.
+ */
nvdimm->sec.overwrite_tmo = 0;
clear_bit(NDD_SECURITY_OVERWRITE, &nvdimm->flags);
clear_bit(NDD_WORK_PENDING, &nvdimm->flags);
- put_device(&nvdimm->dev);
nvdimm->sec.flags = nvdimm_security_flags(nvdimm, NVDIMM_USER);
nvdimm->sec.ext_flags = nvdimm_security_flags(nvdimm, NVDIMM_MASTER);
+ if (nvdimm->sec.overwrite_state)
+ sysfs_notify_dirent(nvdimm->sec.overwrite_state);
+ put_device(&nvdimm->dev);
}
void nvdimm_security_overwrite_query(struct work_struct *work)
--
1.8.3.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v2 3/3] libnvdimm/security: ensure sysfs poll thread woke up and fetch updated attr
2020-08-03 22:41 ` [PATCH v2 3/3] libnvdimm/security: ensure sysfs poll thread woke up and fetch updated attr Jane Chu
@ 2020-08-06 19:26 ` Ira Weiny
0 siblings, 0 replies; 6+ messages in thread
From: Ira Weiny @ 2020-08-06 19:26 UTC (permalink / raw)
To: Jane Chu
Cc: dan.j.williams, vishal.l.verma, dave.jiang, jmoyer, linux-nvdimm,
linux-kernel
On Mon, Aug 03, 2020 at 04:41:39PM -0600, Jane Chu wrote:
> commit 7d988097c546 ("acpi/nfit, libnvdimm/security: Add security DSM overwrite support")
> adds a sysfs_notify_dirent() to wake up userspace poll thread when the "overwrite"
> operation has completed. But the notification is issued before the internal
> dimm security state and flags have been updated, so the userspace poll thread
> wakes up and fetches the not-yet-updated attr and falls back to sleep, forever.
> But if user from another terminal issue "ndctl wait-overwrite nmemX" again,
> the command returns instantly.
>
> Cc: Dave Jiang <dave.jiang@intel.com>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Fixes: 7d988097c546 ("acpi/nfit, libnvdimm/security: Add security DSM overwrite support")
> Signed-off-by: Jane Chu <jane.chu@oracle.com>
> Reviewed-by: Dave Jiang <dave.jiang@intel.com>
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
> ---
> drivers/nvdimm/security.c | 11 ++++++++---
> 1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/nvdimm/security.c b/drivers/nvdimm/security.c
> index 8f3971c..4b80150 100644
> --- a/drivers/nvdimm/security.c
> +++ b/drivers/nvdimm/security.c
> @@ -450,14 +450,19 @@ void __nvdimm_security_overwrite_query(struct nvdimm *nvdimm)
> else
> dev_dbg(&nvdimm->dev, "overwrite completed\n");
>
> - if (nvdimm->sec.overwrite_state)
> - sysfs_notify_dirent(nvdimm->sec.overwrite_state);
> + /*
> + * Mark the overwrite work done and update dimm security flags,
> + * then send a sysfs event notification to wake up userspace
> + * poll threads to picked up the changed state.
> + */
> nvdimm->sec.overwrite_tmo = 0;
> clear_bit(NDD_SECURITY_OVERWRITE, &nvdimm->flags);
> clear_bit(NDD_WORK_PENDING, &nvdimm->flags);
> - put_device(&nvdimm->dev);
> nvdimm->sec.flags = nvdimm_security_flags(nvdimm, NVDIMM_USER);
> nvdimm->sec.ext_flags = nvdimm_security_flags(nvdimm, NVDIMM_MASTER);
> + if (nvdimm->sec.overwrite_state)
> + sysfs_notify_dirent(nvdimm->sec.overwrite_state);
> + put_device(&nvdimm->dev);
> }
>
> void nvdimm_security_overwrite_query(struct work_struct *work)
> --
> 1.8.3.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/3] libnvdimm/security: fix a typo
2020-08-03 22:41 [PATCH v2 1/3] libnvdimm/security: fix a typo Jane Chu
2020-08-03 22:41 ` [PATCH v2 2/3] libnvdimm/security: the 'security' attr never show 'overwrite' state Jane Chu
2020-08-03 22:41 ` [PATCH v2 3/3] libnvdimm/security: ensure sysfs poll thread woke up and fetch updated attr Jane Chu
@ 2020-08-06 19:25 ` Ira Weiny
2 siblings, 0 replies; 6+ messages in thread
From: Ira Weiny @ 2020-08-06 19:25 UTC (permalink / raw)
To: Jane Chu
Cc: dan.j.williams, vishal.l.verma, dave.jiang, jmoyer, linux-nvdimm,
linux-kernel
On Mon, Aug 03, 2020 at 04:41:37PM -0600, Jane Chu wrote:
> commit d78c620a2e82 ("libnvdimm/security: Introduce a 'frozen' attribute")
> introduced a typo, causing a 'nvdimm->sec.flags' update being overwritten
> by the subsequent update meant for 'nvdimm->sec.ext_flags'.
>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Fixes: d78c620a2e82 ("libnvdimm/security: Introduce a 'frozen' attribute")
> Signed-off-by: Jane Chu <jane.chu@oracle.com>
> Reviewed-by: Dave Jiang <dave.jiang@intel.com>
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
> ---
> drivers/nvdimm/security.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/nvdimm/security.c b/drivers/nvdimm/security.c
> index 4cef69b..8f3971c 100644
> --- a/drivers/nvdimm/security.c
> +++ b/drivers/nvdimm/security.c
> @@ -457,7 +457,7 @@ void __nvdimm_security_overwrite_query(struct nvdimm *nvdimm)
> clear_bit(NDD_WORK_PENDING, &nvdimm->flags);
> put_device(&nvdimm->dev);
> nvdimm->sec.flags = nvdimm_security_flags(nvdimm, NVDIMM_USER);
> - nvdimm->sec.flags = nvdimm_security_flags(nvdimm, NVDIMM_MASTER);
> + nvdimm->sec.ext_flags = nvdimm_security_flags(nvdimm, NVDIMM_MASTER);
> }
>
> void nvdimm_security_overwrite_query(struct work_struct *work)
> --
> 1.8.3.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread