[PATCH 0/2] nvme: Check size of command structures

[PATCH 0/2] nvme: Check size of command structures

[Minwoo Im]

Many command structures have been added in linux/nvme.h wihtout
BUILD_BUG_ON() check.  It also needs to be assured to avoid unintended
growth of structures.

Minwoo Im (2):
  nvme: fabrics: Make sure command struct size fixed
  nvme: pci: Make sure command struct size fixed

 drivers/nvme/host/fabrics.c | 1 +
 drivers/nvme/host/pci.c     | 7 +++++++
 2 files changed, 8 insertions(+)

-- 
2.17.1

[PATCH 1/2] nvme: fabrics: Make sure command struct size fixed

[Minwoo Im]

struct common_command provides a common structure for NVMe-oF command
format.  It also needs to be checked for unintended size growth.

Signed-off-by: Minwoo Im <minwoo.im.dev at gmail.com>
---
 drivers/nvme/host/fabrics.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
index d4cb826f58ff..592d1e61ef7e 100644
--- a/drivers/nvme/host/fabrics.c
+++ b/drivers/nvme/host/fabrics.c
@@ -1188,6 +1188,7 @@ static void __exit nvmf_exit(void)
 	class_destroy(nvmf_class);
 	nvmf_host_put(nvmf_default_host);
 
+	BUILD_BUG_ON(sizeof(struct nvmf_common_command) != 64);
 	BUILD_BUG_ON(sizeof(struct nvmf_connect_command) != 64);
 	BUILD_BUG_ON(sizeof(struct nvmf_property_get_command) != 64);
 	BUILD_BUG_ON(sizeof(struct nvmf_property_set_command) != 64);
-- 
2.17.1

[PATCH 2/2] nvme: pci: Make sure command struct size fixed

[Minwoo Im]

All the NVMe command has 64bytes fixed size so that it has been assured
with BUILD_BUG_ON().  The remaining command structures in linux/nvme.h
also need to be checked here.

