Enable DSM pass thru for root functions

Add a bus level dsm_mask to nvdimm_bus_descriptor to allow the passthru calling mechanism to specify a different mask from the cmd_mask.

Populate bus_dsm_mask and use it to filter dsm calls that user can make through the pass thru interface.

Signed-off-by Jerry Hoemann <[email protected]>

---
 drivers/acpi/nfit/core.c  | 8 ++++++++
 include/linux/libnvdimm.h | 1 +
 2 files changed, 9 insertions(+)

drivers/acpi/nfit/core.c +3 -0

index b46fca2..5e4c137 100644
@@ -253,6 +253,8 @@ int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm,
                cmd_name = nvdimm_bus_cmd_name(cmd);
                cmd_mask = nd_desc->cmd_mask;
                dsm_mask = cmd_mask;
+               if (cmd == ND_CMD_CALL)
+                       dsm_mask = nd_desc->bus_dsm_mask;
                desc = nd_cmd_bus_desc(cmd);
                uuid = to_nfit_uuid(NFIT_DEV_BUS);
                handle = adev->handle;

@@ -1613,6 +1615,7 @@ static void acpi_nfit_init_dsms(struct acpi_nfit_desc *acpi_desc)
        struct nvdimm_bus_descriptor *nd_desc = &acpi_desc->nd_desc;
        const u8 *uuid = to_nfit_uuid(NFIT_DEV_BUS);
        struct acpi_device *adev;
+       unsigned long dsm_mask;
        int i;
 
        nd_desc->cmd_mask = acpi_desc->bus_cmd_force_en;

Quoted 14 lines

drivers/acpi/nfit/core.c +3 -0

+               if (cmd == ND_CMD_CALL)
+                       dsm_mask = nd_desc->bus_dsm_mask;
                desc = nd_cmd_bus_desc(cmd);
                uuid = to_nfit_uuid(NFIT_DEV_BUS);
                handle = adev->handle;

@@ -1613,6 +1615,7 @@ static void acpi_nfit_init_dsms(struct acpi_nfit_desc *acpi_desc)
        struct nvdimm_bus_descriptor *nd_desc = &acpi_desc->nd_desc;
        const u8 *uuid = to_nfit_uuid(NFIT_DEV_BUS);
        struct acpi_device *adev;
+       unsigned long dsm_mask;
        int i;
 
        nd_desc->cmd_mask = acpi_desc->bus_cmd_force_en;

drivers/acpi/nfit/core.c static void acpi_nfit_init_dsms(struct acpi_nfit_desc *acpi_desc) · line 1631

Quoted 5 lines

drivers/acpi/nfit/core.c +2 -0

@@ -1624,6 +1627,11 @@ static void acpi_nfit_init_dsms(struct acpi_nfit_desc *acpi_desc)
                if (acpi_check_dsm(adev->handle, uuid, 1, 1ULL << i))
                        set_bit(i, &nd_desc->cmd_mask);
        set_bit(ND_CMD_CALL, &nd_desc->cmd_mask);
+
+       dsm_mask = 0x3bf;

I went ahead and fixed this up to use dsm_mask defined like this:

(quoted diff) +9 -0

+       dsm_mask =
+               (1 << ND_CMD_ARS_CAP) |
+               (1 << ND_CMD_ARS_START) |
+               (1 << ND_CMD_ARS_STATUS) |
+               (1 << ND_CMD_CLEAR_ERROR) |
+               (1 << NFIT_CMD_TRANSLATE_SPA) |
+               (1 << NFIT_CMD_ARS_INJECT_SET) |
+               (1 << NFIT_CMD_ARS_INJECT_CLEAR) |
+               (1 << NFIT_CMD_ARS_INJECT_GET);

This drops function number 0 which userspace has no need to call.

On Fri, Jun 30, 2017 at 08:55:22PM -0700, Dan Williams wrote:

...

Quoted 33 lines · Jerry Hoemann

Quoted 14 lines

drivers/acpi/nfit/core.c +3 -0

+               if (cmd == ND_CMD_CALL)
+                       dsm_mask = nd_desc->bus_dsm_mask;
                desc = nd_cmd_bus_desc(cmd);
                uuid = to_nfit_uuid(NFIT_DEV_BUS);
                handle = adev->handle;

@@ -1613,6 +1615,7 @@ static void acpi_nfit_init_dsms(struct acpi_nfit_desc *acpi_desc)
        struct nvdimm_bus_descriptor *nd_desc = &acpi_desc->nd_desc;
        const u8 *uuid = to_nfit_uuid(NFIT_DEV_BUS);
        struct acpi_device *adev;
+       unsigned long dsm_mask;
        int i;
 
        nd_desc->cmd_mask = acpi_desc->bus_cmd_force_en;

