[PATCH] ndctl/dimm: Fix to dump namespace indexs and labels

[PATCH] ndctl/dimm: Fix to dump namespace indexs and labels

[Jingqi Liu]

The following bug is caused by setting the size of Label Index Block
to a fixed 256 bytes.

Use the following Qemu command to start a Guest with 2MB label-size:
	-object memory-backend-file,id=mem1,share=on,mem-path=/dev/dax1.1,size=14G,align=2M
	-device nvdimm,memdev=mem1,id=nv1,label-size=2M

There is a namespace in the Guest as follows:
	$ ndctl list
	[
	  {
	    "dev":"namespace0.0",
	    "mode":"devdax",
	    "map":"dev",
	    "size":14780727296,
	    "uuid":"58ad5282-5a16-404f-b8ee-e28b4c784eb8",
	    "chardev":"dax0.0",
	    "align":2097152,
	    "name":"namespace0.0"
	  }
	]

Fail to read labels. The result is as follows:
	$ ndctl read-labels -u nmem0
	[
	]
	read 0 nmem

If using the following Qemu command to start the Guest with 128K
label-size, this label can be read correctly.
	-object memory-backend-file,id=mem1,share=on,mem-path=/dev/dax1.1,size=14G,align=2M
	-device nvdimm,memdev=mem1,id=nv1,label-size=128K

The size of a Label Index Block depends on how many label slots fit into
the label storage area. The minimum size of an index block is 256 bytes
and the size must be a multiple of 256 bytes. For a storage area of 128KB,
the corresponding Label Index Block size is 256 bytes. But if the label
storage area is not 128KB, the Label Index Block size should not be 256 bytes.

Namespace Label Index Block appears twice at the top of the label storage area.
Following the two index blocks, an array for storing labels takes up the
remainder of the label storage area.

For obtaining the size of Namespace Index Block, we also cannot rely on
the field of 'mysize' in this index block since it might be corrupted.
Similar to the linux kernel, we use sizeof_namespace_index() to get the size
of Namespace Index Block. Then we can also correctly calculate the starting
offset of the following namespace labels.

Suggested-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Jingqi Liu <jingqi.liu@intel.com>
---
 ndctl/dimm.c           | 19 +++++++++++++++----
 ndctl/lib/dimm.c       |  5 +++++
 ndctl/lib/libndctl.sym |  1 +
 ndctl/libndctl.h       |  1 +
 4 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/ndctl/dimm.c b/ndctl/dimm.c
