Re: [PATCH v8 05/12] nfit/libnvdimm: add unlock of nvdimm support for Intel DIMMs

[Dan Williams <dan.j.williams@intel.com>]

On Fri, Sep 21, 2018 at 4:27 PM Dave Jiang <dave.jiang@intel.com> wrote:
>
>
>
> On 09/21/2018 04:20 PM, David Howells wrote:
> > Dave Jiang <dave.jiang@intel.com> wrote:
> >
> >> +    depends on KEYS
> >
> > That needs to be in patch 2 where you create a keyring.
> >
> >> +    char desc[NVDIMM_KEY_DESC_LEN + strlen(NVDIMM_PREFIX)];
> >
> > You should be using sizeof() not strlen() and do you need + 1 for the NUL
> > char?
> >
> >> +    sprintf(desc, "%s%s", NVDIMM_PREFIX, nvdimm->dimm_id);
> >
> > NVDIMM_PREFIX is a constant string.  I would recommend either declaring it as
> > a const char[] or just sticking it in the format string in place of the %s:
> >
> >       sprintf(desc, NVDIMM_PREFIX "%s", nvdimm->dimm_id);
> >
> >> +            if (!cached_key) {
> >> +                    key_link(nvdimm_keyring, key);
> >> +                    nvdimm->key = key;
> >> +                    key->perm |= KEY_USR_SEARCH;
> >> +            }
> >
> > Ummm...  You're altering the key permission?  That's not really yours to
> > change.
>
> What can I do to allow the user app to look up the right key in order to
> pass the key id to sysfs? Without the KEY_USR_SEARCH I am not able to
> search for that key in the keyring.

I don't think you need to search. I would but NVDIMM_PREFIX into
include/uapi/linux/ndctl.h and userspace would need to know by
convention that for NFIT described DIMMs the piece after the prefix is
the ACPI NFIT unique-id. Other buses will need to have other
conventions, but I think it's fine to just document the convention as
"<prefix><bus-provider-specific-unique-id>", and then write up a
document of the known <bus-provider-specific-unique-id> formats.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm