Hi Dan, This sounds like good idea and would reduce some cmd_pkg checking overhead in nvdimm and bus modules. However have few concerns regarding the proposed PAPR_SCM command family below
Dan Williams <[email protected]> writes: > The ND_CMD_CALL format allows for a general passthrough of whitelisted > commands targeting a given command set. However there is no validation > of the family index relative to what the bus supports. > > - Update the NFIT bus implementation (the only one that supports > ND_CMD_CALL passthrough) to also whitelist the valid set of command > family indices. PAPR_SCM command family will also use ND_CMD_CALL passthrough and will need to be whitelist even though it doesnt use NFIT. > > - Update the generic __nd_ioctl() path to validate that field on behalf > of all implementations. > > Fixes: 31eca76ba2fc ("nfit, libnvdimm: limited/whitelisted dimm command > marshaling mechanism") > Cc: <[email protected]> > Signed-off-by: Dan Williams <[email protected]> > --- > drivers/acpi/nfit/core.c | 11 +++++++++-- > drivers/acpi/nfit/nfit.h | 1 - > drivers/nvdimm/bus.c | 16 ++++++++++++++++ > include/linux/libnvdimm.h | 2 ++ > include/uapi/linux/ndctl.h | 4 ++++ > 5 files changed, 31 insertions(+), 3 deletions(-) > > diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c > index d0090f71585c..bcf5af803941 100644 > --- a/drivers/acpi/nfit/core.c > +++ b/drivers/acpi/nfit/core.c > @@ -1823,6 +1823,7 @@ static void populate_shutdown_status(struct nfit_mem > *nfit_mem) > static int acpi_nfit_add_dimm(struct acpi_nfit_desc *acpi_desc, > struct nfit_mem *nfit_mem, u32 device_handle) > { > + struct nvdimm_bus_descriptor *nd_desc = &acpi_desc->nd_desc; > struct acpi_device *adev, *adev_dimm; > struct device *dev = acpi_desc->dev; > unsigned long dsm_mask, label_mask; > @@ -1834,6 +1835,7 @@ static int acpi_nfit_add_dimm(struct acpi_nfit_desc > *acpi_desc, > /* nfit test assumes 1:1 relationship between commands and dsms */ > nfit_mem->dsm_mask = acpi_desc->dimm_cmd_force_en; > nfit_mem->family = NVDIMM_FAMILY_INTEL; > + set_bit(NVDIMM_FAMILY_INTEL, &nd_desc->dimm_family_mask); > > if (dcr->valid_fields & ACPI_NFIT_CONTROL_MFG_INFO_VALID) > sprintf(nfit_mem->id, "%04x-%02x-%04x-%08x", > @@ -1886,10 +1888,13 @@ static int acpi_nfit_add_dimm(struct acpi_nfit_desc > *acpi_desc, > * Note, that checking for function0 (bit0) tells us if any commands > * are reachable through this GUID. > */ > + clear_bit(NVDIMM_FAMILY_INTEL, &nd_desc->dimm_family_mask); > for (i = 0; i <= NVDIMM_FAMILY_MAX; i++) > - if (acpi_check_dsm(adev_dimm->handle, to_nfit_uuid(i), 1, 1)) > + if (acpi_check_dsm(adev_dimm->handle, to_nfit_uuid(i), 1, 1)) { > + set_bit(i, &nd_desc->dimm_family_mask); > if (family < 0 || i == default_dsm_family) > family = i; > + } > > /* limit the supported commands to those that are publicly documented */ > nfit_mem->family = family; > @@ -2151,6 +2156,9 @@ static void acpi_nfit_init_dsms(struct acpi_nfit_desc > *acpi_desc) > > nd_desc->cmd_mask = acpi_desc->bus_cmd_force_en; > nd_desc->bus_dsm_mask = acpi_desc->bus_nfit_cmd_force_en; > + set_bit(ND_CMD_CALL, &nd_desc->cmd_mask); > + set_bit(NVDIMM_BUS_FAMILY_NFIT, &nd_desc->bus_family_mask); > + > adev = to_acpi_dev(acpi_desc); > if (!adev) > return; > @@ -2158,7 +2166,6 @@ static void acpi_nfit_init_dsms(struct acpi_nfit_desc > *acpi_desc) > for (i = ND_CMD_ARS_CAP; i <= ND_CMD_CLEAR_ERROR; i++) > if (acpi_check_dsm(adev->handle, guid, 1, 1ULL << i)) > set_bit(i, &nd_desc->cmd_mask); > - set_bit(ND_CMD_CALL, &nd_desc->cmd_mask); > > dsm_mask = > (1 << ND_CMD_ARS_CAP) | > diff --git a/drivers/acpi/nfit/nfit.h b/drivers/acpi/nfit/nfit.h > index b317f4043705..5f5f8ce030e7 100644 > --- a/drivers/acpi/nfit/nfit.h > +++ b/drivers/acpi/nfit/nfit.h > @@ -33,7 +33,6 @@ > | ACPI_NFIT_MEM_RESTORE_FAILED | ACPI_NFIT_MEM_FLUSH_FAILED \ > | ACPI_NFIT_MEM_NOT_ARMED | ACPI_NFIT_MEM_MAP_FAILED) > > -#define NVDIMM_FAMILY_MAX NVDIMM_FAMILY_HYPERV > #define NVDIMM_CMD_MAX 31 > > #define NVDIMM_STANDARD_CMDMASK \ > diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c > index 09087c38fabd..955265656b96 100644 > --- a/drivers/nvdimm/bus.c > +++ b/drivers/nvdimm/bus.c > @@ -1037,9 +1037,25 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, > struct nvdimm *nvdimm, > dimm_name = "bus"; > } > > + /* Validate command family support against bus declared support */ > if (cmd == ND_CMD_CALL) { > + unsigned long *mask; > + > if (copy_from_user(&pkg, p, sizeof(pkg))) > return -EFAULT; > + > + if (nvdimm) { > + if (pkg.nd_family > NVDIMM_FAMILY_MAX) > + return -EINVAL; > + mask = &nd_desc->dimm_family_mask; > + } else { > + if (pkg.nd_family > NVDIMM_BUS_FAMILY_MAX) > + return -EINVAL; > + mask = &nd_desc->bus_family_mask; > + } > + > + if (!test_bit(pkg.nd_family, mask)) > + return -EINVAL; > } > > if (!desc || > diff --git a/include/linux/libnvdimm.h b/include/linux/libnvdimm.h > index 9df091bd30ba..b41857f43883 100644 > --- a/include/linux/libnvdimm.h > +++ b/include/linux/libnvdimm.h > @@ -76,6 +76,8 @@ struct nvdimm_bus_descriptor { > const struct attribute_group **attr_groups; > unsigned long bus_dsm_mask; > unsigned long cmd_mask; > + unsigned long dimm_family_mask; > + unsigned long bus_family_mask; > struct module *module; > char *provider_name; > struct device_node *of_node; > diff --git a/include/uapi/linux/ndctl.h b/include/uapi/linux/ndctl.h > index de5d90212409..e28763c234e2 100644 > --- a/include/uapi/linux/ndctl.h > +++ b/include/uapi/linux/ndctl.h > @@ -244,6 +244,10 @@ struct nd_cmd_pkg { > #define NVDIMM_FAMILY_HPE2 2 > #define NVDIMM_FAMILY_MSFT 3 > #define NVDIMM_FAMILY_HYPERV 4 > +#define NVDIMM_FAMILY_MAX NVDIMM_FAMILY_HYPERV How do you envision support for a new command family that doesnt support NFIT like PAPR_SCM, be added to this list ? As I see the value NVDIMM_FAMILY_MAX will be tied to the set of command families supported by nfit and if PAPR_SCM command family is added at index 5 then should or should-not the NVDIMM_FAMILY_MAX be updated to value 5. If NVDIMM_FAMILY_MAX is not updated then __nd_ioctl() wont let PAPR-SCM commands pass through. However if NVDIMM_FAMILY_MAX is updated then nfit module will wrongly advertise support for PAPR-SCM command family. > + > +#define NVDIMM_BUS_FAMILY_NFIT 0 > +#define NVDIMM_BUS_FAMILY_MAX NVDIMM_BUS_FAMILY_NFIT > > #define ND_IOCTL_CALL _IOWR(ND_IOCTL, ND_CMD_CALL,\ > struct nd_cmd_pkg) > Thanks, -- Vaibhav Jain <[email protected]> Linux Technology Center, IBM India Pvt. Ltd. _______________________________________________ Linux-nvdimm mailing list -- [email protected] To unsubscribe send an email to [email protected]