drivers/acpi/nfit/core.c static void acpi_nfit_init_dsms(struct acpi_nfit_desc *acpi_desc) · line 1631

Quoted 5 lines

drivers/acpi/nfit/core.c +2 -0

@@ -1624,6 +1627,11 @@ static void acpi_nfit_init_dsms(struct acpi_nfit_desc *acpi_desc)
                if (acpi_check_dsm(adev->handle, uuid, 1, 1ULL << i))
                        set_bit(i, &nd_desc->cmd_mask);
        set_bit(ND_CMD_CALL, &nd_desc->cmd_mask);
+
+       dsm_mask = 0x3bf;

I went ahead and fixed this up to use dsm_mask defined like this:

(quoted diff) +9 -0

+       dsm_mask =
+               (1 << ND_CMD_ARS_CAP) |
+               (1 << ND_CMD_ARS_START) |
+               (1 << ND_CMD_ARS_STATUS) |
+               (1 << ND_CMD_CLEAR_ERROR) |
+               (1 << NFIT_CMD_TRANSLATE_SPA) |
+               (1 << NFIT_CMD_ARS_INJECT_SET) |
+               (1 << NFIT_CMD_ARS_INJECT_CLEAR) |
+               (1 << NFIT_CMD_ARS_INJECT_GET);

Quoted 1 line

This drops function number 0 which userspace has no need to call.

Actually I like to call function 0. Its an excellent test when modifying the code path as its a no side effects function whose output is known in advance and instantly recognizable. I also use it when testing new firmware.

What is the downside to allowing it? What bad things happen?

Also, I do have to ask why you allow function zero for NVDIMM_FAMILY_MSFT?

Quoted 44 lines · Jerry Hoemann

On Fri, Jun 30, 2017 at 08:55:22PM -0700, Dan Williams wrote:

...

Quoted 33 lines · Jerry Hoemann

Quoted 14 lines

drivers/acpi/nfit/core.c +3 -0

+               if (cmd == ND_CMD_CALL)
+                       dsm_mask = nd_desc->bus_dsm_mask;
                desc = nd_cmd_bus_desc(cmd);
                uuid = to_nfit_uuid(NFIT_DEV_BUS);
                handle = adev->handle;

@@ -1613,6 +1615,7 @@ static void acpi_nfit_init_dsms(struct acpi_nfit_desc *acpi_desc)
        struct nvdimm_bus_descriptor *nd_desc = &acpi_desc->nd_desc;
        const u8 *uuid = to_nfit_uuid(NFIT_DEV_BUS);
        struct acpi_device *adev;
+       unsigned long dsm_mask;
        int i;
 
        nd_desc->cmd_mask = acpi_desc->bus_cmd_force_en;

drivers/acpi/nfit/core.c static void acpi_nfit_init_dsms(struct acpi_nfit_desc *acpi_desc) · line 1631

Quoted 5 lines

drivers/acpi/nfit/core.c +2 -0

@@ -1624,6 +1627,11 @@ static void acpi_nfit_init_dsms(struct acpi_nfit_desc *acpi_desc)
                if (acpi_check_dsm(adev->handle, uuid, 1, 1ULL << i))
                        set_bit(i, &nd_desc->cmd_mask);
        set_bit(ND_CMD_CALL, &nd_desc->cmd_mask);
+
+       dsm_mask = 0x3bf;

I went ahead and fixed this up to use dsm_mask defined like this:

(quoted diff) +9 -0

+       dsm_mask =
+               (1 << ND_CMD_ARS_CAP) |
+               (1 << ND_CMD_ARS_START) |
+               (1 << ND_CMD_ARS_STATUS) |
+               (1 << ND_CMD_CLEAR_ERROR) |
+               (1 << NFIT_CMD_TRANSLATE_SPA) |
+               (1 << NFIT_CMD_ARS_INJECT_SET) |
+               (1 << NFIT_CMD_ARS_INJECT_CLEAR) |
+               (1 << NFIT_CMD_ARS_INJECT_GET);

Quoted 1 line

This drops function number 0 which userspace has no need to call.

Actually I like to call function 0. Its an excellent test when modifying the code path as its a no side effects function whose output is known in advance and instantly recognizable. I also use it when testing new firmware.

Quoted 1 line

What is the downside to allowing it? What bad things happen?

It allows implementations to bypass the standardization process and ship new root DSMs. It's always possible to patch the kernel locally for development, so I see no reason to ship this capability globally.

Quoted 45 lines · Dan Williams

Quoted 44 lines · Jerry Hoemann

On Fri, Jun 30, 2017 at 08:55:22PM -0700, Dan Williams wrote:

...

Quoted 33 lines · Jerry Hoemann

Quoted 14 lines

drivers/acpi/nfit/core.c +3 -0

