[dpdk-dev] [PATCH 00/18] Fix build on gcc8 and various bugs

[dpdk-dev] [PATCH 00/18] Fix build on gcc8 and various bugs

[Andy Green]

The following series gets current master able to build
on Fedora 28 + x86_64 using gcc 8.0.1.  There were a
number of build-breaking problems, mostly around correct
string processing but all of them reflecting some issue
in the code.

Of the two major issues, one around struct alignment in
dpaa I think is properly fixed by the patch 3/18 here,
but for the apparently broken hash function cast in 1/18
I just stopped it breaking the build; unless I miss the
trick it seems it needs fixing properly after some
further discussion.

Because of the kind of widespread string processing /
off-by-one / buffer overflow issue fixed here, I think it'd
be a really good idea to run this stuff through Coverity.

They will give you a free account for OSS projects here

https://scan.coverity.com/

---

Andy Green (18):
      lib: ret_table: workaround hash function cast error
      drivers: bus: pci: fix strncpy dangerous code
      drivers: bus: dpaa: fix inconsistent struct alignment
      drivers: net: axgbe: fix broken eeprom string comp
      drivers: net: nfp: nfpcore: fix strncpy misuse
      drivers: net: nfp: nfpcore fix off-by-one and no NUL on strncpy use
      drivers: net: nfp: don't memcpy out of source range
      drivers: net: nfp: fix buffer overflow in fw_name
      drivers: net: qede: fix strncpy constant and NUL
      drivers: net: qede: fix broken strncpy
      drivers:net:sfc: fix strncpy length
      drivers: net: sfc: fix another strncpy size and NUL
      drivers: net: vdev: readlink inputs cannot be aliased
      drivers: net: vdev: fix 3 x strncpy misuse
      test-pmd: can't find include
      app: fix sprintf overrun bug
      app: test-bbdev: strcpy ok for allocated string
      app: test-bbdev: strcpy ok for allocated string 2


 app/proc-info/main.c                       |    9 +++++++--
 app/test-bbdev/test_bbdev_vector.c         |    5 +++--
 app/test-pmd/Makefile                      |    1 +
 drivers/bus/dpaa/base/qbman/qman.c         |   14 +++++++-------
 drivers/bus/dpaa/include/fsl_qman.h        |   24 +++++++++++++-----------
 drivers/bus/pci/linux/pci.c                |    3 ++-
 drivers/net/axgbe/axgbe_phy_impl.c         |    4 ++--
 drivers/net/nfp/nfp_net.c                  |    4 ++--
 drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c |    3 ++-
 drivers/net/nfp/nfpcore/nfp_resource.c     |    6 +++---
 drivers/net/qede/base/ecore_int.c          |   10 ++++++----
 drivers/net/qede/qede_main.c               |    3 ++-
 drivers/net/sfc/sfc_ethdev.c               |    9 ++++++---
 drivers/net/vdev_netvsc/vdev_netvsc.c      |   16 ++++++++++------
 lib/librte_table/rte_table_hash_cuckoo.c   |    2 +-
 15 files changed, 67 insertions(+), 46 deletions(-)

--

[dpdk-dev] [PATCH 01/18] lib: ret_table: workaround hash function cast error

[Andy Green]

/home/agreen/projects/dpdk/lib/librte_table/rte_table_hash_cuckoo.c:110:16: error: cast between incompatible function types from ‘rte_table_hash_op_hash’ {aka ‘long unsigned int (*)(void *, void *, unsigned int,  long unsigned int)’} to ‘uint32_t (*)(const void *, uint32_t,  uint32_t)’ {aka ‘unsigned int (*)(const void *, unsigned int,  unsigned int)’} [-Werror=cast-function-type]
   .hash_func = (rte_hash_function)(p->f_hash),

The code seems to be quite broken.

It's casting this

typedef uint64_t (*rte_table_hash_op_hash)(
        void *key,
        void *key_mask,
        uint32_t key_size,
        uint64_t seed);

to this

typedef uint32_t (*rte_hash_function)(const void *key, uint32_t key_len,
                                      uint32_t init_val);

if the definition with 4 args is later called with a pointer
giving it three args, obviously it working is just an accident.  I
grepped around a bit and could not see it being cast back to
the original type before use; the uses I saw have three args.

I simply patch it to stop the build breaking, rather than fix it,
since I am not sure what a fix should look like considering the
whole code.
---
 lib/librte_table/rte_table_hash_cuckoo.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_table/rte_table_hash_cuckoo.c b/lib/librte_table/rte_table_hash_cuckoo.c
index dcb4fe978..eca72b506 100644
--- a/lib/librte_table/rte_table_hash_cuckoo.c
+++ b/lib/librte_table/rte_table_hash_cuckoo.c
@@ -107,7 +107,7 @@ rte_table_hash_cuckoo_create(void *params,
 	struct rte_hash_parameters hash_cuckoo_params = {
 		.entries = p->n_keys,
 		.key_len = p->key_size,
-		.hash_func = (rte_hash_function)(p->f_hash),
+		.hash_func = (rte_hash_function)(void *)(p->f_hash),
 		.hash_func_init_val = p->seed,
 		.socket_id = socket_id,
 		.name = p->name

[dpdk-dev] [PATCH 02/18] drivers: bus: pci: fix strncpy dangerous code

[Andy Green]

In function ‘pci_get_kernel_driver_by_path’,
    inlined from ‘pci_scan_one.isra.1’ at /home/agreen/projects/dpdk/drivers/bus/pci/linux/pci.c:317:8:
/home/agreen/projects/dpdk/drivers/bus/pci/linux/pci.c:57:3: error: ‘strncpy’ specified bound depends on the length of the source argument [-Werror=stringop-overflow=]
   strncpy(dri_name, name + 1, strlen(name + 1) + 1);
---
 drivers/bus/pci/linux/pci.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/bus/pci/linux/pci.c b/drivers/bus/pci/linux/pci.c
index 4630a8057..b5bdfd33e 100644
--- a/drivers/bus/pci/linux/pci.c
+++ b/drivers/bus/pci/linux/pci.c
@@ -54,7 +54,8 @@ pci_get_kernel_driver_by_path(const char *filename, char *dri_name)
 
 	name = strrchr(path, '/');
 	if (name) {
-		strncpy(dri_name, name + 1, strlen(name + 1) + 1);
+		strncpy(dri_name, name + 1, sizeof(dri_name) - 1);
+		dri_name[sizeof(dri_name) - 1] = '\0';
 		return 0;
 	}
 

Re: [dpdk-dev] [PATCH 02/18] drivers: bus: pci: fix strncpy dangerous code

[Bruce Richardson]

On Tue, May 08, 2018 at 12:29:38PM +0800, Andy Green wrote:
> In function ‘pci_get_kernel_driver_by_path’,
>     inlined from ‘pci_scan_one.isra.1’ at /home/agreen/projects/dpdk/drivers/bus/pci/linux/pci.c:317:8:
> /home/agreen/projects/dpdk/drivers/bus/pci/linux/pci.c:57:3: error: ‘strncpy’ specified bound depends on the length of the source argument [-Werror=stringop-overflow=]
>    strncpy(dri_name, name + 1, strlen(name + 1) + 1);
> ---
>  drivers/bus/pci/linux/pci.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/bus/pci/linux/pci.c b/drivers/bus/pci/linux/pci.c
> index 4630a8057..b5bdfd33e 100644
> --- a/drivers/bus/pci/linux/pci.c
> +++ b/drivers/bus/pci/linux/pci.c
> @@ -54,7 +54,8 @@ pci_get_kernel_driver_by_path(const char *filename, char *dri_name)
>  
>  	name = strrchr(path, '/');
>  	if (name) {
> -		strncpy(dri_name, name + 1, strlen(name + 1) + 1);
> +		strncpy(dri_name, name + 1, sizeof(dri_name) - 1);
> +		dri_name[sizeof(dri_name) - 1] = '\0';
>  		return 0;
>  	}
While this fix is correct, a better fix would be to use strlcpy from
rte_string_fns.h.

	strlcpy(dri_name, name + 1, sizeof(dri_name));

Regards,
/Bruce

[dpdk-dev] [PATCH 03/18] drivers: bus: dpaa: fix inconsistent struct alignment

[Andy Green]

The actual descriptor for qm_mr_entry is 64-byte aligned.

But the original code plays a trick, and puts a u8 common to the
three descriptor subtypes in the union afterwards outside their
structure definitions.

Unfortunately since they compose a struct qm_fd with alignment
8, this trick destroys the ability of the compiler to understand
what has happened, resulting in this kind of problem:

/home/agreen/projects/dpdk/drivers/bus/dpaa/include/fsl_qman.h:354:3: error: alignment 1 of ‘struct <anonymous>’ is less than 8 [-Werror=packed-not-aligned]
   } __packed dcern;

on gcc 8 / Fedora 28 out of the box.

This patch moves the u8 verb into the structure definitions composed into the
union, so the alignment of the parent struct containing the alignment 8 object
can also be seen to be alignment 8 by the compiler.  Uses of .verb are fixed
up to use .ern.verb (the same offset of +0 inside all the structs in the
union).

The final struct layout should be unchanged.
---
 drivers/bus/dpaa/base/qbman/qman.c  |   14 +++++++-------
 drivers/bus/dpaa/include/fsl_qman.h |   24 +++++++++++++-----------
 2 files changed, 20 insertions(+), 18 deletions(-)

diff --git a/drivers/bus/dpaa/base/qbman/qman.c b/drivers/bus/dpaa/base/qbman/qman.c
index 2810fdd26..27d98cc10 100644
--- a/drivers/bus/dpaa/base/qbman/qman.c
+++ b/drivers/bus/dpaa/base/qbman/qman.c
@@ -314,9 +314,9 @@ static int drain_mr_fqrni(struct qm_portal *p)
 		if (!msg)
 			return 0;
 	}
