[dpdk-dev] [PATCH 1/2] examples/vhost_scsi: add virtio-1.0 feature bit support

[dpdk-dev] [PATCH 1/2] examples/vhost_scsi: add virtio-1.0 feature bit support

[Changpeng Liu]

Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
---
 examples/vhost_scsi/vhost_scsi.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/examples/vhost_scsi/vhost_scsi.c b/examples/vhost_scsi/vhost_scsi.c
index 2908ff6..a1d542b 100644
--- a/examples/vhost_scsi/vhost_scsi.c
+++ b/examples/vhost_scsi/vhost_scsi.c
@@ -20,9 +20,10 @@
 #include "vhost_scsi.h"
 #include "scsi_spec.h"
 
-#define VIRTIO_SCSI_FEATURES ((1 << VIRTIO_F_NOTIFY_ON_EMPTY) |\
-			      (1 << VIRTIO_SCSI_F_INOUT) |\
-			      (1 << VIRTIO_SCSI_F_CHANGE))
+#define VIRTIO_SCSI_FEATURES ((1ULL << VIRTIO_F_NOTIFY_ON_EMPTY) |\
+			      (1ULL << VIRTIO_SCSI_F_INOUT) |\
+			      (1ULL << VIRTIO_SCSI_F_CHANGE) |\
+			      (1ULL << VIRTIO_F_VERSION_1))
 
 /* Path to folder where character device will be created. Can be set by user. */
 static char dev_pathname[PATH_MAX] = "";
-- 
1.9.3

[dpdk-dev] [PATCH 2/2] examples/vhost_scsi: fix potential buffer overrun with safe copy API

[Changpeng Liu]

Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
---
 examples/vhost_scsi/scsi.c       | 23 ++++++++++++-----------
 examples/vhost_scsi/vhost_scsi.c |  5 +++--
 2 files changed, 15 insertions(+), 13 deletions(-)

diff --git a/examples/vhost_scsi/scsi.c b/examples/vhost_scsi/scsi.c
index 0c2fa3e..1572098 100644
--- a/examples/vhost_scsi/scsi.c
+++ b/examples/vhost_scsi/scsi.c
@@ -182,8 +182,8 @@
 			break;
 		case SPC_VPD_UNIT_SERIAL_NUMBER:
 			hlen = 4;
-			strlcpy((char *)vpage->params, bdev->name,
-					sizeof(vpage->params));
+			vhost_strcpy_pad((char *)vpage->params, bdev->name,
+					sizeof(vpage->params), ' ');
 			vpage->alloc_len = rte_cpu_to_be_16(32);
 			break;
 		case SPC_VPD_DEVICE_IDENTIFICATION:
@@ -217,10 +217,11 @@
 			desig->piv = 1;
 			desig->reserved1 = 0;
 			desig->len = 8 + 16 + 32;
-			strlcpy((char *)desig->desig, "INTEL", 8);
+			vhost_strcpy_pad((char *)desig->desig, "INTEL", 8, ' ');
 			vhost_strcpy_pad((char *)&desig->desig[8],
 					 bdev->product_name, 16, ' ');
-			strlcpy((char *)&desig->desig[24], bdev->name, 32);
+			vhost_strcpy_pad((char *)&desig->desig[24], bdev->name,
+					32, ' ');
 			len += sizeof(struct scsi_desig_desc) + 8 + 16 + 32;
 
 			buf += sizeof(struct scsi_desig_desc) + desig->len;
@@ -277,17 +278,17 @@
 		inqdata->flags3 = 0x2;
 
 		/* T10 VENDOR IDENTIFICATION */
-		strlcpy((char *)inqdata->t10_vendor_id, "INTEL",
-			sizeof(inqdata->t10_vendor_id));
+		vhost_strcpy_pad((char *)inqdata->t10_vendor_id, "INTEL",
+			sizeof(inqdata->t10_vendor_id), ' ');
 
 		/* PRODUCT IDENTIFICATION */