+               if (cmd == ND_CMD_CALL)
+                       dsm_mask = nd_desc->bus_dsm_mask;
                desc = nd_cmd_bus_desc(cmd);
                uuid = to_nfit_uuid(NFIT_DEV_BUS);
                handle = adev->handle;

@@ -1613,6 +1615,7 @@ static void acpi_nfit_init_dsms(struct acpi_nfit_desc *acpi_desc)
        struct nvdimm_bus_descriptor *nd_desc = &acpi_desc->nd_desc;
        const u8 *uuid = to_nfit_uuid(NFIT_DEV_BUS);
        struct acpi_device *adev;
+       unsigned long dsm_mask;
        int i;
 
        nd_desc->cmd_mask = acpi_desc->bus_cmd_force_en;

drivers/acpi/nfit/core.c static void acpi_nfit_init_dsms(struct acpi_nfit_desc *acpi_desc) · line 1631

Quoted 5 lines

drivers/acpi/nfit/core.c +2 -0

@@ -1624,6 +1627,11 @@ static void acpi_nfit_init_dsms(struct acpi_nfit_desc *acpi_desc)
                if (acpi_check_dsm(adev->handle, uuid, 1, 1ULL << i))
                        set_bit(i, &nd_desc->cmd_mask);
        set_bit(ND_CMD_CALL, &nd_desc->cmd_mask);
+
+       dsm_mask = 0x3bf;

I went ahead and fixed this up to use dsm_mask defined like this:

(quoted diff) +9 -0

+       dsm_mask =
+               (1 << ND_CMD_ARS_CAP) |
+               (1 << ND_CMD_ARS_START) |
+               (1 << ND_CMD_ARS_STATUS) |
+               (1 << ND_CMD_CLEAR_ERROR) |
+               (1 << NFIT_CMD_TRANSLATE_SPA) |
+               (1 << NFIT_CMD_ARS_INJECT_SET) |
+               (1 << NFIT_CMD_ARS_INJECT_CLEAR) |
+               (1 << NFIT_CMD_ARS_INJECT_GET);

Quoted 1 line

This drops function number 0 which userspace has no need to call.

Actually I like to call function 0. Its an excellent test when modifying the code path as its a no side effects function whose output is known in advance and instantly recognizable. I also use it when testing new firmware.

Quoted 5 lines

Quoted 1 line

What is the downside to allowing it? What bad things happen?

It allows implementations to bypass the standardization process and ship new root DSMs. It's always possible to patch the kernel locally for development, so I see no reason to ship this capability globally.

Actually, just the discovery portion does not lead to this leak, but it's redundant when we have the 'dsm_mask' sysfs attribute.

Quoted 46 lines · Dan Williams

Quoted 45 lines · Dan Williams

Quoted 44 lines · Jerry Hoemann

On Fri, Jun 30, 2017 at 08:55:22PM -0700, Dan Williams wrote:

...

Quoted 33 lines · Jerry Hoemann

Quoted 14 lines

drivers/acpi/nfit/core.c +3 -0

+               if (cmd == ND_CMD_CALL)
+                       dsm_mask = nd_desc->bus_dsm_mask;
                desc = nd_cmd_bus_desc(cmd);
                uuid = to_nfit_uuid(NFIT_DEV_BUS);
                handle = adev->handle;

@@ -1613,6 +1615,7 @@ static void acpi_nfit_init_dsms(struct acpi_nfit_desc *acpi_desc)
        struct nvdimm_bus_descriptor *nd_desc = &acpi_desc->nd_desc;
        const u8 *uuid = to_nfit_uuid(NFIT_DEV_BUS);
        struct acpi_device *adev;
+       unsigned long dsm_mask;
        int i;
 
        nd_desc->cmd_mask = acpi_desc->bus_cmd_force_en;

drivers/acpi/nfit/core.c static void acpi_nfit_init_dsms(struct acpi_nfit_desc *acpi_desc) · line 1631

Quoted 5 lines

drivers/acpi/nfit/core.c +2 -0

@@ -1624,6 +1627,11 @@ static void acpi_nfit_init_dsms(struct acpi_nfit_desc *acpi_desc)
                if (acpi_check_dsm(adev->handle, uuid, 1, 1ULL << i))
                        set_bit(i, &nd_desc->cmd_mask);
        set_bit(ND_CMD_CALL, &nd_desc->cmd_mask);
+
+       dsm_mask = 0x3bf;

I went ahead and fixed this up to use dsm_mask defined like this:

(quoted diff) +9 -0

+       dsm_mask =
+               (1 << ND_CMD_ARS_CAP) |
+               (1 << ND_CMD_ARS_START) |
+               (1 << ND_CMD_ARS_STATUS) |
+               (1 << ND_CMD_CLEAR_ERROR) |
+               (1 << NFIT_CMD_TRANSLATE_SPA) |
+               (1 << NFIT_CMD_ARS_INJECT_SET) |
+               (1 << NFIT_CMD_ARS_INJECT_CLEAR) |
+               (1 << NFIT_CMD_ARS_INJECT_GET);

