[PATCH 1/4] nvmet: change sn size and check validity
Chaitanya Kulkarni
Chaitanya.Kulkarni at wdc.com
Tue Apr 20 18:35:48 BST 2021
On 4/20/21 02:09, Max Gurtovoy wrote:
> From: Noam Gottlieb <ngottlieb at nvidia.com>
>
> According to the NVM specification, the serial_number should be 20 bytes
> (bytes 23:04 of the Identify Controller data structure), and should
> contain only ASCII characters.
>
> In accordance, the serial_number size is changed to 20 bytes and before
> any attempt to store a new value in serial_number we check that the
> input is valid - i.e. contains only ASCII characters, is not empty and
> does not exceed 20 bytes.
>
> Reviewed-by: Max Gurtovoy <mgurtovoy at nvidia.com>
> Signed-off-by: Noam Gottlieb <ngottlieb at nvidia.com>
> ---
> drivers/nvme/target/admin-cmd.c | 4 +---
> drivers/nvme/target/configfs.c | 36 ++++++++++++++++++++++++---------
> drivers/nvme/target/core.c | 4 +++-
> drivers/nvme/target/discovery.c | 4 +---
> drivers/nvme/target/nvmet.h | 3 ++-
> 5 files changed, 33 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
> index d2a26ff3f7b3..91eb7562a88a 100644
> --- a/drivers/nvme/target/admin-cmd.c
> +++ b/drivers/nvme/target/admin-cmd.c
> @@ -357,9 +357,7 @@ static void nvmet_execute_identify_ctrl(struct nvmet_req *req)
> id->vid = 0;
> id->ssvid = 0;
>
> - memset(id->sn, ' ', sizeof(id->sn));
> - bin2hex(id->sn, &ctrl->subsys->serial,
> - min(sizeof(ctrl->subsys->serial), sizeof(id->sn) / 2));
> + memcpy(id->sn, ctrl->subsys->serial, NVMET_SN_MAX_SIZE);
> memcpy_and_pad(id->mn, sizeof(id->mn), subsys->model_number,
> strlen(subsys->model_number), ' ');
> memcpy_and_pad(id->fr, sizeof(id->fr),
> diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
> index 65a0cf99f557..576540fdba73 100644
> --- a/drivers/nvme/target/configfs.c
> +++ b/drivers/nvme/target/configfs.c
> @@ -1030,24 +1030,46 @@ static ssize_t nvmet_subsys_attr_version_store(struct config_item *item,
> }
> CONFIGFS_ATTR(nvmet_subsys_, attr_version);
>
> +/* See Section 1.5 of NVMe 1.4 */
> +static bool nvmet_is_ascii(const char c)
> +{
> + return c >= 0x20 && c <= 0x7e;
> +}
> +
> static ssize_t nvmet_subsys_attr_serial_show(struct config_item *item,
> char *page)
> {
> struct nvmet_subsys *subsys = to_subsys(item);
>
> - return snprintf(page, PAGE_SIZE, "%llx\n", subsys->serial);
> + return snprintf(page, PAGE_SIZE, "%s\n", subsys->serial);
> }
>
> static ssize_t nvmet_subsys_attr_serial_store(struct config_item *item,
> const char *page, size_t count)
> {
> - u64 serial;
> + struct nvmet_subsys *subsys;
> + int pos, len;
> +
> + subsys = to_subsys(item);
> + len = strcspn(page, "\n");
>
nit:- we can avoid two extra initialization lines with :-
struct nvmet_subsys *subsys = to_subsys(item);
size_t pos, len = strcspn(page, "\n");
> - if (sscanf(page, "%llx\n", &serial) != 1)
> + if (len == 0 || len > NVMET_SN_MAX_SIZE) {
nit:- 's/len == 0/!len/' is pretty common in the code.
> + pr_err("Serial Number can not be empty or exceed %d Bytes\n",
> + NVMET_SN_MAX_SIZE);
> return -EINVAL;
> + }
> +
> + for (pos = 0; pos < len; pos++) {
> + if (!nvmet_is_ascii(page[pos])) {
> + pr_err("Serial Number must contain only ASCII strings\n");
> + return -EINVAL;
> + }
> + }
>
> down_write(&nvmet_config_sem);
> - to_subsys(item)->serial = serial;
> + mutex_lock(&subsys->lock);
> + memcpy_and_pad(subsys->serial, NVMET_SN_MAX_SIZE, page, len, ' ');
> + mutex_unlock(&subsys->lock);
> up_write(&nvmet_config_sem);
>
> return count;
> @@ -1128,12 +1150,6 @@ static ssize_t nvmet_subsys_attr_model_show(struct config_item *item,
> return ret;
> }
>
> -/* See Section 1.5 of NVMe 1.4 */
> -static bool nvmet_is_ascii(const char c)
> -{
> - return c >= 0x20 && c <= 0x7e;
> -}
> -
> static ssize_t nvmet_subsys_attr_model_store_locked(struct nvmet_subsys *subsys,
> const char *page, size_t count)
> {
> diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
> index adbede9ab7f3..3efd48b0a34e 100644
> --- a/drivers/nvme/target/core.c
> +++ b/drivers/nvme/target/core.c
> @@ -1482,6 +1482,7 @@ struct nvmet_subsys *nvmet_subsys_alloc(const char *subsysnqn,
> enum nvme_subsys_type type)
> {
> struct nvmet_subsys *subsys;
> + char serial[NVMET_SN_MAX_SIZE / 2];
This needs a comment why NVMET_SN_MAX_SIZE / 2.
>
> subsys = kzalloc(sizeof(*subsys), GFP_KERNEL);
> if (!subsys)
> @@ -1489,7 +1490,8 @@ struct nvmet_subsys *nvmet_subsys_alloc(const char *subsysnqn,
>
> subsys->ver = NVMET_DEFAULT_VS;
> /* generate a random serial number as our controllers are ephemeral: */
> - get_random_bytes(&subsys->serial, sizeof(subsys->serial));
> + get_random_bytes(&serial, sizeof(serial));
> + bin2hex(subsys->serial, &serial, sizeof(serial));
>
> switch (type) {
> case NVME_NQN_NVME:
> diff --git a/drivers/nvme/target/discovery.c b/drivers/nvme/target/discovery.c
> index 4845d12e374a..f39946615fd6 100644
> --- a/drivers/nvme/target/discovery.c
> +++ b/drivers/nvme/target/discovery.c
> @@ -262,9 +262,7 @@ static void nvmet_execute_disc_identify(struct nvmet_req *req)
> goto out;
> }
>
> - memset(id->sn, ' ', sizeof(id->sn));
> - bin2hex(id->sn, &ctrl->subsys->serial,
> - min(sizeof(ctrl->subsys->serial), sizeof(id->sn) / 2));
> + memcpy(id->sn, ctrl->subsys->serial, NVMET_SN_MAX_SIZE);
> memset(id->fr, ' ', sizeof(id->fr));
> memcpy_and_pad(id->mn, sizeof(id->mn), model, sizeof(model) - 1, ' ');
> memcpy_and_pad(id->fr, sizeof(id->fr),
> diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
> index 5566ed403576..53999bd259ed 100644
> --- a/drivers/nvme/target/nvmet.h
> +++ b/drivers/nvme/target/nvmet.h
> @@ -28,6 +28,7 @@
> #define NVMET_NO_ERROR_LOC ((u16)-1)
> #define NVMET_DEFAULT_CTRL_MODEL "Linux"
> #define NVMET_MN_MAX_SIZE 40
> +#define NVMET_SN_MAX_SIZE 20
>
> /*
> * Supported optional AENs:
> @@ -229,7 +230,7 @@ struct nvmet_subsys {
> u16 max_qid;
>
> u64 ver;
> - u64 serial;
> + char serial[NVMET_SN_MAX_SIZE];
> char *subsysnqn;
> bool pi_support;
>
More information about the Linux-nvme
mailing list