-	if ((msg->verb & QM_MR_VERB_TYPE_MASK) != QM_MR_VERB_FQRNI) {
+	if ((msg->ern.verb & QM_MR_VERB_TYPE_MASK) != QM_MR_VERB_FQRNI) {
 		/* We aren't draining anything but FQRNIs */
-		pr_err("Found verb 0x%x in MR\n", msg->verb);
+		pr_err("Found verb 0x%x in MR\n", msg->ern.verb);
 		return -1;
 	}
 	qm_mr_next(p);
@@ -483,7 +483,7 @@ static inline void qm_mr_pvb_update(struct qm_portal *portal)
 	/* when accessing 'verb', use __raw_readb() to ensure that compiler
 	 * inlining doesn't try to optimise out "excess reads".
 	 */
-	if ((__raw_readb(&res->verb) & QM_MR_VERB_VBIT) == mr->vbit) {
+	if ((__raw_readb(&res->ern.verb) & QM_MR_VERB_VBIT) == mr->vbit) {
 		mr->pi = (mr->pi + 1) & (QM_MR_SIZE - 1);
 		if (!mr->pi)
 			mr->vbit ^= QM_MR_VERB_VBIT;
@@ -832,7 +832,7 @@ static u32 __poll_portal_slow(struct qman_portal *p, u32 is)
 			goto mr_done;
 		swapped_msg = *msg;
 		hw_fd_to_cpu(&swapped_msg.ern.fd);
-		verb = msg->verb & QM_MR_VERB_TYPE_MASK;
+		verb = msg->ern.verb & QM_MR_VERB_TYPE_MASK;
 		/* The message is a software ERN iff the 0x20 bit is set */
 		if (verb & 0x20) {
 			switch (verb) {
@@ -1666,7 +1666,7 @@ int qman_retire_fq(struct qman_fq *fq, u32 *flags)
 			 */
 			struct qm_mr_entry msg;
 
-			msg.verb = QM_MR_VERB_FQRNI;
+			msg.ern.verb = QM_MR_VERB_FQRNI;
 			msg.fq.fqs = mcr->alterfq.fqs;
 			msg.fq.fqid = fq->fqid;
 #ifdef CONFIG_FSL_QMAN_FQ_LOOKUP
@@ -2643,7 +2643,7 @@ int qman_shutdown_fq(u32 fqid)
 				qm_mr_pvb_update(low_p);
 				msg = qm_mr_current(low_p);
 				while (msg) {
-					if ((msg->verb &
+					if ((msg->ern.verb &
 					     QM_MR_VERB_TYPE_MASK)
 					    == QM_MR_VERB_FQRN)
 						found_fqrn = 1;
@@ -2711,7 +2711,7 @@ int qman_shutdown_fq(u32 fqid)
 			qm_mr_pvb_update(low_p);
 			msg = qm_mr_current(low_p);
 			while (msg) {
-				if ((msg->verb & QM_MR_VERB_TYPE_MASK) ==
+				if ((msg->ern.verb & QM_MR_VERB_TYPE_MASK) ==
 				    QM_MR_VERB_FQRL)
 					orl_empty = 1;
 				qm_mr_next(low_p);
diff --git a/drivers/bus/dpaa/include/fsl_qman.h b/drivers/bus/dpaa/include/fsl_qman.h
index e9793f30d..cf6dfce11 100644
--- a/drivers/bus/dpaa/include/fsl_qman.h
+++ b/drivers/bus/dpaa/include/fsl_qman.h
@@ -284,20 +284,20 @@ static inline dma_addr_t qm_sg_addr(const struct qm_sg_entry *sg)
 	} while (0)
 
 /* See 1.5.8.1: "Enqueue Command" */
-struct qm_eqcr_entry {
+struct __rte_aligned(8) qm_eqcr_entry {
 	u8 __dont_write_directly__verb;
 	u8 dca;
 	u16 seqnum;
 	u32 orp;	/* 24-bit */
 	u32 fqid;	/* 24-bit */
 	u32 tag;
-	struct qm_fd fd;
+	struct qm_fd fd; /* this has alignment 8 */
 	u8 __reserved3[32];
 } __packed;
 
 
 /* "Frame Dequeue Response" */
-struct qm_dqrr_entry {
+struct __rte_aligned(8) qm_dqrr_entry {
 	u8 verb;
 	u8 stat;
 	u16 seqnum;	/* 15-bit */
@@ -305,7 +305,7 @@ struct qm_dqrr_entry {
 	u8 __reserved2[3];
 	u32 fqid;	/* 24-bit */
 	u32 contextB;
-	struct qm_fd fd;
+	struct qm_fd fd; /* this has alignment 8 */
 	u8 __reserved4[32];
 };
 
@@ -323,18 +323,19 @@ struct qm_dqrr_entry {
 /* "ERN Message Response" */
 /* "FQ State Change Notification" */
 struct qm_mr_entry {
-	u8 verb;
 	union {
 		struct {
+			u8 verb;
 			u8 dca;
 			u16 seqnum;
 			u8 rc;		/* Rejection Code */
 			u32 orp:24;
 			u32 fqid;	/* 24-bit */
 			u32 tag;
-			struct qm_fd fd;
-		} __packed ern;
+			struct qm_fd fd; /* this has alignment 8 */
+		} __packed __rte_aligned(8) ern;
 		struct {
+			u8 verb;
 #if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
 			u8 colour:2;	/* See QM_MR_DCERN_COLOUR_* */
 			u8 __reserved1:4;
@@ -349,18 +350,19 @@ struct qm_mr_entry {
 			u32 __reserved3:24;
 			u32 fqid;	/* 24-bit */
 			u32 tag;
-			struct qm_fd fd;
-		} __packed dcern;
+			struct qm_fd fd; /* this has alignment 8 */
+		} __packed __rte_aligned(8) dcern;
 		struct {
+			u8 verb;
 			u8 fqs;		/* Frame Queue Status */
 			u8 __reserved1[6];
 			u32 fqid;	/* 24-bit */
 			u32 contextB;
 			u8 __reserved2[16];
-		} __packed fq;		/* FQRN/FQRNI/FQRL/FQPN */
+		} __packed __rte_aligned(8) fq;		/* FQRN/FQRNI/FQRL/FQPN */
 	};
 	u8 __reserved2[32];
-} __packed;
+} __packed __rte_aligned(8);
 #define QM_MR_VERB_VBIT			0x80
 /*
  * ERNs originating from direct-connect portals ("dcern") use 0x20 as a verb

[dpdk-dev] [PATCH 04/18] drivers: net: axgbe: fix broken eeprom string comp

[Andy Green]

/home/agreen/projects/dpdk/drivers/net/axgbe/axgbe_phy_impl.c:576:6: error: ‘__builtin_memcmp_eq’ reading 16 bytes from a region of size 9 [-Werror=stringop-overflow=]
  if (memcmp(&sfp_eeprom->base[AXGBE_SFP_BASE_VENDOR_NAME],
      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      AXGBE_BEL_FUSE_VENDOR, AXGBE_SFP_BASE_VENDOR_NAME_LEN))
---
 drivers/net/axgbe/axgbe_phy_impl.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/axgbe/axgbe_phy_impl.c b/drivers/net/axgbe/axgbe_phy_impl.c
index dfa908dd8..973177f69 100644
--- a/drivers/net/axgbe/axgbe_phy_impl.c
+++ b/drivers/net/axgbe/axgbe_phy_impl.c
@@ -574,11 +574,11 @@ static bool axgbe_phy_belfuse_parse_quirks(struct axgbe_port *pdata)
 	struct axgbe_sfp_eeprom *sfp_eeprom = &phy_data->sfp_eeprom;
 
 	if (memcmp(&sfp_eeprom->base[AXGBE_SFP_BASE_VENDOR_NAME],
-		   AXGBE_BEL_FUSE_VENDOR, AXGBE_SFP_BASE_VENDOR_NAME_LEN))
+		   AXGBE_BEL_FUSE_VENDOR, strlen(AXGBE_BEL_FUSE_VENDOR)))
 		return false;
 
 	if (!memcmp(&sfp_eeprom->base[AXGBE_SFP_BASE_VENDOR_PN],
-		    AXGBE_BEL_FUSE_PARTNO, AXGBE_SFP_BASE_VENDOR_PN_LEN)) {
+		    AXGBE_BEL_FUSE_PARTNO, strlen(AXGBE_BEL_FUSE_PARTNO))) {
 		phy_data->sfp_base = AXGBE_SFP_BASE_1000_SX;
 		phy_data->sfp_cable = AXGBE_SFP_CABLE_ACTIVE;
 		phy_data->sfp_speed = AXGBE_SFP_SPEED_1000;

[dpdk-dev] [PATCH 05/18] drivers: net: nfp: nfpcore: fix strncpy misuse

[Andy Green]

---
 drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c b/drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c
index 4e6c66624..9f6704a7f 100644
--- a/drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c
+++ b/drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c
@@ -846,7 +846,8 @@ nfp6000_init(struct nfp_cpp *cpp, const char *devname)
 
 
 	memset(desc->busdev, 0, BUSDEV_SZ);
-	strncpy(desc->busdev, devname, strlen(devname));
+	strncpy(desc->busdev, devname, sizeof(desc->busdev) - 1);
+	desc->busdev[sizeof(desc->busdev) - 1] = '\0';
 
 	ret = nfp_acquire_process_lock(desc);
 	if (ret)

Re: [dpdk-dev] [PATCH 05/18] drivers: net: nfp: nfpcore: fix strncpy misuse

[Bruce Richardson]

On Tue, May 08, 2018 at 12:29:53PM +0800, Andy Green wrote:
> 
> ---
>  drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c b/drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c
> index 4e6c66624..9f6704a7f 100644
> --- a/drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c
> +++ b/drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c
> @@ -846,7 +846,8 @@ nfp6000_init(struct nfp_cpp *cpp, const char *devname)
>  
>  
>  	memset(desc->busdev, 0, BUSDEV_SZ);
> -	strncpy(desc->busdev, devname, strlen(devname));
> +	strncpy(desc->busdev, devname, sizeof(desc->busdev) - 1);
> +	desc->busdev[sizeof(desc->busdev) - 1] = '\0';
>  
>  	ret = nfp_acquire_process_lock(desc);
>  	if (ret)
As with previous patch, a better fix is to use strlcpy. This would apply to
just about all uses of strncpy in the code.

/Bruce

Re: [dpdk-dev] [PATCH 05/18] drivers: net: nfp: nfpcore: fix strncpy misuse

[Andy Green]

On 05/08/2018 04:58 PM, Bruce Richardson wrote:
> On Tue, May 08, 2018 at 12:29:53PM +0800, Andy Green wrote:
>>
>> ---
>>   drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c |    3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c b/drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c
>> index 4e6c66624..9f6704a7f 100644
>> --- a/drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c
>> +++ b/drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c
>> @@ -846,7 +846,8 @@ nfp6000_init(struct nfp_cpp *cpp, const char *devname)
>>   
>>   
>>   	memset(desc->busdev, 0, BUSDEV_SZ);
>> -	strncpy(desc->busdev, devname, strlen(devname));
>> +	strncpy(desc->busdev, devname, sizeof(desc->busdev) - 1);
>> +	desc->busdev[sizeof(desc->busdev) - 1] = '\0';
>>   
>>   	ret = nfp_acquire_process_lock(desc);
>>   	if (ret)
> As with previous patch, a better fix is to use strlcpy. This would apply to
> just about all uses of strncpy in the code.

OK.

But the strncpy() was already there, it's not introduced by the patch.

I agree just doing it in one hit with strlcpy() is nicer.

-Andy

> /Bruce
> 

Re: [dpdk-dev] [PATCH 05/18] drivers: net: nfp: nfpcore: fix strncpy misuse

[Bruce Richardson]

On Tue, May 08, 2018 at 05:00:21PM +0800, Andy Green wrote:
> 
> 
> On 05/08/2018 04:58 PM, Bruce Richardson wrote:
> > On Tue, May 08, 2018 at 12:29:53PM +0800, Andy Green wrote:
> > > 
> > > ---
> > >   drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c |    3 ++-
> > >   1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c b/drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c
> > > index 4e6c66624..9f6704a7f 100644
> > > --- a/drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c
> > > +++ b/drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c
> > > @@ -846,7 +846,8 @@ nfp6000_init(struct nfp_cpp *cpp, const char *devname)
> > >   	memset(desc->busdev, 0, BUSDEV_SZ);
> > > -	strncpy(desc->busdev, devname, strlen(devname));
> > > +	strncpy(desc->busdev, devname, sizeof(desc->busdev) - 1);
> > > +	desc->busdev[sizeof(desc->busdev) - 1] = '\0';
> > >   	ret = nfp_acquire_process_lock(desc);
> > >   	if (ret)
> > As with previous patch, a better fix is to use strlcpy. This would apply to
> > just about all uses of strncpy in the code.
> 
> OK.
> 
> But the strncpy() was already there, it's not introduced by the patch.
> 
> I agree just doing it in one hit with strlcpy() is nicer.
> 
> -Andy
> 
Thanks for the fixes!

[dpdk-dev] [PATCH 06/18] drivers: net: nfp: nfpcore fix off-by-one and no NUL on strncpy use

[Andy Green]

/home/agreen/projects/dpdk/drivers/net/nfp/nfpcore/nfp_resource.c:76:2: error: ‘strncpy’ output may be truncated copying 8 bytes from a string of length 8 [-Werror=stringop-truncation]
  strncpy(name_pad, res->name, sizeof(name_pad));
---
 drivers/net/nfp/nfpcore/nfp_resource.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/nfp/nfpcore/nfp_resource.c b/drivers/net/nfp/nfpcore/nfp_resource.c
index e1df2b2e1..54c6dcf54 100644
--- a/drivers/net/nfp/nfpcore/nfp_resource.c
+++ b/drivers/net/nfp/nfpcore/nfp_resource.c
@@ -65,15 +65,15 @@ struct nfp_resource {
 static int
 nfp_cpp_resource_find(struct nfp_cpp *cpp, struct nfp_resource *res)
 {
-	char name_pad[NFP_RESOURCE_ENTRY_NAME_SZ] = {};
+	char name_pad[NFP_RESOURCE_ENTRY_NAME_SZ + 2];
 	struct nfp_resource_entry entry;
 	uint32_t cpp_id, key;
 	int ret, i;
 
 	cpp_id = NFP_CPP_ID(NFP_RESOURCE_TBL_TARGET, 3, 0);  /* Atomic read */
 
-	memset(name_pad, 0, NFP_RESOURCE_ENTRY_NAME_SZ);
-	strncpy(name_pad, res->name, sizeof(name_pad));
+	memset(name_pad, 0, sizeof(name_pad));
+	strncpy(name_pad, res->name, sizeof(name_pad) - 1);
 
 	/* Search for a matching entry */
 	if (!memcmp(name_pad, NFP_RESOURCE_TBL_NAME "\0\0\0\0\0\0\0\0", 8)) {

[dpdk-dev] [PATCH 07/18] drivers: net: nfp: don't memcpy out of source range

[Andy Green]

/home/agreen/projects/dpdk/drivers/net/nfp/nfp_net.c:669:2: error: ‘memcpy’ forming offset [5, 6] is out of the bounds [0, 4] of object ‘tmp’ with type ‘uint32_t’ {aka ‘unsigned int’} [-Werror=array-bounds]
  memcpy(&hw->mac_addr[0], &tmp, sizeof(struct ether_addr));
---
 drivers/net/nfp/nfp_net.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/nfp/nfp_net.c b/drivers/net/nfp/nfp_net.c
index 048324ec9..199aac40b 100644
--- a/drivers/net/nfp/nfp_net.c
+++ b/drivers/net/nfp/nfp_net.c
@@ -666,7 +666,7 @@ nfp_net_vf_read_mac(struct nfp_net_hw *hw)
 	uint32_t tmp;
 
 	tmp = rte_be_to_cpu_32(nn_cfg_readl(hw, NFP_NET_CFG_MACADDR));
-	memcpy(&hw->mac_addr[0], &tmp, sizeof(struct ether_addr));
+	memcpy(&hw->mac_addr[0], &tmp, 4);
 
 	tmp = rte_be_to_cpu_32(nn_cfg_readl(hw, NFP_NET_CFG_MACADDR + 4));
 	memcpy(&hw->mac_addr[4], &tmp, 2);

[dpdk-dev] [PATCH 08/18] drivers: net: nfp: fix buffer overflow in fw_name

[Andy Green]

/home/agreen/projects/dpdk/drivers/net/nfp/nfp_net.c: In function ‘nfp_pf_pci_probe’:
/home/agreen/projects/dpdk/drivers/net/nfp/nfp_net.c:3160:23: error: ‘%s’ directive writing up to 99 bytes into a region of size 76 [-Werror=format-overflow=]
  sprintf(fw_name, "%s/%s.nffw", DEFAULT_FW_PATH, serial);
---
 drivers/net/nfp/nfp_net.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/nfp/nfp_net.c b/drivers/net/nfp/nfp_net.c
index 199aac40b..d5f0e54e8 100644
--- a/drivers/net/nfp/nfp_net.c
+++ b/drivers/net/nfp/nfp_net.c
@@ -3144,7 +3144,7 @@ nfp_fw_upload(struct rte_pci_device *dev, struct nfp_nsp *nsp, char *card)
 	struct nfp_cpp *cpp = nsp->cpp;
 	int fw_f;
 	char *fw_buf;
-	char fw_name[100];
+	char fw_name[130];
 	char serial[100];
 	struct stat file_stat;
 	off_t fsize, bytes;

[dpdk-dev] [PATCH 09/18] drivers: net: qede: fix strncpy constant and NUL

[Andy Green]

---
 drivers/net/qede/base/ecore_int.c |   10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/net/qede/base/ecore_int.c b/drivers/net/qede/base/ecore_int.c
index f43781ba4..c809d84ef 100644
--- a/drivers/net/qede/base/ecore_int.c
+++ b/drivers/net/qede/base/ecore_int.c
@@ -1103,10 +1103,12 @@ static enum _ecore_status_t ecore_int_deassertion(struct ecore_hwfn *p_hwfn,
 						OSAL_SNPRINTF(bit_name, 30,
 							      p_aeu->bit_name,
 							      num);
-					else
-						OSAL_STRNCPY(bit_name,
-							     p_aeu->bit_name,
-							     30);
+					else {
+						strncpy(bit_name,
+							p_aeu->bit_name,
+							sizeof(bit_name) - 1);
+						bit_name[sizeof(bit_name) - 1] = '\0';
+					}
 
 					/* We now need to pass bitmask in its
 					 * correct position.

Re: [dpdk-dev] [PATCH 09/18] drivers: net: qede: fix strncpy constant and NUL

[Shaikh, Shahed]

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Andy Green
> Sent: Monday, May 7, 2018 11:30 PM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH 09/18] drivers: net: qede: fix strncpy constant and
> NUL
> 
> 
> ---
>  drivers/net/qede/base/ecore_int.c |   10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/qede/base/ecore_int.c
> b/drivers/net/qede/base/ecore_int.c
> index f43781ba4..c809d84ef 100644
> --- a/drivers/net/qede/base/ecore_int.c
> +++ b/drivers/net/qede/base/ecore_int.c
> @@ -1103,10 +1103,12 @@ static enum _ecore_status_t
> ecore_int_deassertion(struct ecore_hwfn *p_hwfn,
>  						OSAL_SNPRINTF(bit_name, 30,
>  							      p_aeu->bit_name,
>  							      num);
> -					else
> -						OSAL_STRNCPY(bit_name,
> -							     p_aeu->bit_name,
> -							     30);
> +					else {
> +						strncpy(bit_name,
> +							p_aeu->bit_name,
> +							sizeof(bit_name) - 1);
> +						bit_name[sizeof(bit_name) - 1]
> = '\0';
> +					}

I think you can retain OSAL_STRNCPY and just replace 30 with 'bit_name[sizeof(bit_name) - 1'  and then set last byte with '\0' just like you did.

Thanks,
Shahed
> 
>  					/* We now need to pass bitmask in its
>  					 * correct position.

Re: [dpdk-dev] [PATCH 09/18] drivers: net: qede: fix strncpy constant and NUL

[Bruce Richardson]

On Tue, May 08, 2018 at 05:59:47PM +0000, dev-bounces@dpdk.org wrote:
> 
> 
> > -----Original Message-----
> > From: dev <dev-bounces@dpdk.org> On Behalf Of Andy Green
> > Sent: Monday, May 7, 2018 11:30 PM
> > To: dev@dpdk.org
> > Subject: [dpdk-dev] [PATCH 09/18] drivers: net: qede: fix strncpy constant and
> > NUL
> > 
> > 
> > ---
> >  drivers/net/qede/base/ecore_int.c |   10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/net/qede/base/ecore_int.c
> > b/drivers/net/qede/base/ecore_int.c
> > index f43781ba4..c809d84ef 100644
> > --- a/drivers/net/qede/base/ecore_int.c
> > +++ b/drivers/net/qede/base/ecore_int.c
> > @@ -1103,10 +1103,12 @@ static enum _ecore_status_t
> > ecore_int_deassertion(struct ecore_hwfn *p_hwfn,
> >  						OSAL_SNPRINTF(bit_name, 30,
> >  							      p_aeu->bit_name,
> >  							      num);
> > -					else
> > -						OSAL_STRNCPY(bit_name,
> > -							     p_aeu->bit_name,
> > -							     30);
> > +					else {
> > +						strncpy(bit_name,
> > +							p_aeu->bit_name,
> > +							sizeof(bit_name) - 1);
> > +						bit_name[sizeof(bit_name) - 1]
> > = '\0';
> > +					}
> 
> I think you can retain OSAL_STRNCPY and just replace 30 with 'bit_name[sizeof(bit_name) - 1'  and then set last byte with '\0' just like you did.

Can that actually be fixed inside OSAL_STRNCPY itself, rather than having
each use needing to explicitly null-terminate?

Re: [dpdk-dev] [PATCH 09/18] drivers: net: qede: fix strncpy constant and NUL

[Shaikh, Shahed]

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Bruce Richardson
> Sent: Tuesday, May 8, 2018 2:53 PM
> To: dev-bounces@dpdk.org
> Cc: Andy Green <andy@warmcat.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 09/18] drivers: net: qede: fix strncpy constant
> and NUL
> 
> On Tue, May 08, 2018 at 05:59:47PM +0000, dev-bounces@dpdk.org wrote:
> >
> >
> > > -----Original Message-----
> > > From: dev <dev-bounces@dpdk.org> On Behalf Of Andy Green
> > > Sent: Monday, May 7, 2018 11:30 PM
> > > To: dev@dpdk.org
> > > Subject: [dpdk-dev] [PATCH 09/18] drivers: net: qede: fix strncpy
> > > constant and NUL
> > >
> > >
> > > ---
> > >  drivers/net/qede/base/ecore_int.c |   10 ++++++----
> > >  1 file changed, 6 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/net/qede/base/ecore_int.c
> > > b/drivers/net/qede/base/ecore_int.c
> > > index f43781ba4..c809d84ef 100644
> > > --- a/drivers/net/qede/base/ecore_int.c
> > > +++ b/drivers/net/qede/base/ecore_int.c
> > > @@ -1103,10 +1103,12 @@ static enum _ecore_status_t
> > > ecore_int_deassertion(struct ecore_hwfn *p_hwfn,
> > >  						OSAL_SNPRINTF(bit_name, 30,
> > >  							      p_aeu->bit_name,
> > >  							      num);
> > > -					else
> > > -						OSAL_STRNCPY(bit_name,
> > > -							     p_aeu->bit_name,
> > > -							     30);
> > > +					else {
> > > +						strncpy(bit_name,
> > > +							p_aeu->bit_name,
> > > +							sizeof(bit_name) - 1);
> > > +						bit_name[sizeof(bit_name) - 1]
> > > = '\0';
> > > +					}
> >
> > I think you can retain OSAL_STRNCPY and just replace 30 with
> 'bit_name[sizeof(bit_name) - 1'  and then set last byte with '\0' just like you did.
> 
> Can that actually be fixed inside OSAL_STRNCPY itself, rather than having each
> use needing to explicitly null-terminate?

Although there is only instance of OSAL_STRNCPY, it makes sense to modify it.

Thanks,
Shahed

Re: [dpdk-dev] [PATCH 09/18] drivers: net: qede: fix strncpy constant and NUL

[Andy Green]

On 05/09/2018 04:02 AM, Shaikh, Shahed wrote:
>> -----Original Message-----
>> From: dev <dev-bounces@dpdk.org> On Behalf Of Bruce Richardson
>> Sent: Tuesday, May 8, 2018 2:53 PM
>> To: dev-bounces@dpdk.org
>> Cc: Andy Green <andy@warmcat.com>; dev@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH 09/18] drivers: net: qede: fix strncpy constant
>> and NUL
>>
>> On Tue, May 08, 2018 at 05:59:47PM +0000, dev-bounces@dpdk.org wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: dev <dev-bounces@dpdk.org> On Behalf Of Andy Green
>>>> Sent: Monday, May 7, 2018 11:30 PM
>>>> To: dev@dpdk.org
>>>> Subject: [dpdk-dev] [PATCH 09/18] drivers: net: qede: fix strncpy
>>>> constant and NUL
>>>>
>>>>
>>>> ---
>>>>   drivers/net/qede/base/ecore_int.c |   10 ++++++----
>>>>   1 file changed, 6 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/net/qede/base/ecore_int.c
>>>> b/drivers/net/qede/base/ecore_int.c
>>>> index f43781ba4..c809d84ef 100644
>>>> --- a/drivers/net/qede/base/ecore_int.c
>>>> +++ b/drivers/net/qede/base/ecore_int.c
>>>> @@ -1103,10 +1103,12 @@ static enum _ecore_status_t
>>>> ecore_int_deassertion(struct ecore_hwfn *p_hwfn,
>>>>   						OSAL_SNPRINTF(bit_name, 30,
>>>>   							      p_aeu->bit_name,
>>>>   							      num);
>>>> -					else
>>>> -						OSAL_STRNCPY(bit_name,
>>>> -							     p_aeu->bit_name,
>>>> -							     30);
>>>> +					else {
>>>> +						strncpy(bit_name,
>>>> +							p_aeu->bit_name,
>>>> +							sizeof(bit_name) - 1);
>>>> +						bit_name[sizeof(bit_name) - 1]
>>>> = '\0';
>>>> +					}
>>>
>>> I think you can retain OSAL_STRNCPY and just replace 30 with
>> 'bit_name[sizeof(bit_name) - 1'  and then set last byte with '\0' just like you did.
>>
>> Can that actually be fixed inside OSAL_STRNCPY itself, rather than having each
>> use needing to explicitly null-terminate?
> 
> Although there is only instance of OSAL_STRNCPY, it makes sense to modify it.

Doesn't it make more sense to get rid of OSAL_* that bring nothing at 
all to the party?

#define OSAL_SPRINTF(name, pattern, ...) \
         sprintf(name, pattern, ##__VA_ARGS__)
#define OSAL_SNPRINTF(buf, size, format, ...) \
         snprintf(buf, size, format, ##__VA_ARGS__)
#define OSAL_STRLEN(string) strlen(string)
#define OSAL_STRCPY(dst, string) strcpy(dst, string)
#define OSAL_STRNCPY(dst, string, len) strncpy(dst, string, len)
#define OSAL_STRCMP(str1, str2) strcmp(str1, str2)

Do I miss the point or these are just cruft?

-Andy

> Thanks,
> Shahed
> 

Re: [dpdk-dev] [PATCH 09/18] drivers: net: qede: fix strncpy constant and NUL

[Shaikh, Shahed]

> >>> I think you can retain OSAL_STRNCPY and just replace 30 with
> >> 'bit_name[sizeof(bit_name) - 1'  and then set last byte with '\0' just like you
> did.
> >>
> >> Can that actually be fixed inside OSAL_STRNCPY itself, rather than
> >> having each use needing to explicitly null-terminate?
> >
> > Although there is only instance of OSAL_STRNCPY, it makes sense to modify it.
> 
> Doesn't it make more sense to get rid of OSAL_* that bring nothing at all to the
> party?
> 
> #define OSAL_SPRINTF(name, pattern, ...) \
>          sprintf(name, pattern, ##__VA_ARGS__) #define OSAL_SNPRINTF(buf, size,
> format, ...) \
>          snprintf(buf, size, format, ##__VA_ARGS__) #define OSAL_STRLEN(string)
> strlen(string) #define OSAL_STRCPY(dst, string) strcpy(dst, string) #define
> OSAL_STRNCPY(dst, string, len) strncpy(dst, string, len) #define
> OSAL_STRCMP(str1, str2) strcmp(str1, str2)
> 
> Do I miss the point or these are just cruft?

Hi Andy,

I'll send a cleanup patch for this. For now, you can go ahead with original patch.

Thanks,
Shahed

Acked-by: Shahed Shaikh <shahed.shaikh@cavium.com>

> 
> -Andy
> 
> > Thanks,
> > Shahed
> >

[dpdk-dev] [PATCH 10/18] drivers: net: qede: fix broken strncpy

[Andy Green]

/home/agreen/projects/dpdk/drivers/net/qede/qede_main.c: In function ‘qed_slowpath_start’:
/home/agreen/projects/dpdk/drivers/net/qede/qede_main.c:307:3: error: ‘strncpy’ output may be truncated copying 12 bytes from a string of length 127 [-Werror=stringop-truncation]
   strncpy((char *)drv_version.name, (const char *)params->name,
   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    MCP_DRV_VER_STR_SIZE - 4);
    ~~~~~~~~~~~~~~~~~~~~~~~~~
---
 drivers/net/qede/qede_main.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/qede/qede_main.c b/drivers/net/qede/qede_main.c
index 2333ca073..243851da6 100644
--- a/drivers/net/qede/qede_main.c
+++ b/drivers/net/qede/qede_main.c
@@ -305,7 +305,8 @@ static int qed_slowpath_start(struct ecore_dev *edev,
 		    (params->drv_rev << 8) | (params->drv_eng);
 		/* TBD: strlcpy() */
 		strncpy((char *)drv_version.name, (const char *)params->name,
-			MCP_DRV_VER_STR_SIZE - 4);
+			sizeof(drv_version.name) - 1);
+		drv_version.name[sizeof(drv_version.name) - 1] = '\0';
 		rc = ecore_mcp_send_drv_version(hwfn, hwfn->p_main_ptt,
 						&drv_version);
 		if (rc) {

[dpdk-dev] [PATCH 11/18] drivers:net:sfc: fix strncpy length

[Andy Green]

---
 drivers/net/sfc/sfc_ethdev.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/sfc/sfc_ethdev.c b/drivers/net/sfc/sfc_ethdev.c
index e42d55350..e9bb283e0 100644
--- a/drivers/net/sfc/sfc_ethdev.c
+++ b/drivers/net/sfc/sfc_ethdev.c
@@ -742,7 +742,7 @@ sfc_xstats_get_names_by_id(struct rte_eth_dev *dev,
 			char *name = xstats_names[nb_written++].name;
 
 			strncpy(name, efx_mac_stat_name(sa->nic, i),
-				sizeof(xstats_names[0].name));
+				sizeof(xstats_names[0].name) - 1);
 			name[sizeof(xstats_names[0].name) - 1] = '\0';
 		}
 

[dpdk-dev] [PATCH 12/18] drivers: net: sfc: fix another strncpy size and NUL

[Andy Green]

---
 drivers/net/sfc/sfc_ethdev.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/net/sfc/sfc_ethdev.c b/drivers/net/sfc/sfc_ethdev.c
index e9bb283e0..bd5f17f33 100644
--- a/drivers/net/sfc/sfc_ethdev.c
+++ b/drivers/net/sfc/sfc_ethdev.c
@@ -662,10 +662,13 @@ sfc_xstats_get_names(struct rte_eth_dev *dev,
 
 	for (i = 0; i < EFX_MAC_NSTATS; ++i) {
 		if (EFX_MAC_STAT_SUPPORTED(port->mac_stats_mask, i)) {
-			if (xstats_names != NULL && nstats < xstats_count)
+			if (xstats_names != NULL && nstats < xstats_count) {
 				strncpy(xstats_names[nstats].name,
 					efx_mac_stat_name(sa->nic, i),
-					sizeof(xstats_names[0].name));
+					sizeof(xstats_names[0].name) - 1);
+				xstats_names[0].name[
+					sizeof(xstats_names[0].name) - 1] = '\0';
+			}
 			nstats++;
 		}
 	}

Re: [dpdk-dev] [PATCH 12/18] drivers: net: sfc: fix another strncpy size and NUL

[Andrew Rybchenko]

Many thanks. I guess the most of below notes are applicable to many other
patches in the series.

Signed-off-by: ,  Fixes: and Cc: stable@dpdk.org tags are missing. See [1].

Changeset summary should start from "net/sfc: "
I.e. something like:
net/sfc: fix strncpy size and NUL

(it looks like "another" is useless in the original subject)

In general all patches should pass ./devtools/check-git-log.sh and 
./devtools/checkpatches.sh
(which requires path to Linux kernel checkpatches.pl).

Andrew.

[1] 
http://dpdk.org/doc/guides/contributing/patches.html#commit-messages-subject-line

On 05/08/2018 07:30 AM, Andy Green wrote:
> ---
>   drivers/net/sfc/sfc_ethdev.c |    7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/sfc/sfc_ethdev.c b/drivers/net/sfc/sfc_ethdev.c
> index e9bb283e0..bd5f17f33 100644
> --- a/drivers/net/sfc/sfc_ethdev.c
> +++ b/drivers/net/sfc/sfc_ethdev.c
> @@ -662,10 +662,13 @@ sfc_xstats_get_names(struct rte_eth_dev *dev,
>   
>   	for (i = 0; i < EFX_MAC_NSTATS; ++i) {
>   		if (EFX_MAC_STAT_SUPPORTED(port->mac_stats_mask, i)) {
> -			if (xstats_names != NULL && nstats < xstats_count)
> +			if (xstats_names != NULL && nstats < xstats_count) {
>   				strncpy(xstats_names[nstats].name,
>   					efx_mac_stat_name(sa->nic, i),
> -					sizeof(xstats_names[0].name));
> +					sizeof(xstats_names[0].name) - 1);
> +				xstats_names[0].name[
> +					sizeof(xstats_names[0].name) - 1] = '\0';
> +			}

In fact strlcpy() should be used.

>   			nstats++;
>   		}
>   	}
>

Re: [dpdk-dev] [PATCH 12/18] drivers: net: sfc: fix another strncpy size and NUL

[Andy Green]

On 05/08/2018 03:36 PM, Andrew Rybchenko wrote:
> Many thanks. I guess the most of below notes are applicable to many other
> patches in the series.
> 
> Signed-off-by: ,  Fixes: and Cc: stable@dpdk.org tags are missing. See [1].

Everybody's project has different prejudices.

> Changeset summary should start from "net/sfc: "
> I.e. something like:
> net/sfc: fix strncpy size and NUL

Yeah if that's what you like.

> (it looks like "another" is useless in the original subject)

It captures my feeling at having to wade through making 18 fixes before 
I could compile the project on current Fedora.

> In general all patches should pass ./devtools/check-git-log.sh and 
> ./devtools/checkpatches.sh
> (which requires path to Linux kernel checkpatches.pl).

Can you help me understand why adding CRLFs at 80 cols on the gcc errors 
I pasted helps anything at all?  The patches actually fix problems in 
the code.

If you don't care about Coverity, let me know and I will register this 
project there and send you fixes when I have time.

> Andrew.
> 
> [1] 
> http://dpdk.org/doc/guides/contributing/patches.html#commit-messages-subject-line
> 
> On 05/08/2018 07:30 AM, Andy Green wrote:
>> ---
>>   drivers/net/sfc/sfc_ethdev.c |    7 +++++--
>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/sfc/sfc_ethdev.c b/drivers/net/sfc/sfc_ethdev.c
>> index e9bb283e0..bd5f17f33 100644
>> --- a/drivers/net/sfc/sfc_ethdev.c
>> +++ b/drivers/net/sfc/sfc_ethdev.c
>> @@ -662,10 +662,13 @@ sfc_xstats_get_names(struct rte_eth_dev *dev,
>>   
>>   	for (i = 0; i < EFX_MAC_NSTATS; ++i) {
>>   		if (EFX_MAC_STAT_SUPPORTED(port->mac_stats_mask, i)) {
>> -			if (xstats_names != NULL && nstats < xstats_count)
>> +			if (xstats_names != NULL && nstats < xstats_count) {
>>   				strncpy(xstats_names[nstats].name,
>>   					efx_mac_stat_name(sa->nic, i),
>> -					sizeof(xstats_names[0].name));
>> +					sizeof(xstats_names[0].name) - 1);
>> +				xstats_names[0].name[
>> +					sizeof(xstats_names[0].name) - 1] = '\0';
>> +			}
> 
> In fact strlcpy() should be used.
Fair enough.  Last time I looked it wasn't in glibc but seems it is now.

-Andy

>>   			nstats++;
>>   		}
>>   	}
>>
> 

Re: [dpdk-dev] [PATCH 12/18] drivers: net: sfc: fix another strncpy size and NUL

[Bruce Richardson]

On Tue, May 08, 2018 at 04:18:41PM +0800, Andy Green wrote:
> 
> 
> On 05/08/2018 03:36 PM, Andrew Rybchenko wrote:
> > Many thanks. I guess the most of below notes are applicable to many other
> > patches in the series.
> > 
> > Signed-off-by: ,  Fixes: and Cc: stable@dpdk.org tags are missing. See [1].
> 
> Everybody's project has different prejudices.
> 
> > Changeset summary should start from "net/sfc: "
> > I.e. something like:
> > net/sfc: fix strncpy size and NUL
> 
> Yeah if that's what you like.
> 
> > (it looks like "another" is useless in the original subject)
> 
> It captures my feeling at having to wade through making 18 fixes before I
> could compile the project on current Fedora.
> 
> > In general all patches should pass ./devtools/check-git-log.sh and
> > ./devtools/checkpatches.sh
> > (which requires path to Linux kernel checkpatches.pl).
> 
> Can you help me understand why adding CRLFs at 80 cols on the gcc errors I
> pasted helps anything at all?  The patches actually fix problems in the
> code.
> 

I don't think there is any need to wrap the error messages since they are
pasted verbatim.

> If you don't care about Coverity, let me know and I will register this
> project there and send you fixes when I have time.
> 

FYI: The DPDK project is already registered in coverity and scanned regularly.
We do try and fix as many issues it flags as possible, but not everything
gets dealt with as quickly as we'd like, sadly.

https://scan.coverity.com/projects/dpdk-data-plane-development-kit

Regards,
/Bruce

Re: [dpdk-dev] [PATCH 12/18] drivers: net: sfc: fix another strncpy size and NUL

[Andrew Rybchenko]

On 05/08/2018 11:18 AM, Andy Green wrote:
> On 05/08/2018 03:36 PM, Andrew Rybchenko wrote:
>> (it looks like "another" is useless in the original subject)
>
> It captures my feeling at having to wade through making 18 fixes 
> before I could compile the project on current Fedora.

I see.

>> In general all patches should pass ./devtools/check-git-log.sh and 
>> ./devtools/checkpatches.sh
>> (which requires path to Linux kernel checkpatches.pl).
>
> Can you help me understand why adding CRLFs at 80 cols on the gcc 
> errors I pasted helps anything at all?  The patches actually fix 
> problems in the code.

Seeing GCC errors which patch fixes is useful to see. Yes, I agree that 
it is real problem in the code.

> If you don't care about Coverity, let me know and I will register this 
> project there and send you fixes when I have time.

dpdk is registered at Coverity and we get reports from time to time.

>
>> Andrew.
>>
>> [1] 
>> http://dpdk.org/doc/guides/contributing/patches.html#commit-messages-subject-line
>>
>> On 05/08/2018 07:30 AM, Andy Green wrote:
>>> ---
>>>   drivers/net/sfc/sfc_ethdev.c |    7 +++++--
>>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/net/sfc/sfc_ethdev.c 
>>> b/drivers/net/sfc/sfc_ethdev.c
>>> index e9bb283e0..bd5f17f33 100644
>>> --- a/drivers/net/sfc/sfc_ethdev.c
>>> +++ b/drivers/net/sfc/sfc_ethdev.c
>>> @@ -662,10 +662,13 @@ sfc_xstats_get_names(struct rte_eth_dev *dev,
>>>         for (i = 0; i < EFX_MAC_NSTATS; ++i) {
>>>           if (EFX_MAC_STAT_SUPPORTED(port->mac_stats_mask, i)) {
>>> -            if (xstats_names != NULL && nstats < xstats_count)
>>> +            if (xstats_names != NULL && nstats < xstats_count) {
>>>                   strncpy(xstats_names[nstats].name,
>>>                       efx_mac_stat_name(sa->nic, i),
>>> -                    sizeof(xstats_names[0].name));
>>> +                    sizeof(xstats_names[0].name) - 1);
>>> +                xstats_names[0].name[
>>> +                    sizeof(xstats_names[0].name) - 1] = '\0';
>>> +            }
>>
>> In fact strlcpy() should be used.
> Fair enough.  Last time I looked it wasn't in glibc but seems it is now.

As far as I know it is not in glibc, but dpdk has internal fallback if 
the function
is not available from external libs.

Andrew.

[dpdk-dev] [PATCH 13/18] drivers: net: vdev: readlink inputs cannot be aliased

[Andy Green]

/home/agreen/projects/dpdk/drivers/net/vdev_netvsc/vdev_netvsc.c:335:2: error: passing argument 2 to restrict-qualified parameter aliases with argument 1 [-Werror=restrict]
  ret = readlink(buf, buf, size);
  ^~~
---
 drivers/net/vdev_netvsc/vdev_netvsc.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/net/vdev_netvsc/vdev_netvsc.c b/drivers/net/vdev_netvsc/vdev_netvsc.c
index c321a9f1b..c11794137 100644
--- a/drivers/net/vdev_netvsc/vdev_netvsc.c
+++ b/drivers/net/vdev_netvsc/vdev_netvsc.c
@@ -327,12 +327,13 @@ static int
 vdev_netvsc_sysfs_readlink(char *buf, size_t size, const char *if_name,
 			   const char *relpath)
 {
+	char in[160];
 	int ret;
 
-	ret = snprintf(buf, size, "/sys/class/net/%s/%s", if_name, relpath);
-	if (ret == -1 || (size_t)ret >= size)
+	ret = snprintf(in, sizeof(buf) - 1, "/sys/class/net/%s/%s", if_name, relpath);
+	if (ret == -1 || (size_t)ret >= sizeof(buf) - 1)
 		return -ENOBUFS;
-	ret = readlink(buf, buf, size);
+	ret = readlink(in, buf, size);
 	if (ret == -1)
 		return -errno;
 	if ((size_t)ret >= size - 1)

[dpdk-dev] [PATCH 14/18] drivers: net: vdev: fix 3 x strncpy misuse

[Andy Green]

---
 drivers/net/vdev_netvsc/vdev_netvsc.c |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/net/vdev_netvsc/vdev_netvsc.c b/drivers/net/vdev_netvsc/vdev_netvsc.c
index c11794137..c36ec0f9a 100644
--- a/drivers/net/vdev_netvsc/vdev_netvsc.c
+++ b/drivers/net/vdev_netvsc/vdev_netvsc.c
@@ -182,7 +182,8 @@ vdev_netvsc_foreach_iface(int (*func)(const struct if_nameindex *iface,
 		is_netvsc_ret = vdev_netvsc_iface_is_netvsc(&iface[i]) ? 1 : 0;
 		if (is_netvsc ^ is_netvsc_ret)
 			continue;
-		strncpy(req.ifr_name, iface[i].if_name, sizeof(req.ifr_name));
+		strncpy(req.ifr_name, iface[i].if_name, sizeof(req.ifr_name) - 1);
+		req.ifr_name[sizeof(req.ifr_name) - 1] = '\0';
 		if (ioctl(s, SIOCGIFHWADDR, &req) == -1) {
 			DRV_LOG(WARNING, "cannot retrieve information about"
 					 " interface \"%s\": %s",
@@ -383,7 +384,8 @@ vdev_netvsc_device_probe(const struct if_nameindex *iface,
 		DRV_LOG(DEBUG,
 			"NetVSC interface \"%s\" (index %u) renamed \"%s\"",
 			ctx->if_name, ctx->if_index, iface->if_name);
-		strncpy(ctx->if_name, iface->if_name, sizeof(ctx->if_name));
+		strncpy(ctx->if_name, iface->if_name, sizeof(ctx->if_name) - 1);
+		ctx->if_name[sizeof(ctx->if_name) - 1] = '\0';
 		return 0;
 	}
 	if (!is_same_ether_addr(eth_addr, &ctx->if_addr))
@@ -581,7 +583,8 @@ vdev_netvsc_netvsc_probe(const struct if_nameindex *iface,
 		goto error;
 	}
 	ctx->id = vdev_netvsc_ctx_count;
-	strncpy(ctx->if_name, iface->if_name, sizeof(ctx->if_name));
+	strncpy(ctx->if_name, iface->if_name, sizeof(ctx->if_name) - 1);
+	ctx->if_name[sizeof(ctx->if_name) - 1] = '\0';
 	ctx->if_index = iface->if_index;
 	ctx->if_addr = *eth_addr;
 	ctx->pipe[0] = -1;

[dpdk-dev] [PATCH 15/18] test-pmd: can't find include

[Andy Green]

/home/agreen/projects/dpdk/app/test-pmd/cmdline.c:64:10: fatal error: rte_pmd_dpaa.h: No such file or directory
 #include <rte_pmd_dpaa.h>
          ^~~~~~~~~~~~~~~~
---
 app/test-pmd/Makefile |    1 +
 1 file changed, 1 insertion(+)

diff --git a/app/test-pmd/Makefile b/app/test-pmd/Makefile
index 60ae9b9c1..a0fdd0e11 100644
--- a/app/test-pmd/Makefile
+++ b/app/test-pmd/Makefile
@@ -13,6 +13,7 @@ APP = testpmd
 CFLAGS += -DALLOW_EXPERIMENTAL_API
 CFLAGS += -O3
 CFLAGS += $(WERROR_FLAGS)
+CFLAGS += -I$(RTE_SDK)/drivers/net/dpaa
 
 #
 # all source are stored in SRCS-y

[dpdk-dev] [PATCH 16/18] app: fix sprintf overrun bug

[Andy Green]

/home/agreen/projects/dpdk/app/proc-info/main.c: In function ‘nic_xstats_display’:
/home/agreen/projects/dpdk/app/proc-info/main.c:495:45: error: ‘%s’ directive writing up to 255 bytes into a region of size between 165 and 232 [-Werror=format-overflow=]
    sprintf(buf, "PUTVAL %s/dpdkstat-port.%u/%s-%s N:%"
                                             ^~
     PRIu64"\n", host_id, port_id, counter_type,
                                   ~~~~~~~~~~~~
/home/agreen/projects/dpdk/app/proc-info/main.c:495:4: note: ‘sprintf’ output between 31 and 435 bytes into a destination of size 256
    sprintf(buf, "PUTVAL %s/dpdkstat-port.%u/%s-%s N:%"
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     PRIu64"\n", host_id, port_id, counter_type,
     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     xstats_names[i].name, values[i]);
---
 app/proc-info/main.c |    9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/app/proc-info/main.c b/app/proc-info/main.c
index 539e13243..df46c235e 100644
--- a/app/proc-info/main.c
+++ b/app/proc-info/main.c
@@ -488,14 +488,19 @@ nic_xstats_display(uint16_t port_id)
 		if (enable_collectd_format) {
 			char counter_type[MAX_STRING_LEN];
 			char buf[MAX_STRING_LEN];
+			size_t n;
 
 			collectd_resolve_cnt_type(counter_type,
 						  sizeof(counter_type),
 						  xstats_names[i].name);
-			sprintf(buf, "PUTVAL %s/dpdkstat-port.%u/%s-%s N:%"
+			n = snprintf(buf, MAX_STRING_LEN,
+				"PUTVAL %s/dpdkstat-port.%u/%s-%s N:%"
 				PRIu64"\n", host_id, port_id, counter_type,
 				xstats_names[i].name, values[i]);
-			ret = write(stdout_fd, buf, strlen(buf));
+			buf[sizeof(buf) - 1] = '\0';
+			if (n > sizeof(buf) - 1)
+				n = sizeof(buf) - 1;
+			ret = write(stdout_fd, buf, n);
 			if (ret < 0)
 				goto err;
 		} else {

[dpdk-dev] [PATCH 17/18] app: test-bbdev: strcpy ok for allocated string

[Andy Green]

---
 app/test-bbdev/test_bbdev_vector.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/test-bbdev/test_bbdev_vector.c b/app/test-bbdev/test_bbdev_vector.c
index addef0572..5ad2a6535 100644
--- a/app/test-bbdev/test_bbdev_vector.c
+++ b/app/test-bbdev/test_bbdev_vector.c
@@ -890,7 +890,7 @@ test_bbdev_vector_read(const char *filename,
 		}
 
 		memset(entry, 0, strlen(line) + 1);
-		strncpy(entry, line, strlen(line));
+		strcpy(entry, line);
 
 		/* check if entry ends with , or = */
 		if (entry[strlen(entry) - 1] == ','

[dpdk-dev] [PATCH 18/18] app: test-bbdev: strcpy ok for allocated string 2

[Andy Green]

---
 app/test-bbdev/test_bbdev_vector.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/app/test-bbdev/test_bbdev_vector.c b/app/test-bbdev/test_bbdev_vector.c
index 5ad2a6535..373f94984 100644
--- a/app/test-bbdev/test_bbdev_vector.c
+++ b/app/test-bbdev/test_bbdev_vector.c
@@ -912,7 +912,8 @@ test_bbdev_vector_read(const char *filename,
 				}
 
 				entry = entry_extended;
-				strncat(entry, line, strlen(line));
+				/* entry has been allocated accordingly */
+				strcpy(&entry[strlen(entry)], line);
 
 				if (entry[strlen(entry) - 1] != ',')
 					break;