Quoted 1 line

This drops function number 0 which userspace has no need to call.

Actually I like to call function 0. Its an excellent test when modifying the code path as its a no side effects function whose output is known in advance and instantly recognizable. I also use it when testing new firmware.

Quoted 5 lines

Quoted 5 lines

Quoted 1 line

What is the downside to allowing it? What bad things happen?

It allows implementations to bypass the standardization process and ship new root DSMs. It's always possible to patch the kernel locally for development, so I see no reason to ship this capability globally.

I don't understand this comment, but I think your next comment essentially says to disregard this comment?

Quoted 2 lines

Actually, just the discovery portion does not lead to this leak, but it's redundant when we have the 'dsm_mask' sysfs attribute.

No. The generation of the mask in sysfs is not done by executing the code in acpi_nfit_ctl. One of the reasons I call function 0 to test changes I am making to the ioctl path itself. The sysfs has nothing to do with that path and cannot be used to serve this purpose.

And since the content of sysfs has been edited it also can not be used as a basic test of firmware.

What is the downside to allowing the calling of function 0?

Quoted 47 lines · Jerry Hoemann

Quoted 46 lines · Dan Williams

Quoted 45 lines · Dan Williams

Quoted 44 lines · Jerry Hoemann

On Fri, Jun 30, 2017 at 08:55:22PM -0700, Dan Williams wrote:

...

Quoted 33 lines · Jerry Hoemann

Quoted 14 lines

(quoted diff) +3 -0

+               if (cmd == ND_CMD_CALL)
+                       dsm_mask = nd_desc->bus_dsm_mask;
                desc = nd_cmd_bus_desc(cmd);
                uuid = to_nfit_uuid(NFIT_DEV_BUS);
                handle = adev->handle;

@@ -1613,6 +1615,7 @@ static void acpi_nfit_init_dsms(struct acpi_nfit_desc *acpi_desc)
        struct nvdimm_bus_descriptor *nd_desc = &acpi_desc->nd_desc;
        const u8 *uuid = to_nfit_uuid(NFIT_DEV_BUS);
        struct acpi_device *adev;
+       unsigned long dsm_mask;
        int i;
 
        nd_desc->cmd_mask = acpi_desc->bus_cmd_force_en;

Review static void acpi_nfit_init_dsms(struct acpi_nfit_desc *acpi_desc) · line 1631

Quoted 5 lines

(quoted diff) +2 -0

@@ -1624,6 +1627,11 @@ static void acpi_nfit_init_dsms(struct acpi_nfit_desc *acpi_desc)
                if (acpi_check_dsm(adev->handle, uuid, 1, 1ULL << i))
                        set_bit(i, &nd_desc->cmd_mask);
        set_bit(ND_CMD_CALL, &nd_desc->cmd_mask);
+
+       dsm_mask = 0x3bf;

I went ahead and fixed this up to use dsm_mask defined like this:

(quoted diff) +9 -0

+       dsm_mask =
+               (1 << ND_CMD_ARS_CAP) |
+               (1 << ND_CMD_ARS_START) |
+               (1 << ND_CMD_ARS_STATUS) |
+               (1 << ND_CMD_CLEAR_ERROR) |
+               (1 << NFIT_CMD_TRANSLATE_SPA) |
+               (1 << NFIT_CMD_ARS_INJECT_SET) |
+               (1 << NFIT_CMD_ARS_INJECT_CLEAR) |
+               (1 << NFIT_CMD_ARS_INJECT_GET);

Quoted 1 line

This drops function number 0 which userspace has no need to call.

Actually I like to call function 0. Its an excellent test when modifying the code path as its a no side effects function whose output is known in advance and instantly recognizable. I also use it when testing new firmware.

Quoted 8 lines

Quoted 5 lines

Quoted 5 lines

Quoted 1 line

What is the downside to allowing it? What bad things happen?

It allows implementations to bypass the standardization process and ship new root DSMs. It's always possible to patch the kernel locally for development, so I see no reason to ship this capability globally.

I don't understand this comment, but I think your next comment essentially says to disregard this comment?

Yes, sorry.

Quoted 9 lines

Quoted 2 lines

Actually, just the discovery portion does not lead to this leak, but it's redundant when we have the 'dsm_mask' sysfs attribute.

No. The generation of the mask in sysfs is not done by executing the code in acpi_nfit_ctl. One of the reasons I call function 0 to test changes I am making to the ioctl path itself. The sysfs has nothing to do with that path and cannot be used to serve this purpose.

Quoted 4 lines

And since the content of sysfs has been edited it also can not be used as a basic test of firmware.

What is the downside to allowing the calling of function 0?

It needlessly expands the kernel ABI. I would suggest, if you want to

Quoted 71 lines · Dan Williams

Quoted 47 lines · Jerry Hoemann

Quoted 46 lines · Dan Williams

Quoted 45 lines · Dan Williams

Quoted 44 lines · Jerry Hoemann

On Fri, Jun 30, 2017 at 08:55:22PM -0700, Dan Williams wrote:

...

Quoted 33 lines · Jerry Hoemann

Quoted 14 lines

(quoted diff) +3 -0

+               if (cmd == ND_CMD_CALL)
+                       dsm_mask = nd_desc->bus_dsm_mask;
                desc = nd_cmd_bus_desc(cmd);
                uuid = to_nfit_uuid(NFIT_DEV_BUS);
                handle = adev->handle;

@@ -1613,6 +1615,7 @@ static void acpi_nfit_init_dsms(struct acpi_nfit_desc *acpi_desc)
        struct nvdimm_bus_descriptor *nd_desc = &acpi_desc->nd_desc;
        const u8 *uuid = to_nfit_uuid(NFIT_DEV_BUS);
        struct acpi_device *adev;
+       unsigned long dsm_mask;
        int i;
 
        nd_desc->cmd_mask = acpi_desc->bus_cmd_force_en;

Review static void acpi_nfit_init_dsms(struct acpi_nfit_desc *acpi_desc) · line 1631

Quoted 5 lines

(quoted diff) +2 -0

@@ -1624,6 +1627,11 @@ static void acpi_nfit_init_dsms(struct acpi_nfit_desc *acpi_desc)
                if (acpi_check_dsm(adev->handle, uuid, 1, 1ULL << i))
                        set_bit(i, &nd_desc->cmd_mask);
        set_bit(ND_CMD_CALL, &nd_desc->cmd_mask);
+
+       dsm_mask = 0x3bf;

I went ahead and fixed this up to use dsm_mask defined like this:

(quoted diff) +9 -0

+       dsm_mask =
+               (1 << ND_CMD_ARS_CAP) |
+               (1 << ND_CMD_ARS_START) |
+               (1 << ND_CMD_ARS_STATUS) |
+               (1 << ND_CMD_CLEAR_ERROR) |
+               (1 << NFIT_CMD_TRANSLATE_SPA) |
+               (1 << NFIT_CMD_ARS_INJECT_SET) |
+               (1 << NFIT_CMD_ARS_INJECT_CLEAR) |
+               (1 << NFIT_CMD_ARS_INJECT_GET);

Quoted 1 line

This drops function number 0 which userspace has no need to call.

Actually I like to call function 0. Its an excellent test when modifying the code path as its a no side effects function whose output is known in advance and instantly recognizable. I also use it when testing new firmware.

Quoted 8 lines

Quoted 5 lines

Quoted 5 lines

Quoted 1 line

What is the downside to allowing it? What bad things happen?

It allows implementations to bypass the standardization process and ship new root DSMs. It's always possible to patch the kernel locally for development, so I see no reason to ship this capability globally.

I don't understand this comment, but I think your next comment essentially says to disregard this comment?

Yes, sorry.

Quoted 12 lines

Quoted 2 lines

Actually, just the discovery portion does not lead to this leak, but it's redundant when we have the 'dsm_mask' sysfs attribute.

No. The generation of the mask in sysfs is not done by executing the code in acpi_nfit_ctl. One of the reasons I call function 0 to test changes I am making to the ioctl path itself. The sysfs has nothing to do with that path and cannot be used to serve this purpose.

And since the content of sysfs has been edited it also can not be used as a basic test of firmware.

Quoted 3 lines

Quoted 1 line

What is the downside to allowing the calling of function 0?

It needlessly expands the kernel ABI. I would suggest, if you want to

No. It is not needless. It is not an ABI extension. Same goes for the override feature.

I hope that ACPI doesn't extend the specification in the future because we'll just have to redo these patches yet again.

Quoted 72 lines · Jerry Hoemann

Quoted 71 lines · Dan Williams

Quoted 47 lines · Jerry Hoemann

Quoted 46 lines · Dan Williams

Quoted 45 lines · Dan Williams

Quoted 44 lines · Jerry Hoemann

On Fri, Jun 30, 2017 at 08:55:22PM -0700, Dan Williams wrote:

...

Quoted 33 lines · Jerry Hoemann

Quoted 14 lines

(quoted diff) +3 -0

+               if (cmd == ND_CMD_CALL)
+                       dsm_mask = nd_desc->bus_dsm_mask;
                desc = nd_cmd_bus_desc(cmd);
                uuid = to_nfit_uuid(NFIT_DEV_BUS);
                handle = adev->handle;

