The implementation is broken in all the ways the unit test did not touch:
1/ The local definition of in_buf and in_obj violated C99 initializer
expectations for zeroing. By only initializing 2 out of the three
struct members the compiler was free to zero-initialize the remaining
entry even though the aliased location in the union was initialized.
2/ The implementation made assumptions about the state of the 'smart'
payload after command execution that are satisfied by
acpi_nfit_ctl(), but not acpi_evaluate_dsm().
3/ populate_shutdown_status() is skipped on Intel NVDIMMs due to the early
return for skipping the common _LS{I,R,W} enabling.
4/ The input length should be zero.
This breakage was missed due to the unit test implementation only
testing the case where nfit_intel_shutdown_status() returns a valid
payload.
Much of this complexity would be saved if acpi_nfit_ctl() could be used, but
that currently requires a 'struct nvdimm *' argument and one is not created
until later in the init process. The health result is needed before the device
is created because the payload gates whether the nmemX/nfit/dirty_shutdown
property is visible in sysfs.
Cc: <[email protected]>
Fixes: 0ead11181fe0 ("acpi, nfit: Collect shutdown status")
Reported-by: Dexuan Cui <[email protected]>
Signed-off-by: Dan Williams <[email protected]>
---
drivers/acpi/nfit/core.c | 41 ++++++++++++++++++++++++-----------------
1 file changed, 24 insertions(+), 17 deletions(-)
diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index e18ade5d74e9..0a49c57334cc 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -1759,14 +1759,14 @@ static bool acpi_nvdimm_has_method(struct acpi_device *adev, char *method)
__weak void nfit_intel_shutdown_status(struct nfit_mem *nfit_mem)
{
+ struct device *dev = &nfit_mem->adev->dev;
struct nd_intel_smart smart = { 0 };
union acpi_object in_buf = {
- .type = ACPI_TYPE_BUFFER,
- .buffer.pointer = (char *) &smart,
- .buffer.length = sizeof(smart),
+ .buffer.type = ACPI_TYPE_BUFFER,
+ .buffer.length = 0,
};
union acpi_object in_obj = {
- .type = ACPI_TYPE_PACKAGE,
+ .package.type = ACPI_TYPE_PACKAGE,
.package.count = 1,
.package.elements = &in_buf,
};
@@ -1781,8 +1781,15 @@ __weak void nfit_intel_shutdown_status(struct nfit_mem *nfit_mem)
return;
out_obj = acpi_evaluate_dsm(handle, guid, revid, func, &in_obj);
- if (!out_obj)
+ if (!out_obj || out_obj->type != ACPI_TYPE_BUFFER
+ || out_obj->buffer.length < sizeof(smart)) {
+ dev_dbg(dev->parent, "%s: failed to retrieve initial health\n",
+ dev_name(dev));
+ ACPI_FREE(out_obj);
return;
+ }
+ memcpy(&smart, out_obj->buffer.pointer, sizeof(smart));
+ ACPI_FREE(out_obj);
if (smart.flags & ND_INTEL_SMART_SHUTDOWN_VALID) {
if (smart.shutdown_state)
@@ -1793,7 +1800,6 @@ __weak void nfit_intel_shutdown_status(struct nfit_mem *nfit_mem)
set_bit(NFIT_MEM_DIRTY_COUNT, &nfit_mem->flags);
nfit_mem->dirty_shutdown = smart.shutdown_count;
}
- ACPI_FREE(out_obj);
}
static void populate_shutdown_status(struct nfit_mem *nfit_mem)
@@ -1915,18 +1921,19 @@ static int acpi_nfit_add_dimm(struct acpi_nfit_desc *acpi_desc,
| 1 << ND_CMD_SET_CONFIG_DATA;
if (family == NVDIMM_FAMILY_INTEL
&& (dsm_mask & label_mask) == label_mask)
- return 0;
-
- if (acpi_nvdimm_has_method(adev_dimm, "_LSI")
- && acpi_nvdimm_has_method(adev_dimm, "_LSR")) {
- dev_dbg(dev, "%s: has _LSR\n", dev_name(&adev_dimm->dev));
- set_bit(NFIT_MEM_LSR, &nfit_mem->flags);
- }
+ /* skip _LS{I,R,W} enabling */;
+ else {
+ if (acpi_nvdimm_has_method(adev_dimm, "_LSI")
+ && acpi_nvdimm_has_method(adev_dimm, "_LSR")) {
+ dev_dbg(dev, "%s: has _LSR\n", dev_name(&adev_dimm->dev));
+ set_bit(NFIT_MEM_LSR, &nfit_mem->flags);
+ }
- if (test_bit(NFIT_MEM_LSR, &nfit_mem->flags)
- && acpi_nvdimm_has_method(adev_dimm, "_LSW")) {
- dev_dbg(dev, "%s: has _LSW\n", dev_name(&adev_dimm->dev));
- set_bit(NFIT_MEM_LSW, &nfit_mem->flags);
+ if (test_bit(NFIT_MEM_LSR, &nfit_mem->flags)
+ && acpi_nvdimm_has_method(adev_dimm, "_LSW")) {
+ dev_dbg(dev, "%s: has _LSW\n", dev_name(&adev_dimm->dev));
+ set_bit(NFIT_MEM_LSW, &nfit_mem->flags);
+ }
}
populate_shutdown_status(nfit_mem);
> From: Dan Williams <[email protected]>
> Sent: Monday, January 28, 2019 9:22 PM
> To: [email protected]
> Cc: [email protected]; Dexuan Cui <[email protected]>;
> [email protected]
> Subject: [PATCH] nfit: Fix nfit_intel_shutdown_status() command submission
>
> The implementation is broken in all the ways the unit test did not touch:
>
> 1/ The local definition of in_buf and in_obj violated C99 initializer
> expectations for zeroing. By only initializing 2 out of the three
> struct members the compiler was free to zero-initialize the remaining
> entry even though the aliased location in the union was initialized.
>
> 2/ The implementation made assumptions about the state of the 'smart'
> payload after command execution that are satisfied by
> acpi_nfit_ctl(), but not acpi_evaluate_dsm().
>
> 3/ populate_shutdown_status() is skipped on Intel NVDIMMs due to the early
> return for skipping the common _LS{I,R,W} enabling.
>
> 4/ The input length should be zero.
>
> This breakage was missed due to the unit test implementation only
> testing the case where nfit_intel_shutdown_status() returns a valid
> payload.
>
> Much of this complexity would be saved if acpi_nfit_ctl() could be used, but
> that currently requires a 'struct nvdimm *' argument and one is not created
> until later in the init process. The health result is needed before the device
> is created because the payload gates whether the nmemX/nfit/dirty_shutdown
> property is visible in sysfs.
>
> Cc: <[email protected]>
> Fixes: 0ead11181fe0 ("acpi, nfit: Collect shutdown status")
> Reported-by: Dexuan Cui <[email protected]>
> Signed-off-by: Dan Williams <[email protected]>
> ---
> drivers/acpi/nfit/core.c | 41 ++++++++++++++++++++++++-----------------
> 1 file changed, 24 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> index e18ade5d74e9..0a49c57334cc 100644
> --- a/drivers/acpi/nfit/core.c
> +++ b/drivers/acpi/nfit/core.c
> @@ -1759,14 +1759,14 @@ static bool acpi_nvdimm_has_method(struct
> acpi_device *adev, char *method)
>
> __weak void nfit_intel_shutdown_status(struct nfit_mem *nfit_mem)
> {
> + struct device *dev = &nfit_mem->adev->dev;
> struct nd_intel_smart smart = { 0 };
> union acpi_object in_buf = {
> - .type = ACPI_TYPE_BUFFER,
> - .buffer.pointer = (char *) &smart,
> - .buffer.length = sizeof(smart),
> + .buffer.type = ACPI_TYPE_BUFFER,
> + .buffer.length = 0,
> };
> union acpi_object in_obj = {
> - .type = ACPI_TYPE_PACKAGE,
> + .package.type = ACPI_TYPE_PACKAGE,
> .package.count = 1,
> .package.elements = &in_buf,
> };
> @@ -1781,8 +1781,15 @@ __weak void nfit_intel_shutdown_status(struct
> nfit_mem *nfit_mem)
> return;
>
> out_obj = acpi_evaluate_dsm(handle, guid, revid, func, &in_obj);
> - if (!out_obj)
> + if (!out_obj || out_obj->type != ACPI_TYPE_BUFFER
> + || out_obj->buffer.length < sizeof(smart)) {
> + dev_dbg(dev->parent, "%s: failed to retrieve initial health\n",
> + dev_name(dev));
> + ACPI_FREE(out_obj);
> return;
> + }
> + memcpy(&smart, out_obj->buffer.pointer, sizeof(smart));
> + ACPI_FREE(out_obj);
>
> if (smart.flags & ND_INTEL_SMART_SHUTDOWN_VALID) {
> if (smart.shutdown_state)
> @@ -1793,7 +1800,6 @@ __weak void nfit_intel_shutdown_status(struct
> nfit_mem *nfit_mem)
> set_bit(NFIT_MEM_DIRTY_COUNT, &nfit_mem->flags);
> nfit_mem->dirty_shutdown = smart.shutdown_count;
> }
> - ACPI_FREE(out_obj);
> }
>
> static void populate_shutdown_status(struct nfit_mem *nfit_mem)
> @@ -1915,18 +1921,19 @@ static int acpi_nfit_add_dimm(struct
> acpi_nfit_desc *acpi_desc,
> | 1 << ND_CMD_SET_CONFIG_DATA;
> if (family == NVDIMM_FAMILY_INTEL
> && (dsm_mask & label_mask) == label_mask)
> - return 0;
> -
> - if (acpi_nvdimm_has_method(adev_dimm, "_LSI")
> - && acpi_nvdimm_has_method(adev_dimm, "_LSR")) {
> - dev_dbg(dev, "%s: has _LSR\n", dev_name(&adev_dimm->dev));
> - set_bit(NFIT_MEM_LSR, &nfit_mem->flags);
> - }
> + /* skip _LS{I,R,W} enabling */;
> + else {
> + if (acpi_nvdimm_has_method(adev_dimm, "_LSI")
> + && acpi_nvdimm_has_method(adev_dimm, "_LSR")) {
> + dev_dbg(dev, "%s: has _LSR\n", dev_name(&adev_dimm->dev));
> + set_bit(NFIT_MEM_LSR, &nfit_mem->flags);
> + }
>
> - if (test_bit(NFIT_MEM_LSR, &nfit_mem->flags)
> - && acpi_nvdimm_has_method(adev_dimm, "_LSW")) {
> - dev_dbg(dev, "%s: has _LSW\n", dev_name(&adev_dimm->dev));
> - set_bit(NFIT_MEM_LSW, &nfit_mem->flags);
> + if (test_bit(NFIT_MEM_LSR, &nfit_mem->flags)
> + && acpi_nvdimm_has_method(adev_dimm, "_LSW")) {
> + dev_dbg(dev, "%s: has _LSW\n", dev_name(&adev_dimm->dev));
> + set_bit(NFIT_MEM_LSW, &nfit_mem->flags);
> + }
> }
>
> populate_shutdown_status(nfit_mem);
Reviewed-by: Dexuan Cui <[email protected]>
Thanks for the fix, Dan!
-- Dexuan