On Thu, 2024-02-15 at 09:16 -0700, Dave Jiang wrote:
> A memdev may optionally not host a mailbox and therefore not able to execute
> the IDENTIFY command. Currently the kernel emits empty strings for some of
> the attributes instead of making them invisible in order to keep backward
> compatibility for CXL CLI. Remove dependency of CXL CLI on the existance of
> these attributes and only expose them if they exist. Without the dependency
> the kernel will be able to make the non-existant attributes invisible.
>
> Link: https://lore.kernel.org/all/[email protected]/
> Suggested-by: Dan Williams <[email protected]>
> Signed-off-by: Dave Jiang <[email protected]>
> ---
> v2:
> - Add lable size check for action_write(). (Wonjae)
> ---
> cxl/lib/libcxl.c | 48 ++++++++++++++++++++++++++----------------------
> cxl/memdev.c | 18 +++++++++++++-----
> 2 files changed, 39 insertions(+), 27 deletions(-)
>
> diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c
> index 1537a33d370e..f807ec4ed4e6 100644
> --- a/cxl/lib/libcxl.c
> +++ b/cxl/lib/libcxl.c
> @@ -1251,28 +1251,30 @@ static void *add_cxl_memdev(void *parent, int id,
> const char *cxlmem_base)
> memdev->minor = minor(st.st_rdev);
>
> sprintf(path, "%s/pmem/size", cxlmem_base);
> - if (sysfs_read_attr(ctx, path, buf) < 0)
> - goto err_read;
> - memdev->pmem_size = strtoull(buf, NULL, 0);
> + if (sysfs_read_attr(ctx, path, buf) == 0)
> + memdev->pmem_size = strtoull(buf, NULL, 0);
>
> sprintf(path, "%s/ram/size", cxlmem_base);
> - if (sysfs_read_attr(ctx, path, buf) < 0)
> - goto err_read;
> - memdev->ram_size = strtoull(buf, NULL, 0);
> + if (sysfs_read_attr(ctx, path, buf) == 0)
> + memdev->ram_size = strtoull(buf, NULL, 0);
>
> sprintf(path, "%s/payload_max", cxlmem_base);
> - if (sysfs_read_attr(ctx, path, buf) < 0)
> - goto err_read;
> - memdev->payload_max = strtoull(buf, NULL, 0);
> - if (memdev->payload_max < 0)
> - goto err_read;
> + if (sysfs_read_attr(ctx, path, buf) == 0) {
> + memdev->payload_max = strtoull(buf, NULL, 0);
> + if (memdev->payload_max < 0)
> + goto err_read;
> + } else {
> + memdev->payload_max = -1;
> + }
>
> sprintf(path, "%s/label_storage_size", cxlmem_base);
> - if (sysfs_read_attr(ctx, path, buf) < 0)
> - goto err_read;
> - memdev->lsa_size = strtoull(buf, NULL, 0);
> - if (memdev->lsa_size == ULLONG_MAX)
> - goto err_read;
> + if (sysfs_read_attr(ctx, path, buf) == 0) {
> + memdev->lsa_size = strtoull(buf, NULL, 0);
> + if (memdev->lsa_size == ULLONG_MAX)
> + goto err_read;
> + } else {
> + memdev->lsa_size = ULLONG_MAX;
> + }
>
> sprintf(path, "%s/serial", cxlmem_base);
> if (sysfs_read_attr(ctx, path, buf) < 0)
> @@ -1299,12 +1301,11 @@ static void *add_cxl_memdev(void *parent, int id,
> const char *cxlmem_base)
> host[0] = '\0';
>
> sprintf(path, "%s/firmware_version", cxlmem_base);
> - if (sysfs_read_attr(ctx, path, buf) < 0)
> - goto err_read;
> -
> - memdev->firmware_version = strdup(buf);
> - if (!memdev->firmware_version)
> - goto err_read;
> + if (sysfs_read_attr(ctx, path, buf) == 0) {
> + memdev->firmware_version = strdup(buf);
> + if (!memdev->firmware_version)
> + goto err_read;
> + }
>
> memdev->dev_buf = calloc(1, strlen(cxlmem_base) + 50);
> if (!memdev->dev_buf)
> @@ -4543,6 +4544,9 @@ static int lsa_op(struct cxl_memdev *memdev, int op,
> void *buf,
> if (length == 0)
> return 0;
>
> + if (memdev->payload_max < 0)
> + return -EINVAL;
> +
> label_iter_max = memdev->payload_max - sizeof(struct cxl_cmd_set_lsa);
> while (remaining) {
> cur_len = min((size_t)label_iter_max, remaining);
> diff --git a/cxl/memdev.c b/cxl/memdev.c
> index 81f07991da06..08b6aa50175f 100644
> --- a/cxl/memdev.c
> +++ b/cxl/memdev.c
> @@ -473,10 +473,13 @@ static int action_zero(struct cxl_memdev *memdev,
> struct action_context *actx)
> size_t size;
> int rc;
>
> - if (param.len)
> + if (param.len) {
> size = param.len;
> - else
> + } else {
> size = cxl_memdev_get_label_size(memdev);
> + if (size == ULLONG_MAX)
Inconsistency between ULLONG_MAX here and ULONG_MAX below. This should
actually be SIZE_MAX considering the return type.
Similarly, when setting lsa_size in add_cxl_memdev() above, in the
'else' case where the sysfs attr was absent, we should use SIZE_MAX.
In the error case for strtoull(), it's probably fine to leave the check
for ULLONG_MAX since that is the max value strtoull() will return, and
we're erroring out at that point anyway. I suppose the right thing to
do there would be to use sscanf() with %zu to parse it correctly for
size_t (of course this incorrectness predates this patch..)
> + return -EINVAL;
> + }
>
> if (cxl_memdev_nvdimm_bridge_active(memdev)) {
> log_err(&ml,
> @@ -509,6 +512,9 @@ static int action_write(struct cxl_memdev *memdev, struct
> action_context *actx)
> if (!size) {
> size_t label_size = cxl_memdev_get_label_size(memdev);
>
> + if (label_size == ULONG_MAX)
> + return -EINVAL;
> +
> fseek(actx->f_in, 0L, SEEK_END);
> size = ftell(actx->f_in);
> fseek(actx->f_in, 0L, SEEK_SET);
> @@ -547,11 +553,13 @@ static int action_read(struct cxl_memdev *memdev,
> struct action_context *actx)
> char *buf;
> int rc;
>
> - if (param.len)
> + if (param.len) {
> size = param.len;
> - else
> + } else {
> size = cxl_memdev_get_label_size(memdev);
> -
> + if (size == ULLONG_MAX)
> + return -EINVAL;
> + }
> buf = calloc(1, size);
> if (!buf)
> return -ENOMEM;