@@ -1613,6 +1615,7 @@ static void acpi_nfit_init_dsms(struct acpi_nfit_desc *acpi_desc)
        struct nvdimm_bus_descriptor *nd_desc = &acpi_desc->nd_desc;
        const u8 *uuid = to_nfit_uuid(NFIT_DEV_BUS);
        struct acpi_device *adev;
+       unsigned long dsm_mask;
        int i;
 
        nd_desc->cmd_mask = acpi_desc->bus_cmd_force_en;

Review static void acpi_nfit_init_dsms(struct acpi_nfit_desc *acpi_desc) · line 1631

Quoted 5 lines

(quoted diff) +2 -0

@@ -1624,6 +1627,11 @@ static void acpi_nfit_init_dsms(struct acpi_nfit_desc *acpi_desc)
                if (acpi_check_dsm(adev->handle, uuid, 1, 1ULL << i))
                        set_bit(i, &nd_desc->cmd_mask);
        set_bit(ND_CMD_CALL, &nd_desc->cmd_mask);
+
+       dsm_mask = 0x3bf;

I went ahead and fixed this up to use dsm_mask defined like this:

(quoted diff) +9 -0

+       dsm_mask =
+               (1 << ND_CMD_ARS_CAP) |
+               (1 << ND_CMD_ARS_START) |
+               (1 << ND_CMD_ARS_STATUS) |
+               (1 << ND_CMD_CLEAR_ERROR) |
+               (1 << NFIT_CMD_TRANSLATE_SPA) |
+               (1 << NFIT_CMD_ARS_INJECT_SET) |
+               (1 << NFIT_CMD_ARS_INJECT_CLEAR) |
+               (1 << NFIT_CMD_ARS_INJECT_GET);

Quoted 1 line

This drops function number 0 which userspace has no need to call.

Actually I like to call function 0. Its an excellent test when modifying the code path as its a no side effects function whose output is known in advance and instantly recognizable. I also use it when testing new firmware.

Quoted 8 lines

Quoted 5 lines

Quoted 5 lines

Quoted 1 line

What is the downside to allowing it? What bad things happen?

It allows implementations to bypass the standardization process and ship new root DSMs. It's always possible to patch the kernel locally for development, so I see no reason to ship this capability globally.

I don't understand this comment, but I think your next comment essentially says to disregard this comment?

Yes, sorry.

Quoted 12 lines

Quoted 2 lines

Actually, just the discovery portion does not lead to this leak, but it's redundant when we have the 'dsm_mask' sysfs attribute.

No. The generation of the mask in sysfs is not done by executing the code in acpi_nfit_ctl. One of the reasons I call function 0 to test changes I am making to the ioctl path itself. The sysfs has nothing to do with that path and cannot be used to serve this purpose.

And since the content of sysfs has been edited it also can not be used as a basic test of firmware.

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

What is the downside to allowing the calling of function 0?

It needlessly expands the kernel ABI. I would suggest, if you want to

No. It is not needless. It is not an ABI extension. Same goes for the override feature.

If the need is testing then we have a tools/testing/nvdimm for that. Of course it's an ABI extension, it allows userspace to discover DSM function numbers the kernel didn't know about at compile time.

Quoted 2 lines

I hope that ACPI doesn't extend the specification in the future because we'll just have to redo these patches yet again.

Hopefully this is the last ACPI spec version where we add new DSMs to the root device.

All future methods should be named methods like what the specification started doing for NVIDMM leaf devices with _LSI, _LSR, and _LSW.

_______________________________________________
Linux-nvdimm mailing list
[email protected]
https://lists.01.org/mailman/listinfo/linux-nvdimm

Quoted 8 lines · Jerry Hoemann

Quoted 7 lines · Dan Williams

Quoted 6 lines · Jerry Hoemann

Quoted 5 lines · Dan Williams

Quoted 4 lines · Dan Williams

Quoted 3 lines · Jerry Hoemann

On Fri, Jun 30, 2017 at 08:55:22PM -0700, Dan Williams wrote:

...

Quoted 31 lines

Quoted 31 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 2 lines

This drops function number 0 which userspace has no need to call.

Actually I like to call function 0. Its an excellent test when modifying the code path as its a no side effects function whose output is known in advance and instantly recognizable. I also use it when testing new firmware.

Quoted 8 lines

Quoted 5 lines

Quoted 5 lines

Quoted 1 line

What is the downside to allowing it? What bad things happen?

It allows implementations to bypass the standardization process and ship new root DSMs. It's always possible to patch the kernel locally for development, so I see no reason to ship this capability globally.

I don't understand this comment, but I think your next comment essentially says to disregard this comment?

Yes, sorry.

Quoted 12 lines

Quoted 2 lines

Actually, just the discovery portion does not lead to this leak, but it's redundant when we have the 'dsm_mask' sysfs attribute.