-		snprintf((char *)inqdata->product_id,
-				RTE_DIM(inqdata->product_id), "%s",
-				bdev->product_name);
+		vhost_strcpy_pad((char *)inqdata->product_id,
+				bdev->product_name,
+				sizeof(inqdata->product_id), ' ');
 
 		/* PRODUCT REVISION LEVEL */
-		strlcpy((char *)inqdata->product_rev, "0001",
-			sizeof(inqdata->product_rev));
+		vhost_strcpy_pad((char *)inqdata->product_rev, "0001",
+			sizeof(inqdata->product_rev), ' ');
 
 		/* Standard inquiry data ends here. Only populate
 		 * remaining fields if alloc_len indicates enough
diff --git a/examples/vhost_scsi/vhost_scsi.c b/examples/vhost_scsi/vhost_scsi.c
index a1d542b..4e57462 100644
--- a/examples/vhost_scsi/vhost_scsi.c
+++ b/examples/vhost_scsi/vhost_scsi.c
@@ -183,8 +183,9 @@ static uint64_t gpa_to_vva(int vid, uint64_t gpa, uint64_t *len)
 	if (!bdev)
 		return NULL;
 
-	strncpy(bdev->name, bdev_name, sizeof(bdev->name));
-	strncpy(bdev->product_name, bdev_serial, sizeof(bdev->product_name));
+	snprintf(bdev->name, sizeof(bdev->name), "%s", bdev_name);
+	snprintf(bdev->product_name, sizeof(bdev->product_name),
+		"%s", bdev_serial);
 	bdev->blocklen = blk_size;
 	bdev->blockcnt = blk_cnt;
 	bdev->write_cache = wce_enable;
-- 
1.9.3

Re: [dpdk-dev] [PATCH 1/2] examples/vhost_scsi: add virtio-1.0 feature bit support

[Maxime Coquelin]

On 05/18/2018 01:32 AM, Changpeng Liu wrote:
> Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
> ---
>   examples/vhost_scsi/vhost_scsi.c | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime

Re: [dpdk-dev] [PATCH 2/2] examples/vhost_scsi: fix potential buffer overrun with safe copy API

[Thomas Monjalon]

18/05/2018 01:32, Changpeng Liu:
> Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>

Missing explanations.

> -			strlcpy((char *)vpage->params, bdev->name,
> -					sizeof(vpage->params));
> +			vhost_strcpy_pad((char *)vpage->params, bdev->name,
> +					sizeof(vpage->params), ' ');

Why do you think vhost_strcpy_pad is safer than strlcpy?

> -	strncpy(bdev->name, bdev_name, sizeof(bdev->name));
> -	strncpy(bdev->product_name, bdev_serial, sizeof(bdev->product_name));
> +	snprintf(bdev->name, sizeof(bdev->name), "%s", bdev_name);
> +	snprintf(bdev->product_name, sizeof(bdev->product_name),
> +		"%s", bdev_serial);

You should use strlcpy.

Re: [dpdk-dev] [PATCH 1/2] examples/vhost_scsi: add virtio-1.0 feature bit support

[Thomas Monjalon]

18/05/2018 14:35, Maxime Coquelin:
> 
> On 05/18/2018 01:32 AM, Changpeng Liu wrote:
> > Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
> > ---
> >   examples/vhost_scsi/vhost_scsi.c | 7 ++++---
> >   1 file changed, 4 insertions(+), 3 deletions(-)
> 
> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Patch 1 applied alone, thanks

Re: [dpdk-dev] [PATCH 2/2] examples/vhost_scsi: fix potential buffer overrun with safe copy API

[Liu, Changpeng]

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Tuesday, May 22, 2018 10:48 AM
> To: Liu, Changpeng <changpeng.liu@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 2/2] examples/vhost_scsi: fix potential buffer
> overrun with safe copy API
> 
> 18/05/2018 01:32, Changpeng Liu:
> > Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
> 
> Missing explanations.
> 
> > -			strlcpy((char *)vpage->params, bdev->name,
> > -					sizeof(vpage->params));
> > +			vhost_strcpy_pad((char *)vpage->params, bdev->name,
> > +					sizeof(vpage->params), ' ');
> 
> Why do you think vhost_strcpy_pad is safer than strlcpy?
A code Coverity issue 279452 reported for strlcpy, so here replace with internal API can avoid it.
> 
> > -	strncpy(bdev->name, bdev_name, sizeof(bdev->name));
> > -	strncpy(bdev->product_name, bdev_serial, sizeof(bdev->product_name));
> > +	snprintf(bdev->name, sizeof(bdev->name), "%s", bdev_name);
> > +	snprintf(bdev->product_name, sizeof(bdev->product_name),
> > +		"%s", bdev_serial);
> 
> You should use strlcpy.
> 

Re: [dpdk-dev] [PATCH 2/2] examples/vhost_scsi: fix potential buffer overrun with safe copy API

[Thomas Monjalon]

22/05/2018 19:58, Liu, Changpeng:
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > 18/05/2018 01:32, Changpeng Liu:
> > > -			strlcpy((char *)vpage->params, bdev->name,
> > > -					sizeof(vpage->params));
> > > +			vhost_strcpy_pad((char *)vpage->params, bdev->name,
> > > +					sizeof(vpage->params), ' ');
> > 
> > Why do you think vhost_strcpy_pad is safer than strlcpy?
> 
> A code Coverity issue 279452 reported for strlcpy, so here replace with internal API can avoid it.

I think it is a false positive.
Remember that Coverity is just a tool.

Re: [dpdk-dev] [PATCH 1/2] examples/vhost_scsi: add virtio-1.0 feature bit support

[Thomas Monjalon]

22/05/2018 19:51, Thomas Monjalon:
> 18/05/2018 14:35, Maxime Coquelin:
> > 
> > On 05/18/2018 01:32 AM, Changpeng Liu wrote:
> > > Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
> > > ---
> > >   examples/vhost_scsi/vhost_scsi.c | 7 ++++---
> > >   1 file changed, 4 insertions(+), 3 deletions(-)
> > 
> > Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> 
> Patch 1 applied alone, thanks

After compilation tests, the patch is not accepted in 18.05,
because VIRTIO_F_VERSION_1 seems not defined on some distributions,
despite the fallback implemented.

Re: [dpdk-dev] [PATCH 1/2] examples/vhost_scsi: add virtio-1.0 feature bit support

[Liu, Changpeng]

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Tuesday, May 22, 2018 1:33 PM
> To: Liu, Changpeng <changpeng.liu@intel.com>
> Cc: dev@dpdk.org; Maxime Coquelin <maxime.coquelin@redhat.com>
> Subject: Re: [dpdk-dev] [PATCH 1/2] examples/vhost_scsi: add virtio-1.0 feature
> bit support
> 
> 22/05/2018 19:51, Thomas Monjalon:
> > 18/05/2018 14:35, Maxime Coquelin:
> > >
> > > On 05/18/2018 01:32 AM, Changpeng Liu wrote:
> > > > Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
> > > > ---
> > > >   examples/vhost_scsi/vhost_scsi.c | 7 ++++---
> > > >   1 file changed, 4 insertions(+), 3 deletions(-)
> > >
> > > Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> >
> > Patch 1 applied alone, thanks
> 
> After compilation tests, the patch is not accepted in 18.05,
> because VIRTIO_F_VERSION_1 seems not defined on some distributions,
> despite the fallback implemented.

What a mess for vhost library. :).
lib/librte_vhost/vhost.h has the definition VIRTIO_F_VERSION_1 for some older kernel, 
but didn't include in the public API header <rte_vhost.h>, looks like we should move
this definition to rte_vhost.h for some old kernels.
>