acpi/nfit: Fix command-supported detection

Commit Message

Dan Williams Jan. 11, 2019, 11:59 p.m. UTC

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: <[e-mail]>
Link: https://github.com/pmem/ndctl/issues/78
Reported-by: Sujith Pandel <[e-mail]>
Signed-off-by: Dan Williams <[e-mail]>
---
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(-)

Comments

Verma, Vishal L Jan. 12, 2019, 1:33 a.m. UTC | #1

On Fri, 2019-01-11 at 15:59 -0800, Dan Williams wrote:
> 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: <[e-mail]>
> Link: https://github.com/pmem/ndctl/issues/78
> Reported-by: Sujith Pandel <[e-mail]>
> Signed-off-by: Dan Williams <[e-mail]>
> ---
> 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(-)

Looks good to me || good find ;)
Reviewed-by: Vishal Verma <[e-mail]>

> 
> 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);
> +
> +	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;
> +}
> +
>  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 +445,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(nvdimm, 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 +470,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 +485,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;
>

Pandel, Sujith Jan. 14, 2019, 7:21 a.m. UTC | #2

Hi Dan,

On 12-Jan-19 5:29 AM, Dan Williams wrote:
> 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: <[e-mail]>
> Link: https://github.com/pmem/ndctl/issues/78
> Reported-by: Sujith Pandel <[e-mail]>
> Signed-off-by: Dan Williams <[e-mail]>
> ---
> 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!
Re-verified by applying this patch over LTS kernel-4.20.2 with ndctl v62.
namespace creation and modification works fine on Dell NVDIMM-Ns.
Thanks for helping me on this!

Tested-by: Sujith Pandel <[e-mail]>

Jeff Moyer Jan. 14, 2019, 3:19 p.m. UTC | #3

Dan Williams <[e-mail]> 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: <[e-mail]>
> Link: https://github.com/pmem/ndctl/issues/78
> Reported-by: Sujith Pandel <[e-mail]>
> Signed-off-by: Dan Williams <[e-mail]>
> ---
> 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

Dan Williams Jan. 14, 2019, 4:43 p.m. UTC | #4

On Mon, Jan 14, 2019 at 7:19 AM Jeff Moyer <[e-mail]> wrote:
>
> Dan Williams <[e-mail]> 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: <[e-mail]>
> > Link: https://github.com/pmem/ndctl/issues/78
> > Reported-by: Sujith Pandel <[e-mail]>
> > Signed-off-by: Dan Williams <[e-mail]>
> > ---
> > 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

Dan Williams Jan. 14, 2019, 4:47 p.m. UTC | #5

On Mon, Jan 14, 2019 at 8:43 AM Dan Williams <[e-mail]> wrote:
> On Mon, Jan 14, 2019 at 7:19 AM Jeff Moyer <[e-mail]> 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.

Jeff Moyer Jan. 14, 2019, 8:51 p.m. UTC | #6

Dan Williams <[e-mail]> writes:

> On Mon, Jan 14, 2019 at 8:43 AM Dan Williams <[e-mail]> wrote:
>> On Mon, Jan 14, 2019 at 7:19 AM Jeff Moyer <[e-mail]> 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

Sasha Levin Jan. 16, 2019, 1:35 p.m. UTC | #7

Hi,

[This is an automated email]

This commit has been processed because it contains a "Fixes:" tag,
fixing commit: 31eca76ba2fc nfit, libnvdimm: limited/whitelisted dimm command marshaling mechanism.

The bot has tested the following trees: v4.20.2, v4.19.15, v4.14.93, v4.9.150.

v4.20.2: Failed to apply! Possible dependencies:
    b3ed2ce024c3 ("acpi/nfit: Add support for Intel DSM 1.8 commands")

v4.19.15: Failed to apply! Possible dependencies:
    0ead11181fe0 ("acpi, nfit: Collect shutdown status")
    6f07f86c4940 ("acpi, nfit: Introduce nfit_mem flags")
    b3ed2ce024c3 ("acpi/nfit: Add support for Intel DSM 1.8 commands")

v4.14.93: Build OK!
v4.9.150: Failed to apply! Possible dependencies:
    0f817ae696b0 ("usb: dwc3: pci: add a private driver structure")
    11e142701609 ("acpi, nfit: add support for NVDIMM_FAMILY_INTEL v1.6 DSMs")
    23222f8f8dce ("acpi, nfit: Add function to look up nvdimm device and provide SMBIOS handle")
    36daf3aa399c ("usb: dwc3: pci: avoid build warning")
    3f23df72dc35 ("mmc: sdhci-pci: Use ACPI to get max frequency for Intel NI byt sdio")
    41c8bdb3ab10 ("acpi, nfit: Switch to use new generic UUID API")
    42237e393f64 ("libnvdimm: allow a platform to force enable label support")
    42b06496407c ("mmc: sdhci-pci: Add PCI ID for Intel NI byt sdio")
    4b27db7e26cd ("acpi, nfit: add support for the _LSI, _LSR, and _LSW label methods")
    8f078b38dd38 ("libnvdimm: convert NDD_ flags to use bitops, introduce NDD_LOCKED")
    94116f8126de ("ACPI: Switch to use generic guid_t in acpi_evaluate_dsm()")
    9cecca75b5a0 ("usb: dwc3: pci: call _DSM for suspend/resume")
    9d62ed965118 ("libnvdimm: handle locked label storage areas")
    b3ed2ce024c3 ("acpi/nfit: Add support for Intel DSM 1.8 commands")
    b7fe92999a98 ("ACPI / extlog: Switch to use new generic UUID API")
    b917078c1c10 ("net: hns: Add ACPI support to check SFP present")
    c959a6b00ff5 ("mmc: sdhci-pci: Don't re-tune with runtime pm for some Intel devices")
    d2061f9cc32d ("usb: typec: add driver for Intel Whiskey Cove PMIC USB Type-C PHY")
    fab9288428ec ("usb: USB Type-C connector class")


How should we proceed with this patch?

--
Thanks,
Sasha

Patch

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);
+
+	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;
+}
+
 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 +445,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(nvdimm, 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 +470,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 +485,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;