No. The generation of the mask in sysfs is not done by executing the code in acpi_nfit_ctl. One of the reasons I call function 0 to test changes I am making to the ioctl path itself. The sysfs has nothing to do with that path and cannot be used to serve this purpose.

And since the content of sysfs has been edited it also can not be used as a basic test of firmware.

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

What is the downside to allowing the calling of function 0?

It needlessly expands the kernel ABI. I would suggest, if you want to

No. It is not needless. It is not an ABI extension. Same goes for the override feature.

If the need is testing then we have a tools/testing/nvdimm for that.

Of course it's an ABI extension, it allows userspace to discover DSM function numbers the kernel didn't know about at compile time.

Quoted 9 lines · Dan Williams

Quoted 8 lines · Jerry Hoemann

Quoted 7 lines · Dan Williams

Quoted 6 lines · Jerry Hoemann

Quoted 5 lines · Dan Williams

Quoted 4 lines · Dan Williams

Quoted 3 lines · Jerry Hoemann

On Fri, Jun 30, 2017 at 08:55:22PM -0700, Dan Williams wrote:

...

...

Quoted 38 lines

Quoted 31 lines

Quoted 31 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 2 lines

This drops function number 0 which userspace has no need to call.

Actually I like to call function 0. Its an excellent test when modifying the code path as its a no side effects function whose output is known in advance and instantly recognizable. I also use it when testing new firmware.

Quoted 8 lines

Quoted 5 lines

Quoted 5 lines

Quoted 1 line

What is the downside to allowing it? What bad things happen?

It allows implementations to bypass the standardization process and ship new root DSMs. It's always possible to patch the kernel locally for development, so I see no reason to ship this capability globally.

I don't understand this comment, but I think your next comment essentially says to disregard this comment?

Yes, sorry.

Quoted 12 lines

Quoted 2 lines

Actually, just the discovery portion does not lead to this leak, but it's redundant when we have the 'dsm_mask' sysfs attribute.

No. The generation of the mask in sysfs is not done by executing the code in acpi_nfit_ctl. One of the reasons I call function 0 to test changes I am making to the ioctl path itself. The sysfs has nothing to do with that path and cannot be used to serve this purpose.

And since the content of sysfs has been edited it also can not be used as a basic test of firmware.

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

What is the downside to allowing the calling of function 0?

It needlessly expands the kernel ABI. I would suggest, if you want to

No. It is not needless. It is not an ABI extension. Same goes for the override feature.

Quoted 6 lines

If the need is testing then we have a tools/testing/nvdimm for that.

Of course it's an ABI extension, it allows userspace to discover DSM function numbers the kernel didn't know about at compile time.

A modification to a library or kernel that changes the results of a function (or system call) doesn't necessarily break (or extend) an ABI. An obvious example is that of a random number generator function. A library/kernel is completely free to change the implementation of the random number generator (and the values it returns) without breaking the ABI provided all other rules of ABI preservation are followed.

Now lets look at problem at hand. The pass thru mechanism has very little semantic overhead. Fill in the nd_cmd_pkg as described in ndctl.h, call the ioctl w/ argument with ND_CMD_CALL, and the kernel will marshal up the arguments, call the DSM and return the results. The values of nd_command could be any value and it is for the DSM to either accept or reject the input argument. I wrote this interface and this is how I defined it.

The user application is not changing irrespective of if the kernel applies a mask to the passed in nd_command argument. The data structures are not changing at either source level or binary level. The calling convention is not changing. No object file changes are required. Nothing related to ABI preservation is impacted. The only question is whether the application of a mask to special case function 0 breaks/extends the ABI.

It turns out that this point doesn't really matter as your position is invalid either way.

The argument for this not being an API breakage/extension:

A DSM could either implement or not a function index for any value of N. So, a correctly written application must take into account that for any value of N, the DSM may return error or not. Preserving an ABI doesn't require the library/kernel preserve incorrect application behavior.

Now, assume that the special casing of function zero does constitute a breakage/extension of the ABI:

I'm not the one wishing to special case function 0, you are. So, to this point I say, Dan please don't make needless extension to the ABI. Its and extension and you've provided no valid reason for making it.

Your argument to disallow function zero is invalid.

There is nothing harmful per se to allow function 0. All DSMs that return non zero are required to have it. By excluding it, you actually create the impression that the underlying DSM is violating the DSM specification.

Quoted 103 lines · Jerry Hoemann

Quoted 9 lines · Dan Williams

Quoted 8 lines · Jerry Hoemann

Quoted 7 lines · Dan Williams

Quoted 6 lines · Jerry Hoemann

Quoted 5 lines · Dan Williams

Quoted 4 lines · Dan Williams

Quoted 3 lines · Jerry Hoemann

On Fri, Jun 30, 2017 at 08:55:22PM -0700, Dan Williams wrote:

...

...

Quoted 38 lines

Quoted 31 lines