index 09ce49e..1d2d9a2 100644
--- a/ndctl/dimm.c
+++ b/ndctl/dimm.c
@@ -94,13 +94,18 @@ static struct json_object *dump_label_json(struct ndctl_dimm *dimm,
 	struct json_object *jarray = json_object_new_array();
 	struct json_object *jlabel = NULL;
 	struct namespace_label nslabel;
+	unsigned int nsindex_size;
 	unsigned int slot = -1;
 	ssize_t offset;
 
 	if (!jarray)
 		return NULL;
 
-	for (offset = NSINDEX_ALIGN * 2; offset < size;
+	nsindex_size = ndctl_dimm_sizeof_namespace_index(dimm);
+	if (nsindex_size == 0)
+		return NULL;
+
+	for (offset = nsindex_size * 2; offset < size;
 			offset += ndctl_dimm_sizeof_namespace_label(dimm)) {
 		ssize_t len = min_t(ssize_t,
 				ndctl_dimm_sizeof_namespace_label(dimm),
@@ -204,17 +209,23 @@ static struct json_object *dump_label_json(struct ndctl_dimm *dimm,
 	return jarray;
 }
 
-static struct json_object *dump_index_json(struct ndctl_cmd *cmd_read, ssize_t size)
+static struct json_object *dump_index_json(struct ndctl_dimm *dimm,
+		struct ndctl_cmd *cmd_read, ssize_t size)
 {
 	struct json_object *jarray = json_object_new_array();
 	struct json_object *jindex = NULL;
 	struct namespace_index nsindex;
+	unsigned int nsindex_size;
 	ssize_t offset;
 
 	if (!jarray)
 		return NULL;
 
-	for (offset = 0; offset < NSINDEX_ALIGN * 2; offset += NSINDEX_ALIGN) {
+	nsindex_size = ndctl_dimm_sizeof_namespace_index(dimm);
+	if (nsindex_size == 0)
+		return NULL;
+
+	for (offset = 0; offset < nsindex_size * 2; offset += nsindex_size) {
 		ssize_t len = min_t(ssize_t, sizeof(nsindex), size - offset);
 		struct json_object *jobj;
 
@@ -288,7 +299,7 @@ static struct json_object *dump_json(struct ndctl_dimm *dimm,
 		goto err;
 	json_object_object_add(jdimm, "dev", jobj);
 
-	jindex = dump_index_json(cmd_read, size);
+	jindex = dump_index_json(dimm, cmd_read, size);
 	if (!jindex)
 		goto err;
 	json_object_object_add(jdimm, "index", jindex);
diff --git a/ndctl/lib/dimm.c b/ndctl/lib/dimm.c
index c045cbe..9e36e28 100644
--- a/ndctl/lib/dimm.c
+++ b/ndctl/lib/dimm.c
@@ -256,6 +256,11 @@ static int __label_validate(struct nvdimm_data *ndd)
 	return -EINVAL;
 }
 
+NDCTL_EXPORT unsigned int ndctl_dimm_sizeof_namespace_index(struct ndctl_dimm *dimm)
+{
+	return sizeof_namespace_index(&dimm->ndd);
+}
+
 /*
  * If the dimm labels have not been previously validated this routine
  * will make up a default size. Otherwise, it will pick the size based
diff --git a/ndctl/lib/libndctl.sym b/ndctl/lib/libndctl.sym
index 0a82616..0ce2bb9 100644
--- a/ndctl/lib/libndctl.sym
+++ b/ndctl/lib/libndctl.sym
@@ -290,6 +290,7 @@ global:
 	ndctl_dimm_validate_labels;
 	ndctl_dimm_init_labels;
 	ndctl_dimm_sizeof_namespace_label;
+	ndctl_dimm_sizeof_namespace_index;
 	ndctl_mapping_get_position;
 	ndctl_namespace_set_enforce_mode;
 	ndctl_namespace_get_enforce_mode;
diff --git a/ndctl/libndctl.h b/ndctl/libndctl.h
index 60e1288..9a1a799 100644
--- a/ndctl/libndctl.h
+++ b/ndctl/libndctl.h
@@ -335,6 +335,7 @@ int ndctl_dimm_init_labels(struct ndctl_dimm *dimm,
 		enum ndctl_namespace_version v);
 unsigned long ndctl_dimm_get_available_labels(struct ndctl_dimm *dimm);
 unsigned int ndctl_dimm_sizeof_namespace_label(struct ndctl_dimm *dimm);
+unsigned int ndctl_dimm_sizeof_namespace_index(struct ndctl_dimm *dimm);
 unsigned int ndctl_cmd_cfg_size_get_size(struct ndctl_cmd *cfg_size);
 ssize_t ndctl_cmd_cfg_read_get_data(struct ndctl_cmd *cfg_read, void *buf,
 		unsigned int len, unsigned int offset);
-- 
2.21.3

Re: [PATCH] ndctl/dimm: Fix to dump namespace indexs and labels

[Liu, Jingqi]

Hi Dan,

This is the second version of the patch.
Any comments?

Thanks,
Jingqi

On 6/9/2021 11:06 AM, Liu, Jingqi wrote:
> The following bug is caused by setting the size of Label Index Block
> to a fixed 256 bytes.
> 
> Use the following Qemu command to start a Guest with 2MB label-size:
> 	-object memory-backend-file,id=mem1,share=on,mem-path=/dev/dax1.1,size=14G,align=2M
> 	-device nvdimm,memdev=mem1,id=nv1,label-size=2M
> 
> There is a namespace in the Guest as follows:
> 	$ ndctl list
> 	[
> 	  {
> 	    "dev":"namespace0.0",
> 	    "mode":"devdax",
> 	    "map":"dev",
> 	    "size":14780727296,
> 	    "uuid":"58ad5282-5a16-404f-b8ee-e28b4c784eb8",
> 	    "chardev":"dax0.0",
> 	    "align":2097152,
> 	    "name":"namespace0.0"
> 	  }
> 	]
> 
> Fail to read labels. The result is as follows:
> 	$ ndctl read-labels -u nmem0
> 	[
> 	]
> 	read 0 nmem
> 
> If using the following Qemu command to start the Guest with 128K
> label-size, this label can be read correctly.
> 	-object memory-backend-file,id=mem1,share=on,mem-path=/dev/dax1.1,size=14G,align=2M
> 	-device nvdimm,memdev=mem1,id=nv1,label-size=128K
> 
> The size of a Label Index Block depends on how many label slots fit into
> the label storage area. The minimum size of an index block is 256 bytes
> and the size must be a multiple of 256 bytes. For a storage area of 128KB,
> the corresponding Label Index Block size is 256 bytes. But if the label
> storage area is not 128KB, the Label Index Block size should not be 256 bytes.
> 
> Namespace Label Index Block appears twice at the top of the label storage area.
> Following the two index blocks, an array for storing labels takes up the
> remainder of the label storage area.
> 
> For obtaining the size of Namespace Index Block, we also cannot rely on
> the field of 'mysize' in this index block since it might be corrupted.
> Similar to the linux kernel, we use sizeof_namespace_index() to get the size
> of Namespace Index Block. Then we can also correctly calculate the starting
> offset of the following namespace labels.
> 
> Suggested-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Jingqi Liu <jingqi.liu@intel.com>
> ---
>   ndctl/dimm.c           | 19 +++++++++++++++----
>   ndctl/lib/dimm.c       |  5 +++++
>   ndctl/lib/libndctl.sym |  1 +
>   ndctl/libndctl.h       |  1 +
>   4 files changed, 22 insertions(+), 4 deletions(-)
> 
> diff --git a/ndctl/dimm.c b/ndctl/dimm.c
> index 09ce49e..1d2d9a2 100644
> --- a/ndctl/dimm.c
> +++ b/ndctl/dimm.c
> @@ -94,13 +94,18 @@ static struct json_object *dump_label_json(struct ndctl_dimm *dimm,
>   	struct json_object *jarray = json_object_new_array();
>   	struct json_object *jlabel = NULL;
>   	struct namespace_label nslabel;
> +	unsigned int nsindex_size;
>   	unsigned int slot = -1;
>   	ssize_t offset;
>   
>   	if (!jarray)
>   		return NULL;
>   
> -	for (offset = NSINDEX_ALIGN * 2; offset < size;
> +	nsindex_size = ndctl_dimm_sizeof_namespace_index(dimm);
> +	if (nsindex_size == 0)
> +		return NULL;
> +
> +	for (offset = nsindex_size * 2; offset < size;
>   			offset += ndctl_dimm_sizeof_namespace_label(dimm)) {
>   		ssize_t len = min_t(ssize_t,
>   				ndctl_dimm_sizeof_namespace_label(dimm),
> @@ -204,17 +209,23 @@ static struct json_object *dump_label_json(struct ndctl_dimm *dimm,
>   	return jarray;
>   }
>   
> -static struct json_object *dump_index_json(struct ndctl_cmd *cmd_read, ssize_t size)
> +static struct json_object *dump_index_json(struct ndctl_dimm *dimm,
> +		struct ndctl_cmd *cmd_read, ssize_t size)
>   {
>   	struct json_object *jarray = json_object_new_array();
>   	struct json_object *jindex = NULL;
>   	struct namespace_index nsindex;
> +	unsigned int nsindex_size;
>   	ssize_t offset;
>   
>   	if (!jarray)
>   		return NULL;
>   
> -	for (offset = 0; offset < NSINDEX_ALIGN * 2; offset += NSINDEX_ALIGN) {
> +	nsindex_size = ndctl_dimm_sizeof_namespace_index(dimm);
> +	if (nsindex_size == 0)
> +		return NULL;
> +
> +	for (offset = 0; offset < nsindex_size * 2; offset += nsindex_size) {
>   		ssize_t len = min_t(ssize_t, sizeof(nsindex), size - offset);
>   		struct json_object *jobj;
>   
> @@ -288,7 +299,7 @@ static struct json_object *dump_json(struct ndctl_dimm *dimm,
>   		goto err;
>   	json_object_object_add(jdimm, "dev", jobj);
>   
> -	jindex = dump_index_json(cmd_read, size);
> +	jindex = dump_index_json(dimm, cmd_read, size);
>   	if (!jindex)
>   		goto err;
>   	json_object_object_add(jdimm, "index", jindex);
> diff --git a/ndctl/lib/dimm.c b/ndctl/lib/dimm.c
> index c045cbe..9e36e28 100644
> --- a/ndctl/lib/dimm.c
> +++ b/ndctl/lib/dimm.c
> @@ -256,6 +256,11 @@ static int __label_validate(struct nvdimm_data *ndd)
>   	return -EINVAL;
>   }
>   
> +NDCTL_EXPORT unsigned int ndctl_dimm_sizeof_namespace_index(struct ndctl_dimm *dimm)
> +{
> +	return sizeof_namespace_index(&dimm->ndd);
> +}
> +
>   /*
>    * If the dimm labels have not been previously validated this routine
>    * will make up a default size. Otherwise, it will pick the size based
> diff --git a/ndctl/lib/libndctl.sym b/ndctl/lib/libndctl.sym
> index 0a82616..0ce2bb9 100644
> --- a/ndctl/lib/libndctl.sym
> +++ b/ndctl/lib/libndctl.sym
> @@ -290,6 +290,7 @@ global:
>   	ndctl_dimm_validate_labels;
>   	ndctl_dimm_init_labels;
>   	ndctl_dimm_sizeof_namespace_label;
> +	ndctl_dimm_sizeof_namespace_index;
>   	ndctl_mapping_get_position;
>   	ndctl_namespace_set_enforce_mode;
>   	ndctl_namespace_get_enforce_mode;
> diff --git a/ndctl/libndctl.h b/ndctl/libndctl.h
> index 60e1288..9a1a799 100644
> --- a/ndctl/libndctl.h
> +++ b/ndctl/libndctl.h
> @@ -335,6 +335,7 @@ int ndctl_dimm_init_labels(struct ndctl_dimm *dimm,
>   		enum ndctl_namespace_version v);
>   unsigned long ndctl_dimm_get_available_labels(struct ndctl_dimm *dimm);
>   unsigned int ndctl_dimm_sizeof_namespace_label(struct ndctl_dimm *dimm);
> +unsigned int ndctl_dimm_sizeof_namespace_index(struct ndctl_dimm *dimm);
>   unsigned int ndctl_cmd_cfg_size_get_size(struct ndctl_cmd *cfg_size);
>   ssize_t ndctl_cmd_cfg_read_get_data(struct ndctl_cmd *cfg_read, void *buf,
>   		unsigned int len, unsigned int offset);
> 

Re: [PATCH] ndctl/dimm: Fix to dump namespace indexs and labels

[Dan Williams]

[ add Vishal ]


On Tue, Jun 8, 2021 at 8:16 PM Jingqi Liu <jingqi.liu@intel.com> wrote:
>
> The following bug is caused by setting the size of Label Index Block
> to a fixed 256 bytes.
>
> Use the following Qemu command to start a Guest with 2MB label-size:
>         -object memory-backend-file,id=mem1,share=on,mem-path=/dev/dax1.1,size=14G,align=2M
>         -device nvdimm,memdev=mem1,id=nv1,label-size=2M
>
> There is a namespace in the Guest as follows:
>         $ ndctl list
>         [
>           {
>             "dev":"namespace0.0",
>             "mode":"devdax",
>             "map":"dev",
>             "size":14780727296,
>             "uuid":"58ad5282-5a16-404f-b8ee-e28b4c784eb8",
>             "chardev":"dax0.0",
>             "align":2097152,
>             "name":"namespace0.0"
>           }
>         ]
>
> Fail to read labels. The result is as follows:
>         $ ndctl read-labels -u nmem0
>         [
>         ]
>         read 0 nmem
>
> If using the following Qemu command to start the Guest with 128K
> label-size, this label can be read correctly.
>         -object memory-backend-file,id=mem1,share=on,mem-path=/dev/dax1.1,size=14G,align=2M
>         -device nvdimm,memdev=mem1,id=nv1,label-size=128K
>
> The size of a Label Index Block depends on how many label slots fit into
> the label storage area. The minimum size of an index block is 256 bytes
> and the size must be a multiple of 256 bytes. For a storage area of 128KB,
> the corresponding Label Index Block size is 256 bytes. But if the label
> storage area is not 128KB, the Label Index Block size should not be 256 bytes.
>
> Namespace Label Index Block appears twice at the top of the label storage area.
> Following the two index blocks, an array for storing labels takes up the
> remainder of the label storage area.
>
> For obtaining the size of Namespace Index Block, we also cannot rely on
> the field of 'mysize' in this index block since it might be corrupted.
> Similar to the linux kernel, we use sizeof_namespace_index() to get the size
> of Namespace Index Block. Then we can also correctly calculate the starting
> offset of the following namespace labels.
>
> Suggested-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Jingqi Liu <jingqi.liu@intel.com>

Apologies for the delay in responding, this looks good and passes my tests:

Reviewed-by: Dan Williams <dan.j.williams@intel.com>

Re: [PATCH] ndctl/dimm: Fix to dump namespace indexs and labels

[Verma, Vishal L]

On Wed, 2021-06-09 at 11:06 +0800, Jingqi Liu wrote:
> The following bug is caused by setting the size of Label Index Block
> to a fixed 256 bytes.
> 
> Use the following Qemu command to start a Guest with 2MB label-size:
> 	-object memory-backend-file,id=mem1,share=on,mem-path=/dev/dax1.1,size=14G,align=2M
> 	-device nvdimm,memdev=mem1,id=nv1,label-size=2M
> 
> There is a namespace in the Guest as follows:
> 	$ ndctl list
> 	[
> 	  {
> 	    "dev":"namespace0.0",
> 	    "mode":"devdax",
> 	    "map":"dev",
> 	    "size":14780727296,
> 	    "uuid":"58ad5282-5a16-404f-b8ee-e28b4c784eb8",
> 	    "chardev":"dax0.0",
> 	    "align":2097152,
> 	    "name":"namespace0.0"
> 	  }
> 	]
> 
> Fail to read labels. The result is as follows:
> 	$ ndctl read-labels -u nmem0
> 	[
> 	]
> 	read 0 nmem
> 
> If using the following Qemu command to start the Guest with 128K
> label-size, this label can be read correctly.
> 	-object memory-backend-file,id=mem1,share=on,mem-path=/dev/dax1.1,size=14G,align=2M
> 	-device nvdimm,memdev=mem1,id=nv1,label-size=128K
> 
> The size of a Label Index Block depends on how many label slots fit into
> the label storage area. The minimum size of an index block is 256 bytes
> and the size must be a multiple of 256 bytes. For a storage area of 128KB,
> the corresponding Label Index Block size is 256 bytes. But if the label
> storage area is not 128KB, the Label Index Block size should not be 256 bytes.
> 
> Namespace Label Index Block appears twice at the top of the label storage area.
> Following the two index blocks, an array for storing labels takes up the
> remainder of the label storage area.
> 
> For obtaining the size of Namespace Index Block, we also cannot rely on
> the field of 'mysize' in this index block since it might be corrupted.
> Similar to the linux kernel, we use sizeof_namespace_index() to get the size
> of Namespace Index Block. Then we can also correctly calculate the starting
> offset of the following namespace labels.
> 
> Suggested-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Jingqi Liu <jingqi.liu@intel.com>
> ---
>  ndctl/dimm.c           | 19 +++++++++++++++----
>  ndctl/lib/dimm.c       |  5 +++++
>  ndctl/lib/libndctl.sym |  1 +
>  ndctl/libndctl.h       |  1 +
>  4 files changed, 22 insertions(+), 4 deletions(-)

Hi Jingqi,

This looks fine, one comment below.

[..]
> 
> diff --git a/ndctl/lib/libndctl.sym b/ndctl/lib/libndctl.sym
> index 0a82616..0ce2bb9 100644
> --- a/ndctl/lib/libndctl.sym
> +++ b/ndctl/lib/libndctl.sym
> @@ -290,6 +290,7 @@ global:
>  	ndctl_dimm_validate_labels;
>  	ndctl_dimm_init_labels;
>  	ndctl_dimm_sizeof_namespace_label;
> +	ndctl_dimm_sizeof_namespace_index;

This can't go into an 'old' section of the symbol version script - if
you base off the current 'pending' branch, you should see a LIBNDCTL_26
section at the bottom. You can add this there.

>  	ndctl_mapping_get_position;
>  	ndctl_namespace_set_enforce_mode;
>  	ndctl_namespace_get_enforce_mode;
> diff --git a/ndctl/libndctl.h b/ndctl/libndctl.h
> index 60e1288..9a1a799 100644
> --- a/ndctl/libndctl.h
> +++ b/ndctl/libndctl.h
> @@ -335,6 +335,7 @@ int ndctl_dimm_init_labels(struct ndctl_dimm *dimm,
>  		enum ndctl_namespace_version v);
>  unsigned long ndctl_dimm_get_available_labels(struct ndctl_dimm *dimm);
>  unsigned int ndctl_dimm_sizeof_namespace_label(struct ndctl_dimm *dimm);
> +unsigned int ndctl_dimm_sizeof_namespace_index(struct ndctl_dimm *dimm);
>  unsigned int ndctl_cmd_cfg_size_get_size(struct ndctl_cmd *cfg_size);
>  ssize_t ndctl_cmd_cfg_read_get_data(struct ndctl_cmd *cfg_read, void *buf,
>  		unsigned int len, unsigned int offset);

Re: [PATCH] ndctl/dimm: Fix to dump namespace indexs and labels

[Liu, Jingqi]

Hi Vishal,

Thanks for your comments.

On 7/8/2021 8:21 AM, Verma, Vishal L wrote:
> On Wed, 2021-06-09 at 11:06 +0800, Jingqi Liu wrote:
>> The following bug is caused by setting the size of Label Index Block
>> to a fixed 256 bytes.
>>
>> Use the following Qemu command to start a Guest with 2MB label-size:
>>        -object memory-backend-file,id=mem1,share=on,mem-path=/dev/dax1.1,size=14G,align=2M
>>        -device nvdimm,memdev=mem1,id=nv1,label-size=2M
>>
>> There is a namespace in the Guest as follows:
>>        $ ndctl list
>>        [
>>          {
>>            "dev":"namespace0.0",
>>            "mode":"devdax",
>>            "map":"dev",
>>            "size":14780727296,
>>            "uuid":"58ad5282-5a16-404f-b8ee-e28b4c784eb8",
>>            "chardev":"dax0.0",
>>            "align":2097152,
>>            "name":"namespace0.0"
>>          }
>>        ]
>>
>> Fail to read labels. The result is as follows:
>>        $ ndctl read-labels -u nmem0
>>        [
>>        ]
>>        read 0 nmem
>>
>> If using the following Qemu command to start the Guest with 128K
>> label-size, this label can be read correctly.
>>        -object memory-backend-file,id=mem1,share=on,mem-path=/dev/dax1.1,size=14G,align=2M
>>        -device nvdimm,memdev=mem1,id=nv1,label-size=128K
>>
>> The size of a Label Index Block depends on how many label slots fit into
>> the label storage area. The minimum size of an index block is 256 bytes
>> and the size must be a multiple of 256 bytes. For a storage area of 128KB,
>> the corresponding Label Index Block size is 256 bytes. But if the label
>> storage area is not 128KB, the Label Index Block size should not be 256 bytes.
>>
>> Namespace Label Index Block appears twice at the top of the label storage area.
>> Following the two index blocks, an array for storing labels takes up the
>> remainder of the label storage area.
>>
>> For obtaining the size of Namespace Index Block, we also cannot rely on
>> the field of 'mysize' in this index block since it might be corrupted.
>> Similar to the linux kernel, we use sizeof_namespace_index() to get the size
>> of Namespace Index Block. Then we can also correctly calculate the starting
>> offset of the following namespace labels.
>>
>> Suggested-by: Dan Williams <dan.j.williams@intel.com>
>> Signed-off-by: Jingqi Liu <jingqi.liu@intel.com>
>> ---
>>   ndctl/dimm.c           | 19 +++++++++++++++----
>>   ndctl/lib/dimm.c       |  5 +++++
>>   ndctl/lib/libndctl.sym |  1 +
>>   ndctl/libndctl.h       |  1 +
>>   4 files changed, 22 insertions(+), 4 deletions(-)
> 
> Hi Jingqi,
> 
> This looks fine, one comment below.
> 
> [..]
>>
>> diff --git a/ndctl/lib/libndctl.sym b/ndctl/lib/libndctl.sym
>> index 0a82616..0ce2bb9 100644
>> --- a/ndctl/lib/libndctl.sym
>> +++ b/ndctl/lib/libndctl.sym
>> @@ -290,6 +290,7 @@ global:
>>        ndctl_dimm_validate_labels;
>>        ndctl_dimm_init_labels;
>>        ndctl_dimm_sizeof_namespace_label;
>> +     ndctl_dimm_sizeof_namespace_index;
> 
> This can't go into an 'old' section of the symbol version script - if
> you base off the current 'pending' branch, you should see a LIBNDCTL_26
> section at the bottom. You can add this there.

It's based on the current 'master' branch.
I don't see a LIBNDCTL_26 section, just 'LIBNDCTL_25'.
How about adding 'ndctl_dimm_sizeof_namespace_index' to LIBNDCTL_25 
section ?

Thanks,
Jingqi

Re: [PATCH] ndctl/dimm: Fix to dump namespace indexs and labels

[Verma, Vishal L]

On Thu, 2021-07-08 at 09:53 +0800, Liu, Jingqi wrote:
> > 
> > [..]
> > > 
> > > diff --git a/ndctl/lib/libndctl.sym b/ndctl/lib/libndctl.sym
> > > index 0a82616..0ce2bb9 100644
> > > --- a/ndctl/lib/libndctl.sym
> > > +++ b/ndctl/lib/libndctl.sym
> > > @@ -290,6 +290,7 @@ global:
> > >        ndctl_dimm_validate_labels;
> > >        ndctl_dimm_init_labels;
> > >        ndctl_dimm_sizeof_namespace_label;
> > > +     ndctl_dimm_sizeof_namespace_index;
> > 
> > This can't go into an 'old' section of the symbol version script - if
> > you base off the current 'pending' branch, you should see a LIBNDCTL_26
> > section at the bottom. You can add this there.
> 
> It's based on the current 'master' branch.
> I don't see a LIBNDCTL_26 section, just 'LIBNDCTL_25'.
> How about adding 'ndctl_dimm_sizeof_namespace_index' to LIBNDCTL_25 
> section ?
> 
No - so once a release happens, that section is 'closed' forever. The
master branch coincides with the v71 release. That release had added
new symbols in the LIBNDCTL_25 section, and that section is now done.
New symbols after v71 need to go in a new section, LIBNDCTL_26.

The pending branch just happens to have patches that added a new
symbol, so the new section is already created for you - so if you
rebase to pending, you can just reuse that. Alternatively, base off
master, and create a new LIBNDCTL_26 section, and I'll fix up the
trivial conflict when merging.

Hope this clarifies things a bit!

Thanks,
-Vishal

Re: [PATCH] ndctl/dimm: Fix to dump namespace indexs and labels

[Liu, Jingqi]

On 7/8/2021 1:59 PM, Verma, Vishal L wrote:
> On Thu, 2021-07-08 at 09:53 +0800, Liu, Jingqi wrote:
>>>
>>> [..]
>>>>
>>>> diff --git a/ndctl/lib/libndctl.sym b/ndctl/lib/libndctl.sym
>>>> index 0a82616..0ce2bb9 100644
>>>> --- a/ndctl/lib/libndctl.sym
>>>> +++ b/ndctl/lib/libndctl.sym
>>>> @@ -290,6 +290,7 @@ global:
>>>>         ndctl_dimm_validate_labels;
>>>>         ndctl_dimm_init_labels;
>>>>         ndctl_dimm_sizeof_namespace_label;
>>>> +     ndctl_dimm_sizeof_namespace_index;
>>>
>>> This can't go into an 'old' section of the symbol version script - if
>>> you base off the current 'pending' branch, you should see a LIBNDCTL_26
>>> section at the bottom. You can add this there.
>>
>> It's based on the current 'master' branch.
>> I don't see a LIBNDCTL_26 section, just 'LIBNDCTL_25'.
>> How about adding 'ndctl_dimm_sizeof_namespace_index' to LIBNDCTL_25
>> section ?
>>
> No - so once a release happens, that section is 'closed' forever. The
> master branch coincides with the v71 release. That release had added
> new symbols in the LIBNDCTL_25 section, and that section is now done.
> New symbols after v71 need to go in a new section, LIBNDCTL_26.
> 
> The pending branch just happens to have patches that added a new
> symbol, so the new section is already created for you - so if you
> rebase to pending, you can just reuse that. Alternatively, base off
> master, and create a new LIBNDCTL_26 section, and I'll fix up the
> trivial conflict when merging.
> 
> Hope this clarifies things a bit!

Got it.
Thanks your clarification.
The other modifications of this patch are based on the master branch.
So for this file, I'll base off master.
Thank you for fixing up the conflict when merging.

Thanks,
Jingqi
> 
> Thanks,
> -Vishal
> 

[PATCH] ndctl/dimm: Fix to dump namespace indexs and labels

[Jingqi Liu]

The following bug is caused by setting the size of Label Index Block
to a fixed 256 bytes.

Use the following Qemu command to start a Guest with 2MB label-size:
	-object memory-backend-file,id=mem1,share=on,mem-path=/dev/dax1.1,size=14G,align=2M
	-device nvdimm,memdev=mem1,id=nv1,label-size=2M

There is a namespace in the Guest as follows:
	$ ndctl list
	[
	  {
	    "dev":"namespace0.0",
	    "mode":"devdax",
	    "map":"dev",
	    "size":14780727296,
	    "uuid":"58ad5282-5a16-404f-b8ee-e28b4c784eb8",
	    "chardev":"dax0.0",
	    "align":2097152,
	    "name":"namespace0.0"
	  }
	]

Fail to read labels. The result is as follows:
	$ ndctl read-labels -u nmem0
	[
	]
	read 0 nmem

If using the following Qemu command to start the Guest with 128K
label-size, this label can be read correctly.
	-object memory-backend-file,id=mem1,share=on,mem-path=/dev/dax1.1,size=14G,align=2M
	-device nvdimm,memdev=mem1,id=nv1,label-size=128K

The size of a Label Index Block depends on how many label slots fit into
the label storage area. The minimum size of an index block is 256 bytes
and the size must be a multiple of 256 bytes. For a storage area of 128KB,
the corresponding Label Index Block size is 256 bytes. But if the label
storage area is not 128KB, the Label Index Block size should not be 256 bytes.

Namespace Label Index Block appears twice at the top of the label storage area.
Following the two index blocks, an array for storing labels takes up the
remainder of the label storage area.

When reading namespace index and labels, we should read the field of 'mysize'
in the Label Index Block. Then we can correctly calculate the starting offset
of another Label Index Block and the following namespace labels.

Signed-off-by: Jingqi Liu <jingqi.liu@intel.com>
---
 ndctl/dimm.c | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/ndctl/dimm.c b/ndctl/dimm.c
index 09ce49e..e05dcc2 100644
--- a/ndctl/dimm.c
+++ b/ndctl/dimm.c
@@ -94,13 +94,25 @@ static struct json_object *dump_label_json(struct ndctl_dimm *dimm,
 	struct json_object *jarray = json_object_new_array();
 	struct json_object *jlabel = NULL;
 	struct namespace_label nslabel;
+	struct namespace_index nsindex;
+	ssize_t nsindex_len = min_t(ssize_t, sizeof(nsindex), size);
+	ssize_t nsindex_mysize;
 	unsigned int slot = -1;
 	ssize_t offset;
 
 	if (!jarray)
 		return NULL;
 
-	for (offset = NSINDEX_ALIGN * 2; offset < size;
+	nsindex_len = ndctl_cmd_cfg_read_get_data(cmd_read, &nsindex, nsindex_len, 0);
+	if (nsindex_len < 0)
+		return NULL;
+
+	nsindex_mysize = le64_to_cpu(nsindex.mysize);
+	if ((nsindex_mysize > size)
+			|| !IS_ALIGNED(nsindex_mysize, NSINDEX_ALIGN))
+		return NULL;
+
+	for (offset = nsindex_mysize * 2; offset < size;
 			offset += ndctl_dimm_sizeof_namespace_label(dimm)) {
 		ssize_t len = min_t(ssize_t,
 				ndctl_dimm_sizeof_namespace_label(dimm),
@@ -210,13 +222,15 @@ static struct json_object *dump_index_json(struct ndctl_cmd *cmd_read, ssize_t s
 	struct json_object *jindex = NULL;
 	struct namespace_index nsindex;
 	ssize_t offset;
+	int i;
 
 	if (!jarray)
 		return NULL;
 
-	for (offset = 0; offset < NSINDEX_ALIGN * 2; offset += NSINDEX_ALIGN) {
+	for (i = 0, offset = 0; i < 2 ; i++) {
 		ssize_t len = min_t(ssize_t, sizeof(nsindex), size - offset);
 		struct json_object *jobj;
+		ssize_t nsindex_mysize;
 
 		jindex = json_object_new_object();
 		if (!jindex)
@@ -229,6 +243,11 @@ static struct json_object *dump_index_json(struct ndctl_cmd *cmd_read, ssize_t s
 		if (len < 0)
 			break;
 
+		nsindex_mysize = le64_to_cpu(nsindex.mysize);
+		if ((nsindex_mysize > size)
+				|| !IS_ALIGNED(nsindex_mysize, NSINDEX_ALIGN))
+			break;
+
 		nsindex.sig[NSINDEX_SIG_LEN - 1] = 0;
 		jobj = json_object_new_string(nsindex.sig);
 		if (!jobj)
@@ -261,6 +280,8 @@ static struct json_object *dump_index_json(struct ndctl_cmd *cmd_read, ssize_t s
 		json_object_object_add(jindex, "nslot", jobj);
 
 		json_object_array_add(jarray, jindex);
+
+		offset += nsindex_mysize;
 	}
 
 	if (json_object_array_length(jarray) < 1) {
-- 
2.21.3

Re: [PATCH] ndctl/dimm: Fix to dump namespace indexs and labels

[Dan Williams]

On Wed, Jun 2, 2021 at 6:36 PM Jingqi Liu <jingqi.liu@intel.com> wrote:
>
> The following bug is caused by setting the size of Label Index Block
> to a fixed 256 bytes.
>
> Use the following Qemu command to start a Guest with 2MB label-size:
>         -object memory-backend-file,id=mem1,share=on,mem-path=/dev/dax1.1,size=14G,align=2M
>         -device nvdimm,memdev=mem1,id=nv1,label-size=2M
>
> There is a namespace in the Guest as follows:
>         $ ndctl list
>         [
>           {
>             "dev":"namespace0.0",
>             "mode":"devdax",
>             "map":"dev",
>             "size":14780727296,
>             "uuid":"58ad5282-5a16-404f-b8ee-e28b4c784eb8",
>             "chardev":"dax0.0",
>             "align":2097152,
>             "name":"namespace0.0"
>           }
>         ]
>
> Fail to read labels. The result is as follows:
>         $ ndctl read-labels -u nmem0
>         [
>         ]
>         read 0 nmem
>
> If using the following Qemu command to start the Guest with 128K
> label-size, this label can be read correctly.
>         -object memory-backend-file,id=mem1,share=on,mem-path=/dev/dax1.1,size=14G,align=2M
>         -device nvdimm,memdev=mem1,id=nv1,label-size=128K
>
> The size of a Label Index Block depends on how many label slots fit into
> the label storage area. The minimum size of an index block is 256 bytes
> and the size must be a multiple of 256 bytes. For a storage area of 128KB,
> the corresponding Label Index Block size is 256 bytes. But if the label
> storage area is not 128KB, the Label Index Block size should not be 256 bytes.
>
> Namespace Label Index Block appears twice at the top of the label storage area.
> Following the two index blocks, an array for storing labels takes up the
> remainder of the label storage area.
>
> When reading namespace index and labels, we should read the field of 'mysize'
> in the Label Index Block. Then we can correctly calculate the starting offset
> of another Label Index Block and the following namespace labels.

Good find! I agree this is broken, but I'm not sure this is the way to
fix it. The ndctl enabling is meant to support dumping index blocks
that might be corrupt, so I don't want to rely on index block data for
this value. It should copy the kernel which has this definition for
determining sizeof_namespace_index():

size_t sizeof_namespace_index(struct nvdimm_drvdata *ndd)
{
        u32 nslot, space, size;

        /*
         * Per UEFI 2.7, the minimum size of the Label Storage Area is large
         * enough to hold 2 index blocks and 2 labels.  The minimum index
         * block size is 256 bytes. The label size is 128 for namespaces
         * prior to version 1.2 and at minimum 256 for version 1.2 and later.
         */
        nslot = nvdimm_num_label_slots(ndd);
        space = ndd->nsarea.config_size - nslot * sizeof_namespace_label(ndd);
        size = __sizeof_namespace_index(nslot) * 2;
        if (size <= space && nslot >= 2)
                return size / 2;

        dev_err(ndd->dev, "label area (%d) too small to host (%d byte)
labels\n",
                        ndd->nsarea.config_size, sizeof_namespace_label(ndd));
        return 0;
}

Re: [PATCH] ndctl/dimm: Fix to dump namespace indexs and labels

[Liu, Jingqi]

Hi Dan,

On 6/8/2021 4:03 AM, Dan Williams wrote:
> On Wed, Jun 2, 2021 at 6:36 PM Jingqi Liu <jingqi.liu@intel.com> wrote:
>>
>> The following bug is caused by setting the size of Label Index Block
>> to a fixed 256 bytes.
>>
>> Use the following Qemu command to start a Guest with 2MB label-size:
>>          -object memory-backend-file,id=mem1,share=on,mem-path=/dev/dax1.1,size=14G,align=2M
>>          -device nvdimm,memdev=mem1,id=nv1,label-size=2M
>>
>> There is a namespace in the Guest as follows:
>>          $ ndctl list
>>          [
>>            {
>>              "dev":"namespace0.0",
>>              "mode":"devdax",
>>              "map":"dev",
>>              "size":14780727296,
>>              "uuid":"58ad5282-5a16-404f-b8ee-e28b4c784eb8",
>>              "chardev":"dax0.0",
>>              "align":2097152,
>>              "name":"namespace0.0"
>>            }
>>          ]
>>
>> Fail to read labels. The result is as follows:
>>          $ ndctl read-labels -u nmem0
>>          [
>>          ]
>>          read 0 nmem
>>
>> If using the following Qemu command to start the Guest with 128K
>> label-size, this label can be read correctly.
>>          -object memory-backend-file,id=mem1,share=on,mem-path=/dev/dax1.1,size=14G,align=2M
>>          -device nvdimm,memdev=mem1,id=nv1,label-size=128K
>>
>> The size of a Label Index Block depends on how many label slots fit into
>> the label storage area. The minimum size of an index block is 256 bytes
>> and the size must be a multiple of 256 bytes. For a storage area of 128KB,
>> the corresponding Label Index Block size is 256 bytes. But if the label
>> storage area is not 128KB, the Label Index Block size should not be 256 bytes.
>>
>> Namespace Label Index Block appears twice at the top of the label storage area.
>> Following the two index blocks, an array for storing labels takes up the
>> remainder of the label storage area.
>>
>> When reading namespace index and labels, we should read the field of 'mysize'
>> in the Label Index Block. Then we can correctly calculate the starting offset
>> of another Label Index Block and the following namespace labels.
> 
> Good find! I agree this is broken, but I'm not sure this is the way to
> fix it. The ndctl enabling is meant to support dumping index blocks
> that might be corrupt, so I don't want to rely on index block data for
> this value. It should copy the kernel which has this definition for
> determining sizeof_namespace_index():
> 
> size_t sizeof_namespace_index(struct nvdimm_drvdata *ndd)
> {
>          u32 nslot, space, size;
> 
>          /*
>           * Per UEFI 2.7, the minimum size of the Label Storage Area is large
>           * enough to hold 2 index blocks and 2 labels.  The minimum index
>           * block size is 256 bytes. The label size is 128 for namespaces
>           * prior to version 1.2 and at minimum 256 for version 1.2 and later.
>           */
>          nslot = nvdimm_num_label_slots(ndd);
>          space = ndd->nsarea.config_size - nslot * sizeof_namespace_label(ndd);
>          size = __sizeof_namespace_index(nslot) * 2;
>          if (size <= space && nslot >= 2)
>                  return size / 2;
> 
>          dev_err(ndd->dev, "label area (%d) too small to host (%d byte)
> labels\n",
>                          ndd->nsarea.config_size, sizeof_namespace_label(ndd));
>          return 0;
> }
> 
Good point. Thanks for your comment.
I'll send a patch based on your suggestion soon.

Thanks,
Jingqi

[PATCH] ndctl/dimm: Fix to dump namespace indexs and labels

[Jingqi Liu]

The following bug is caused by setting the size of Label Index Block
to a fixed 256 bytes.

Use the following Qemu command to start a Guest with 2MB label-size:
-object memory-backend-file,id=mem1,share=on,mem-path=/dev/dax1.1,size=14G,align=2M
-device nvdimm,memdev=mem1,id=nv1,label-size=2M

There is a namespace in the Guest as follows:
[
  {
    "dev":"namespace0.0",
    "mode":"devdax",
    "map":"dev",
    "size":14780727296,
    "uuid":"58ad5282-5a16-404f-b8ee-e28b4c784eb8",
    "chardev":"dax0.0",
    "align":2097152,
    "name":"namespace0.0"
  }
]

Fail to read labels. The result is as follows:
[
]
read 0 nmem

If using the following Qemu command to start the Guest with 128K
label-size, this label can be read correctly.
-object memory-backend-file,id=mem1,share=on,mem-path=/dev/dax1.1,size=14G,align=2M
-device nvdimm,memdev=mem1,id=nv1,label-size=128K

The size of a Label Index Block depends on how many label slots fit into
the label storage area. The minimum size of an index block is 256 bytes
and the size must be a multiple of 256 bytes. For a storage area of 128KB,
the corresponding Label Index Block size is 256 bytes. But if the label
storage area is not 128KB, the Label Index Block size should not be 256 bytes.

Namespace Label Index Block appears twice at the top of the label storage area.
Following the two index blocks, an array for storing labels takes up the
remainder of the label storage area.

When reading namespace index and labels, we should read the field of 'mysize'
in the Label Index Block. Then we can correctly calculate the starting offset
of another Label Index Block and the following namespace labels.

Signed-off-by: Jingqi Liu <jingqi.liu@intel.com>
---
 ndctl/dimm.c | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/ndctl/dimm.c b/ndctl/dimm.c
index 09ce49e..e05dcc2 100644
--- a/ndctl/dimm.c
+++ b/ndctl/dimm.c
@@ -94,13 +94,25 @@ static struct json_object *dump_label_json(struct ndctl_dimm *dimm,
 	struct json_object *jarray = json_object_new_array();
 	struct json_object *jlabel = NULL;
 	struct namespace_label nslabel;
+	struct namespace_index nsindex;
+	ssize_t nsindex_len = min_t(ssize_t, sizeof(nsindex), size);
+	ssize_t nsindex_mysize;
 	unsigned int slot = -1;
 	ssize_t offset;
 
 	if (!jarray)
 		return NULL;
 
-	for (offset = NSINDEX_ALIGN * 2; offset < size;
+	nsindex_len = ndctl_cmd_cfg_read_get_data(cmd_read, &nsindex, nsindex_len, 0);
+	if (nsindex_len < 0)
+		return NULL;
+
+	nsindex_mysize = le64_to_cpu(nsindex.mysize);
+	if ((nsindex_mysize > size)
+			|| !IS_ALIGNED(nsindex_mysize, NSINDEX_ALIGN))
+		return NULL;
+
+	for (offset = nsindex_mysize * 2; offset < size;
 			offset += ndctl_dimm_sizeof_namespace_label(dimm)) {
 		ssize_t len = min_t(ssize_t,
 				ndctl_dimm_sizeof_namespace_label(dimm),
@@ -210,13 +222,15 @@ static struct json_object *dump_index_json(struct ndctl_cmd *cmd_read, ssize_t s
 	struct json_object *jindex = NULL;
 	struct namespace_index nsindex;
 	ssize_t offset;
+	int i;
 
 	if (!jarray)
 		return NULL;
 
-	for (offset = 0; offset < NSINDEX_ALIGN * 2; offset += NSINDEX_ALIGN) {
+	for (i = 0, offset = 0; i < 2 ; i++) {
 		ssize_t len = min_t(ssize_t, sizeof(nsindex), size - offset);
 		struct json_object *jobj;
+		ssize_t nsindex_mysize;
 
 		jindex = json_object_new_object();
 		if (!jindex)
@@ -229,6 +243,11 @@ static struct json_object *dump_index_json(struct ndctl_cmd *cmd_read, ssize_t s
 		if (len < 0)
 			break;
 
+		nsindex_mysize = le64_to_cpu(nsindex.mysize);
+		if ((nsindex_mysize > size)
+				|| !IS_ALIGNED(nsindex_mysize, NSINDEX_ALIGN))
+			break;
+
 		nsindex.sig[NSINDEX_SIG_LEN - 1] = 0;
 		jobj = json_object_new_string(nsindex.sig);
 		if (!jobj)
@@ -261,6 +280,8 @@ static struct json_object *dump_index_json(struct ndctl_cmd *cmd_read, ssize_t s
 		json_object_object_add(jindex, "nslot", jobj);
 
 		json_object_array_add(jarray, jindex);
+
+		offset += nsindex_mysize;
 	}
 
 	if (json_object_array_length(jarray) < 1) {
-- 
2.21.3