Signed-off-by: Minwoo Im <minwoo.im.dev at gmail.com>
---
 drivers/nvme/host/pci.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index c1eecde6b853..fd43d1a7bd16 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -231,19 +231,26 @@ struct nvme_iod {
  */
 static inline void _nvme_check_size(void)
 {
+	BUILD_BUG_ON(sizeof(struct nvme_common_command) != 64);
 	BUILD_BUG_ON(sizeof(struct nvme_rw_command) != 64);
+	BUILD_BUG_ON(sizeof(struct nvme_identify) != 64);
 	BUILD_BUG_ON(sizeof(struct nvme_create_cq) != 64);
 	BUILD_BUG_ON(sizeof(struct nvme_create_sq) != 64);
 	BUILD_BUG_ON(sizeof(struct nvme_delete_queue) != 64);
 	BUILD_BUG_ON(sizeof(struct nvme_features) != 64);
+	BUILD_BUG_ON(sizeof(struct nvme_download_firmware) != 64);
 	BUILD_BUG_ON(sizeof(struct nvme_format_cmd) != 64);
+	BUILD_BUG_ON(sizeof(struct nvme_dsm_cmd) != 64);
+	BUILD_BUG_ON(sizeof(struct nvme_write_zeroes_cmd) != 64);
 	BUILD_BUG_ON(sizeof(struct nvme_abort_cmd) != 64);
+	BUILD_BUG_ON(sizeof(struct nvme_get_log_page_command) != 64);
 	BUILD_BUG_ON(sizeof(struct nvme_command) != 64);
 	BUILD_BUG_ON(sizeof(struct nvme_id_ctrl) != NVME_IDENTIFY_DATA_SIZE);
 	BUILD_BUG_ON(sizeof(struct nvme_id_ns) != NVME_IDENTIFY_DATA_SIZE);
 	BUILD_BUG_ON(sizeof(struct nvme_lba_range_type) != 64);
 	BUILD_BUG_ON(sizeof(struct nvme_smart_log) != 512);
 	BUILD_BUG_ON(sizeof(struct nvme_dbbuf) != 64);
+	BUILD_BUG_ON(sizeof(struct nvme_directive_cmd) != 64);
 }
 
 static unsigned int max_io_queues(void)
-- 
2.17.1

[PATCH 0/2] nvme: Check size of command structures

[Keith Busch]

On Fri, Apr 12, 2019@12:18:30AM +0900, Minwoo Im wrote:
> Many command structures have been added in linux/nvme.h wihtout
> BUILD_BUG_ON() check.  It also needs to be assured to avoid unintended
> growth of structures.

We're already protected from accidentally increasing a command size since
'struct nvme_command' unionizes them all. Its size would be the largest
command in the union and we do check it's 64 bytes. Unintended shrinkage,
though, that would require individual command type checks, so okay.

[PATCH 0/2] nvme: Check size of command structures

[Minwoo Im]

On 4/12/19 12:27 AM, Keith Busch wrote:
> On Fri, Apr 12, 2019@12:18:30AM +0900, Minwoo Im wrote:
>> Many command structures have been added in linux/nvme.h wihtout
>> BUILD_BUG_ON() check.  It also needs to be assured to avoid unintended
>> growth of structures.
> 
> We're already protected from accidentally increasing a command size since
> 'struct nvme_command' unionizes them all. Its size would be the largest
> command in the union and we do check it's 64 bytes. Unintended shrinkage,
> though, that would require individual command type checks, so okay.
> 

Agreed. struct nvme_command is a largest one in that union. By the way,
remaining structures also need to be checked for the unintended shrinkage.

Thanks,

[PATCH 1/2] nvme: fabrics: Make sure command struct size fixed

[Chaitanya Kulkarni]

Looks good,

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni at wdc.com>

On 4/11/19 8:19 AM, Minwoo Im wrote:
> struct common_command provides a common structure for NVMe-oF command
> format.  It also needs to be checked for unintended size growth.
> 
> Signed-off-by: Minwoo Im <minwoo.im.dev at gmail.com>
> ---
>   drivers/nvme/host/fabrics.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
> index d4cb826f58ff..592d1e61ef7e 100644
> --- a/drivers/nvme/host/fabrics.c
> +++ b/drivers/nvme/host/fabrics.c
> @@ -1188,6 +1188,7 @@ static void __exit nvmf_exit(void)
>   	class_destroy(nvmf_class);
>   	nvmf_host_put(nvmf_default_host);
>   
> +	BUILD_BUG_ON(sizeof(struct nvmf_common_command) != 64);
>   	BUILD_BUG_ON(sizeof(struct nvmf_connect_command) != 64);
>   	BUILD_BUG_ON(sizeof(struct nvmf_property_get_command) != 64);
>   	BUILD_BUG_ON(sizeof(struct nvmf_property_set_command) != 64);
> 

[PATCH 2/2] nvme: pci: Make sure command struct size fixed

[Chaitanya Kulkarni]

Looks good,

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni at wdc.com>

On 4/11/19 8:19 AM, Minwoo Im wrote:
> All the NVMe command has 64bytes fixed size so that it has been assured
> with BUILD_BUG_ON().  The remaining command structures in linux/nvme.h
> also need to be checked here.
> 
> Signed-off-by: Minwoo Im <minwoo.im.dev at gmail.com>
> ---
>   drivers/nvme/host/pci.c | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index c1eecde6b853..fd43d1a7bd16 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -231,19 +231,26 @@ struct nvme_iod {
>    */
>   static inline void _nvme_check_size(void)
>   {
> +	BUILD_BUG_ON(sizeof(struct nvme_common_command) != 64);
>   	BUILD_BUG_ON(sizeof(struct nvme_rw_command) != 64);
> +	BUILD_BUG_ON(sizeof(struct nvme_identify) != 64);
>   	BUILD_BUG_ON(sizeof(struct nvme_create_cq) != 64);
>   	BUILD_BUG_ON(sizeof(struct nvme_create_sq) != 64);
>   	BUILD_BUG_ON(sizeof(struct nvme_delete_queue) != 64);
>   	BUILD_BUG_ON(sizeof(struct nvme_features) != 64);
> +	BUILD_BUG_ON(sizeof(struct nvme_download_firmware) != 64);
>   	BUILD_BUG_ON(sizeof(struct nvme_format_cmd) != 64);
> +	BUILD_BUG_ON(sizeof(struct nvme_dsm_cmd) != 64);
> +	BUILD_BUG_ON(sizeof(struct nvme_write_zeroes_cmd) != 64);
>   	BUILD_BUG_ON(sizeof(struct nvme_abort_cmd) != 64);
> +	BUILD_BUG_ON(sizeof(struct nvme_get_log_page_command) != 64);
>   	BUILD_BUG_ON(sizeof(struct nvme_command) != 64);
>   	BUILD_BUG_ON(sizeof(struct nvme_id_ctrl) != NVME_IDENTIFY_DATA_SIZE);
>   	BUILD_BUG_ON(sizeof(struct nvme_id_ns) != NVME_IDENTIFY_DATA_SIZE);
>   	BUILD_BUG_ON(sizeof(struct nvme_lba_range_type) != 64);
>   	BUILD_BUG_ON(sizeof(struct nvme_smart_log) != 512);
>   	BUILD_BUG_ON(sizeof(struct nvme_dbbuf) != 64);
> +	BUILD_BUG_ON(sizeof(struct nvme_directive_cmd) != 64);
>   }
>   

>   static unsigned int max_io_queues(void)
> 

[PATCH 1/2] nvme: fabrics: Make sure command struct size fixed

[Sagi Grimberg]

Looks fine,

Reviewed-by: Sagi Grimberg <sagi at grimberg.me>

[PATCH 2/2] nvme: pci: Make sure command struct size fixed

[Sagi Grimberg]

Looks fine,

Reviewed-by: Sagi Grimberg <sagi at grimberg.me>

[PATCH 0/2] nvme: Check size of command structures

[Christoph Hellwig]

I'm not sure we are going a little too crazy with all these
BUILD_BUG_ONs.  It would be a lot easier if we could just throw
the assert into the struct defintion somehow, but that would need
compiler magic..

On Fri, Apr 12, 2019@12:18:30AM +0900, Minwoo Im wrote:
> Many command structures have been added in linux/nvme.h wihtout
> BUILD_BUG_ON() check.  It also needs to be assured to avoid unintended
> growth of structures.
> 
> Minwoo Im (2):
>   nvme: fabrics: Make sure command struct size fixed
>   nvme: pci: Make sure command struct size fixed
> 
>  drivers/nvme/host/fabrics.c | 1 +
>  drivers/nvme/host/pci.c     | 7 +++++++
>  2 files changed, 8 insertions(+)
> 
> -- 
> 2.17.1
---end quoted text---

[PATCH 0/2] nvme: Check size of command structures

[Minwoo Im]

On 4/25/19 11:44 PM, Christoph Hellwig wrote:
> I'm not sure we are going a little too crazy with all these
> BUILD_BUG_ONs.  It would be a lot easier if we could just throw
> the assert into the struct defintion somehow, but that would need
> compiler magic..

Agreed.. But we can have it to prevent mis-size issues unless we have
that magic. ;)

Thanks,

[PATCH 0/2] nvme: Check size of command structures

[Bart Van Assche]

On Thu, 2019-04-25@16:44 +0200, Christoph Hellwig wrote:
> I'm not sure we are going a little too crazy with all these
> BUILD_BUG_ONs.  It would be a lot easier if we could just throw
> the assert into the struct defintion somehow, but that would need
> compiler magic..

How about something like the following just after each structure definition?

static inline void nvmf_common_command_size_check(void)
{
        BUILD_BUG_ON(sizeof(struct nvmf_common_command) != 64);
}

Bart.

[PATCH 0/2] nvme: Check size of command structures

[Minwoo Im]

> -----Original Message-----
> From: Linux-nvme [mailto:linux-nvme-bounces at lists.infradead.org] On
> Behalf Of Bart Van Assche
> Sent: Friday, April 26, 2019 12:39 AM
> To: Christoph Hellwig; Minwoo Im
> Cc: Keith Busch; Jens Axboe; Sagi Grimberg; linux-nvme at lists.infradead.org
> Subject: Re: [PATCH 0/2] nvme: Check size of command structures
> 
> On Thu, 2019-04-25@16:44 +0200, Christoph Hellwig wrote:
> > I'm not sure we are going a little too crazy with all these
> > BUILD_BUG_ONs.  It would be a lot easier if we could just throw
> > the assert into the struct defintion somehow, but that would need
> > compiler magic..
> 
> How about something like the following just after each structure definition?
> 
> static inline void nvmf_common_command_size_check(void)
> {
>         BUILD_BUG_ON(sizeof(struct nvmf_common_command) != 64);
> }

Agree with that we don't have to BUILD_BUG_ON() in .c level.  If it's in header,
that would be better than as it is now. 
But, I'm concerning that if we do that, per one structure will have a one static
inline function to check the size of it which might make code too long. ;)

> 
> Bart.
> 
> _______________________________________________
> Linux-nvme mailing list
> Linux-nvme at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-nvme

[PATCH 0/2] nvme: Check size of command structures

[Christoph Hellwig]

Thanks,

applied to nvme-5.2.