Quoted 31 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 2 lines

This drops function number 0 which userspace has no need to call.

Actually I like to call function 0. Its an excellent test when modifying the code path as its a no side effects function whose output is known in advance and instantly recognizable. I also use it when testing new firmware.

Quoted 8 lines

Quoted 5 lines

Quoted 5 lines

Quoted 1 line

What is the downside to allowing it? What bad things happen?

It allows implementations to bypass the standardization process and ship new root DSMs. It's always possible to patch the kernel locally for development, so I see no reason to ship this capability globally.

I don't understand this comment, but I think your next comment essentially says to disregard this comment?

Yes, sorry.

Quoted 12 lines

Quoted 2 lines

Actually, just the discovery portion does not lead to this leak, but it's redundant when we have the 'dsm_mask' sysfs attribute.

No. The generation of the mask in sysfs is not done by executing the code in acpi_nfit_ctl. One of the reasons I call function 0 to test changes I am making to the ioctl path itself. The sysfs has nothing to do with that path and cannot be used to serve this purpose.

And since the content of sysfs has been edited it also can not be used as a basic test of firmware.

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

What is the downside to allowing the calling of function 0?

It needlessly expands the kernel ABI. I would suggest, if you want to

No. It is not needless. It is not an ABI extension. Same goes for the override feature.

Quoted 6 lines

If the need is testing then we have a tools/testing/nvdimm for that.

Of course it's an ABI extension, it allows userspace to discover DSM function numbers the kernel didn't know about at compile time.

A modification to a library or kernel that changes the results of a function (or system call) doesn't necessarily break (or extend) an ABI. An obvious example is that of a random number generator function. A library/kernel is completely free to change the implementation of the random number generator (and the values it returns) without breaking the ABI provided all other rules of ABI preservation are followed.

Now lets look at problem at hand. The pass thru mechanism has very little semantic overhead. Fill in the nd_cmd_pkg as described in ndctl.h, call the ioctl w/ argument with ND_CMD_CALL, and the kernel will marshal up the arguments, call the DSM and return the results. The values of nd_command could be any value and it is for the DSM to either accept or reject the input argument. I wrote this interface and this is how I defined it.

The user application is not changing irrespective of if the kernel applies a mask to the passed in nd_command argument. The data structures are not changing at either source level or binary level. The calling convention is not changing. No object file changes are required. Nothing related to ABI preservation is impacted. The only question is whether the application of a mask to special case function 0 breaks/extends the ABI.

It turns out that this point doesn't really matter as your position is invalid either way.

The argument for this not being an API breakage/extension:

A DSM could either implement or not a function index for any value of N. So, a correctly written application must take into account that for any value of N, the DSM may return error or not. Preserving an ABI doesn't require the library/kernel preserve incorrect application behavior.

Now, assume that the special casing of function zero does constitute a breakage/extension of the ABI:

I'm not the one wishing to special case function 0, you are. So, to this point I say, Dan please don't make needless extension to the ABI. Its and extension and you've provided no valid reason for making it.

Your argument to disallow function zero is invalid.

Quoted 3 lines

There is nothing harmful per se to allow function 0. All DSMs that return non zero are required to have it. By excluding it, you actually create the impression that the underlying DSM is violating the DSM specification.

This goes back to the original reasoning for pushing back on the override for the leaf-level _DSM methods. Specifically the ability to bypass the standardization process to ship vendor-specific behavior.

Now, the other side of the argument is that if the next spec adds new _DSMs a simple override can enable them. I am more sympathetic to the override for the leaf / DIMM level because those _DSMs truly are DIMM-vendor specific, but the root device is not. Also, none of the root-level DSMs added for 6.2 are in any way critical for proper operation of the platform, and I do not see any bus-level functionality on the horizon that we need to aggressively pre-enable.

It was a mistake to use _DSM for common root-level functionality, and we shouldn't double down on that mistake by allowing unfettered

definition of new interfaces. NVDIMM is not so special that it needs to bypass the standard ACPI-to-kernel development pipeline.

On Wed, Jul 05, 2017 at 09:35:48AM -0700, Dan Williams wrote:

Quoted 2 lines

It was a mistake to use _DSM for common root-level functionality, and we shouldn't double down on that mistake by allowing unfettered

As to the moral aspects of ACPI's decision to standardiz the DSM for NVDIMM, I take no position on whether it was a good thing or a bad thing; but it is a thing. We need to handle it. I see no particular benefit to making our own lives more difficult.

Have module parameter override_dsm_mask override the dsm_mask for root calls like it does for non-root dsm calls.

Signed-off-by Jerry Hoemann <[email protected]>

---
 drivers/acpi/nfit/core.c | 2 ++
 1 file changed, 2 insertions(+)

drivers/acpi/nfit/core.c +0 -0

index 4e24d69..87acaf2 100644