[dpdk-dev] [PATCH v4 00/18] Fix default build on gcc8.0.1

[dpdk-dev] [PATCH v4 00/18] Fix default build on gcc8.0.1

[Andy Green]

This series allows dpdk master to build on Fedora 28,
with the x86_64 default config.

---

Andy Green (18):
      devtools/check-git: provide more generic grep pattern
      net/nfp: solve buffer overflow
      bus/pci: replace strncpy dangerous code
      bus/dpaa: solve inconsistent struct alignment
      net/axgbe: solve broken eeprom string comp
      net/nfp/nfpcore: solve strncpy misuse
      net/nfp/nfpcore: off-by-one and no NUL on strncpy use
      net/nfp: don't memcpy out of source range
      net/qede: strncpy length constant and NUL
      net/qede: solve broken strncpy
      net/sfc: correct strncpy length
      net/sfc: solve strncpy size and NUL
      net/vdev_netvsc: readlink inputs cannot be aliased
      net/vdev_netvsc: 3 x strncpy misuse
      app: can't find include
      app/proc-info: sprintf overrun bug
      app/test-bbdev: strcpy ok for allocated string
      app/test-bbdev: strcpy ok for allocated string


 app/proc-info/main.c                       |    9 +++++++--
 app/test-bbdev/test_bbdev_vector.c         |    5 +++--
 app/test-pmd/Makefile                      |    1 +
 devtools/check-git-log.sh                  |    4 ++--
 drivers/bus/dpaa/base/qbman/qman.c         |   14 +++++++-------
 drivers/bus/dpaa/include/fsl_qman.h        |   24 +++++++++++++-----------
 drivers/bus/pci/linux/pci.c                |    2 +-
 drivers/net/axgbe/axgbe_phy_impl.c         |    4 ++--
 drivers/net/nfp/nfp_net.c                  |    6 +++---
 drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c |    4 +++-
 drivers/net/nfp/nfpcore/nfp_resource.c     |   10 ++++++----
 drivers/net/qede/base/ecore_int.c          |    8 +++++---
 drivers/net/qede/qede_main.c               |    7 ++++---
 drivers/net/sfc/sfc_ethdev.c               |    6 +++---
 drivers/net/vdev_netvsc/vdev_netvsc.c      |   15 +++++++++------
 15 files changed, 69 insertions(+), 50 deletions(-)

--
Signature

[dpdk-dev] [PATCH v4 01/18] devtools/check-git: provide more generic grep pattern

[Andy Green]

On Fedora 28, every patch is faulted for
"Wrong headline uppercase", because [A-Z] is not
always case sensitive.

Change to use [[:upper:]]
---
 devtools/check-git-log.sh |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/devtools/check-git-log.sh b/devtools/check-git-log.sh
index c601f6ae9..2542d9ee0 100755
--- a/devtools/check-git-log.sh
+++ b/devtools/check-git-log.sh
@@ -106,8 +106,8 @@ bad=$(echo "$headlines" | grep --color=always \
 
 # check headline lowercase for first words
 bad=$(echo "$headlines" | grep --color=always \
-	-e '^.*[A-Z].*:' \
-	-e ': *[A-Z]' \
+	-e '^.*[[:upper:]].*:' \
+	-e ': *[[:upper:]]' \
 	| sed 's,^,\t,')
 [ -z "$bad" ] || printf "Wrong headline uppercase:\n$bad\n"
 

Re: [dpdk-dev] [PATCH v4 01/18] devtools/check-git: provide more generic grep pattern

[De Lara Guarch, Pablo]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Andy Green
> Sent: Friday, May 11, 2018 2:45 AM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH v4 01/18] devtools/check-git: provide more generic
> grep pattern
> 
> On Fedora 28, every patch is faulted for "Wrong headline uppercase", because
> [A-Z] is not always case sensitive.
> 
> Change to use [[:upper:]]

Missing signed-off line.

Acked-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>

[dpdk-dev] [PATCH v4 02/18] net/nfp: solve buffer overflow

[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);

Note fw_buf still has to increase somewhat even after
restricting serial[], since otherwise:

/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:3176:23:
error: ‘%s’ directive writing up to 99 bytes into a region
of size 76 [-Werror=format-overflow=]
  sprintf(fw_name, "%s/%s", DEFAULT_FW_PATH, card);
                       ^~
/home/agreen/projects/dpdk/drivers/net/nfp/nfp_net.c:3262:32:
  err = nfp_fw_upload(dev, nsp, card_desc);
                                ~~~~~~~~~
/home/agreen/projects/dpdk/drivers/net/nfp/nfp_net.c:3176:2:
note: ‘sprintf’ output between 25 and 124 bytes into a
destination of size 100
  sprintf(fw_name, "%s/%s", DEFAULT_FW_PATH, card);

Signed-off-by: Andy Green <andy@warmcat.com>
---
 drivers/net/nfp/nfp_net.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/nfp/nfp_net.c b/drivers/net/nfp/nfp_net.c
index 048324ec9..78113b41b 100644
--- a/drivers/net/nfp/nfp_net.c
+++ b/drivers/net/nfp/nfp_net.c
@@ -3144,8 +3144,8 @@ 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 serial[100];
+	char fw_name[125];
+	char serial[40];
 	struct stat file_stat;
 	off_t fsize, bytes;
 

Re: [dpdk-dev] [PATCH v4 02/18] net/nfp: solve buffer overflow

[De Lara Guarch, Pablo]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Andy Green
> Sent: Friday, May 11, 2018 2:45 AM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH v4 02/18] net/nfp: solve buffer overflow
> 
> /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);
> 
> Note fw_buf still has to increase somewhat even after restricting serial[], since
> otherwise:
> 
> /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:3176:23:
> error: ‘%s’ directive writing up to 99 bytes into a region of size 76 [-
> Werror=format-overflow=]
>   sprintf(fw_name, "%s/%s", DEFAULT_FW_PATH, card);
>                        ^~
> /home/agreen/projects/dpdk/drivers/net/nfp/nfp_net.c:3262:32:
>   err = nfp_fw_upload(dev, nsp, card_desc);
>                                 ~~~~~~~~~
> /home/agreen/projects/dpdk/drivers/net/nfp/nfp_net.c:3176:2:
> note: ‘sprintf’ output between 25 and 124 bytes into a destination of size 100
>   sprintf(fw_name, "%s/%s", DEFAULT_FW_PATH, card);
> 
> Signed-off-by: Andy Green <andy@warmcat.com>

Missing fixes line and CC stable.

Fixes: 896c265ef954 ("net/nfp: use new CPP interface")
Cc: stable@dpdk.org

Acked-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>

Re: [dpdk-dev] [PATCH v4 02/18] net/nfp: solve buffer overflow

[De Lara Guarch, Pablo]

Hi,

> -----Original Message-----
> From: De Lara Guarch, Pablo
> Sent: Friday, May 11, 2018 9:58 AM
> To: 'Andy Green' <andy@warmcat.com>; dev@dpdk.org
> Cc: stable@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH v4 02/18] net/nfp: solve buffer overflow
> 
> 
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Andy Green
> > Sent: Friday, May 11, 2018 2:45 AM
> > To: dev@dpdk.org
> > Subject: [dpdk-dev] [PATCH v4 02/18] net/nfp: solve buffer overflow
> >
> > /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);
> >
> > Note fw_buf still has to increase somewhat even after restricting
> > serial[], since
> > otherwise:
> >
> > /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:3176:23:
> > error: ‘%s’ directive writing up to 99 bytes into a region of size 76
> > [- Werror=format-overflow=]
> >   sprintf(fw_name, "%s/%s", DEFAULT_FW_PATH, card);
> >                        ^~
> > /home/agreen/projects/dpdk/drivers/net/nfp/nfp_net.c:3262:32:
> >   err = nfp_fw_upload(dev, nsp, card_desc);
> >                                 ~~~~~~~~~
> > /home/agreen/projects/dpdk/drivers/net/nfp/nfp_net.c:3176:2:
> > note: ‘sprintf’ output between 25 and 124 bytes into a destination of size 100
> >   sprintf(fw_name, "%s/%s", DEFAULT_FW_PATH, card);
> >
> > Signed-off-by: Andy Green <andy@warmcat.com>
> 
> Missing fixes line and CC stable.
> 
> Fixes: 896c265ef954 ("net/nfp: use new CPP interface")
> Cc: stable@dpdk.org
> 
> Acked-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>

Actually, this does not need to be backported to stable, as it was merged in this release.
Sorry about the noise.

[dpdk-dev] [PATCH v4 03/18] bus/pci: replace 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);

Signed-off-by: Andy Green <andy@warmcat.com>
Acked-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>
---
 drivers/bus/pci/linux/pci.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/bus/pci/linux/pci.c b/drivers/bus/pci/linux/pci.c
index 4630a8057..a73ee49c2 100644
--- a/drivers/bus/pci/linux/pci.c
+++ b/drivers/bus/pci/linux/pci.c
@@ -54,7 +54,7 @@ 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);
+		strlcpy(dri_name, name + 1, sizeof(dri_name));
 		return 0;
 	}
 

Re: [dpdk-dev] [PATCH v4 03/18] bus/pci: replace strncpy dangerous code

[De Lara Guarch, Pablo]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Andy Green
> Sent: Friday, May 11, 2018 2:46 AM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH v4 03/18] bus/pci: replace strncpy dangerous code
> 
> 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);
> 
> Signed-off-by: Andy Green <andy@warmcat.com>
> Acked-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>

Forgot to say that this needs fixes lines and cc stable.

Fixes: d9a8cd9595f2 ("pci: add kernel driver type")
Cc: stable@dpdk.org

[dpdk-dev] [PATCH v4 04/18] bus/dpaa: solve 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.

Signed-off-by: Andy Green <andy@warmcat.com>
Acked-by: Hemant Agrawal <hemant.agrawal@nxp.com>
Tested-by: Hemant Agrawal <hemant.agrawal@nxp.com>
---
 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..e4ad7ae48 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

Re: [dpdk-dev] [PATCH v4 04/18] bus/dpaa: solve inconsistent struct alignment

[De Lara Guarch, Pablo]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Andy Green
> Sent: Friday, May 11, 2018 2:46 AM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH v4 04/18] bus/dpaa: solve inconsistent struct
> alignment
> 
> 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.
> 
> Signed-off-by: Andy Green <andy@warmcat.com>
> Acked-by: Hemant Agrawal <hemant.agrawal@nxp.com>
> Tested-by: Hemant Agrawal <hemant.agrawal@nxp.com>

Missing fixes line and CC stable:

Fixes: c47ff048b99a ("bus/dpaa: add QMAN driver core routines")
Fixes: f6fadc3e6310 ("bus/dpaa: add QMAN interface driver")
Cc: stable@dpdk.org

[dpdk-dev] [PATCH v4 05/18] net/axgbe: solve 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))

Signed-off-by: Andy Green <andy@warmcat.com>
---
 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;

Re: [dpdk-dev] [PATCH v4 05/18] net/axgbe: solve broken eeprom string comp

[De Lara Guarch, Pablo]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Andy Green
> Sent: Friday, May 11, 2018 2:46 AM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH v4 05/18] net/axgbe: solve broken eeprom string
> comp
> 
> /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))
> 
> Signed-off-by: Andy Green <andy@warmcat.com>

Missing fixes line:

Fixes: a5c7273771e8 ("net/axgbe: add phy programming APIs")

Acked-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>

[dpdk-dev] [PATCH v4 06/18] net/nfp/nfpcore: solve strncpy misuse

[Andy Green]

Signed-off-by: Andy Green <andy@warmcat.com>
Acked-by: Alejandro Lucero <alejandro.lucero@netronome.com>
Tested-by: Alejandro Lucero <alejandro.lucero@netronome.com>
---
 drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c |    4 +++-
 1 file changed, 3 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..52b294888 100644
--- a/drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c
+++ b/drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c
@@ -31,6 +31,8 @@
 #include <sys/file.h>
 #include <sys/stat.h>
 
+#include <rte_string_fns.h>
+
 #include "nfp_cpp.h"
 #include "nfp_target.h"
 #include "nfp6000/nfp6000.h"
@@ -846,7 +848,7 @@ nfp6000_init(struct nfp_cpp *cpp, const char *devname)
 
 
 	memset(desc->busdev, 0, BUSDEV_SZ);
-	strncpy(desc->busdev, devname, strlen(devname));
+	strlcpy(desc->busdev, devname, sizeof(desc->busdev));
 
 	ret = nfp_acquire_process_lock(desc);
 	if (ret)

Re: [dpdk-dev] [PATCH v4 06/18] net/nfp/nfpcore: solve strncpy misuse

[De Lara Guarch, Pablo]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Andy Green
> Sent: Friday, May 11, 2018 2:46 AM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH v4 06/18] net/nfp/nfpcore: solve strncpy misuse
> 
> Signed-off-by: Andy Green <andy@warmcat.com>
> Acked-by: Alejandro Lucero <alejandro.lucero@netronome.com>
> Tested-by: Alejandro Lucero <alejandro.lucero@netronome.com>

Missing fixes line.

Fixes: c7e9729da6b5 ("net/nfp: support CPP")

[dpdk-dev] [PATCH v4 07/18] net/nfp/nfpcore: 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));

Signed-off-by: Andy Green <andy@warmcat.com>
---
 drivers/net/nfp/nfpcore/nfp_resource.c |   10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/net/nfp/nfpcore/nfp_resource.c b/drivers/net/nfp/nfpcore/nfp_resource.c
index e1df2b2e1..dd41fa4de 100644
--- a/drivers/net/nfp/nfpcore/nfp_resource.c
+++ b/drivers/net/nfp/nfpcore/nfp_resource.c
@@ -7,6 +7,8 @@
 #include <time.h>
 #include <endian.h>
 
+#include <rte_string_fns.h>
+
 #include "nfp_cpp.h"
 #include "nfp6000/nfp6000.h"
 #include "nfp_resource.h"
@@ -65,22 +67,22 @@ 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));
+	strlcpy(name_pad, res->name, sizeof(name_pad));
 
 	/* Search for a matching entry */
 	if (!memcmp(name_pad, NFP_RESOURCE_TBL_NAME "\0\0\0\0\0\0\0\0", 8)) {
 		printf("Grabbing device lock not supported\n");
 		return -EOPNOTSUPP;
 	}
-	key = nfp_crc32_posix(name_pad, sizeof(name_pad));
+	key = nfp_crc32_posix(name_pad, NFP_RESOURCE_ENTRY_NAME_SZ);
 
 	for (i = 0; i < NFP_RESOURCE_TBL_ENTRIES; i++) {
 		uint64_t addr = NFP_RESOURCE_TBL_BASE +

Re: [dpdk-dev] [PATCH v4 07/18] net/nfp/nfpcore: off-by-one and no NUL on strncpy use

[De Lara Guarch, Pablo]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Andy Green
> Sent: Friday, May 11, 2018 2:46 AM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH v4 07/18] net/nfp/nfpcore: off-by-one and no NUL
> on strncpy use
> 
> /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));
> 
> Signed-off-by: Andy Green <andy@warmcat.com>
> ---
>  drivers/net/nfp/nfpcore/nfp_resource.c |   10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/nfp/nfpcore/nfp_resource.c
> b/drivers/net/nfp/nfpcore/nfp_resource.c
> index e1df2b2e1..dd41fa4de 100644
> --- a/drivers/net/nfp/nfpcore/nfp_resource.c
> +++ b/drivers/net/nfp/nfpcore/nfp_resource.c

...

> -	memset(name_pad, 0, NFP_RESOURCE_ENTRY_NAME_SZ);
> -	strncpy(name_pad, res->name, sizeof(name_pad));
> +	memset(name_pad, 0, sizeof(name_pad));
> +	strlcpy(name_pad, res->name, sizeof(name_pad));

I think memset is not required, as strlcpy already null terminate the buffer.

...

Missing fixes line.

Fixes: c7e9729da6b5 ("net/nfp: support CPP")

Acked-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>

Re: [dpdk-dev] [PATCH v4 07/18] net/nfp/nfpcore: off-by-one and no NUL on strncpy use

[Andy Green]

On 05/11/2018 06:33 PM, De Lara Guarch, Pablo wrote:
> 
> 
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Andy Green
>> Sent: Friday, May 11, 2018 2:46 AM
>> To: dev@dpdk.org
>> Subject: [dpdk-dev] [PATCH v4 07/18] net/nfp/nfpcore: off-by-one and no NUL
>> on strncpy use
>>
>> /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));
>>
>> Signed-off-by: Andy Green <andy@warmcat.com>
>> ---
>>   drivers/net/nfp/nfpcore/nfp_resource.c |   10 ++++++----
>>   1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/nfp/nfpcore/nfp_resource.c
>> b/drivers/net/nfp/nfpcore/nfp_resource.c
>> index e1df2b2e1..dd41fa4de 100644
>> --- a/drivers/net/nfp/nfpcore/nfp_resource.c
>> +++ b/drivers/net/nfp/nfpcore/nfp_resource.c
> 
> ...
> 
>> -	memset(name_pad, 0, NFP_RESOURCE_ENTRY_NAME_SZ);
>> -	strncpy(name_pad, res->name, sizeof(name_pad));
>> +	memset(name_pad, 0, sizeof(name_pad));
>> +	strlcpy(name_pad, res->name, sizeof(name_pad));
> 
> I think memset is not required, as strlcpy already null terminate the buffer.

It seems required to keep it, because of the exciting code just below it:

         /* Search for a matching entry */
         if (!memcmp(name_pad, NFP_RESOURCE_TBL_NAME "\0\0\0\0\0\0\0\0", 
8)) {
                 printf("Grabbing device lock not supported\n");
                 return -EOPNOTSUPP;
         }

-Andy

> ...
> 
> Missing fixes line.
> 
> Fixes: c7e9729da6b5 ("net/nfp: support CPP")
> 
> Acked-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>
> 

[dpdk-dev] [PATCH v4 08/18] 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));

Signed-off-by: Andy Green <andy@warmcat.com>
Acked-by: Alejandro Lucero <alejandro.lucero@netronome.com>
Tested-by: Alejandro Lucero <alejandro.lucero@netronome.com>
---
 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 78113b41b..f114b1839 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);

Re: [dpdk-dev] [PATCH v4 08/18] net/nfp: don't memcpy out of source range

[De Lara Guarch, Pablo]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Andy Green
> Sent: Friday, May 11, 2018 2:46 AM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH v4 08/18] net/nfp: don't memcpy out of source
> range
> 
> /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));
> 
> Signed-off-by: Andy Green <andy@warmcat.com>
> Acked-by: Alejandro Lucero <alejandro.lucero@netronome.com>
> Tested-by: Alejandro Lucero <alejandro.lucero@netronome.com>

Missing fixes line and CC stable.

Fixes: e6decee38209 ("net/nfp: use random MAC address if not configured")

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

[Andy Green]

Signed-off-by: Andy Green <andy@warmcat.com>
---
 drivers/net/qede/base/ecore_int.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/net/qede/base/ecore_int.c b/drivers/net/qede/base/ecore_int.c
index f43781ba4..d9e22b5ed 100644
--- a/drivers/net/qede/base/ecore_int.c
+++ b/drivers/net/qede/base/ecore_int.c
@@ -6,6 +6,8 @@
  * See LICENSE.qede_pmd for copyright and licensing details.
  */
 
+#include <rte_string_fns.h>
+
 #include "bcm_osal.h"
 #include "ecore.h"
 #include "ecore_spq.h"
@@ -1104,9 +1106,9 @@ static enum _ecore_status_t ecore_int_deassertion(struct ecore_hwfn *p_hwfn,
 							      p_aeu->bit_name,
 							      num);
 					else
-						OSAL_STRNCPY(bit_name,
-							     p_aeu->bit_name,
-							     30);
+						strlcpy(bit_name,
+							p_aeu->bit_name,
+							sizeof(bit_name));
 
 					/* We now need to pass bitmask in its
 					 * correct position.

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

[De Lara Guarch, Pablo]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Andy Green
> Sent: Friday, May 11, 2018 2:46 AM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH v4 09/18] net/qede: strncpy length constant and
> NUL
> 
> Signed-off-by: Andy Green <andy@warmcat.com>
> ---
>  drivers/net/qede/base/ecore_int.c |    8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/qede/base/ecore_int.c
> b/drivers/net/qede/base/ecore_int.c
> index f43781ba4..d9e22b5ed 100644
> --- a/drivers/net/qede/base/ecore_int.c
> +++ b/drivers/net/qede/base/ecore_int.c
> @@ -6,6 +6,8 @@
>   * See LICENSE.qede_pmd for copyright and licensing details.
>   */
> 
> +#include <rte_string_fns.h>
> +
>  #include "bcm_osal.h"
>  #include "ecore.h"
>  #include "ecore_spq.h"
> @@ -1104,9 +1106,9 @@ static enum _ecore_status_t
> ecore_int_deassertion(struct ecore_hwfn *p_hwfn,
>  							      p_aeu->bit_name,
>  							      num);
>  					else
> -						OSAL_STRNCPY(bit_name,
> -							     p_aeu->bit_name,
> -							     30);
> +						strlcpy(bit_name,
> +							p_aeu->bit_name,
> +							sizeof(bit_name));
> 
>  					/* We now need to pass bitmask in its
>  					 * correct position.

I'd say it should be better to change OSAL_STRNCPY to OSAL_STRLCPY and
modify the macro to use strlcpy, so we avoid further uses of that strlcpy.

However, this modifies base driver code, so it is up to the maintainers to make that decision.
(CC'ing maintainers here).

Also, missing fixes line and CC stable.

Fixes: 8427c6647964 ("net/qede/base: add attention formatting string")
Cc: stable@dpdk.org

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

[Andy Green]

On 05/11/2018 06:43 PM, De Lara Guarch, Pablo wrote:
> 
> 
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Andy Green
>> Sent: Friday, May 11, 2018 2:46 AM
>> To: dev@dpdk.org
>> Subject: [dpdk-dev] [PATCH v4 09/18] net/qede: strncpy length constant and
>> NUL
>>
>> Signed-off-by: Andy Green <andy@warmcat.com>
>> ---
>>   drivers/net/qede/base/ecore_int.c |    8 +++++---
>>   1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/qede/base/ecore_int.c
>> b/drivers/net/qede/base/ecore_int.c
>> index f43781ba4..d9e22b5ed 100644
>> --- a/drivers/net/qede/base/ecore_int.c
>> +++ b/drivers/net/qede/base/ecore_int.c
>> @@ -6,6 +6,8 @@
>>    * See LICENSE.qede_pmd for copyright and licensing details.
>>    */
>>
>> +#include <rte_string_fns.h>
>> +
>>   #include "bcm_osal.h"
>>   #include "ecore.h"
>>   #include "ecore_spq.h"
>> @@ -1104,9 +1106,9 @@ static enum _ecore_status_t
>> ecore_int_deassertion(struct ecore_hwfn *p_hwfn,
>>   							      p_aeu->bit_name,
>>   							      num);
>>   					else
>> -						OSAL_STRNCPY(bit_name,
>> -							     p_aeu->bit_name,
>> -							     30);
>> +						strlcpy(bit_name,
>> +							p_aeu->bit_name,
>> +							sizeof(bit_name));
>>
>>   					/* We now need to pass bitmask in its
>>   					 * correct position.
> 
> I'd say it should be better to change OSAL_STRNCPY to OSAL_STRLCPY and
> modify the macro to use strlcpy, so we avoid further uses of that strlcpy.
> 
> However, this modifies base driver code, so it is up to the maintainers to make that decision.
> (CC'ing maintainers here).

There's no value for any OSAL_* that simply defines itself to be the 
same as the direct api, as does OSAL_STRNCPY.

It's better to just remove any OSAL_* that calls straight through since 
all it does is obfuscate what the code does, for no benefit.

> Also, missing fixes line and CC stable.
> 
> Fixes: 8427c6647964 ("net/qede/base: add attention formatting string")

The idea of this "Fixes" thing is to look up in git blame what is being 
edited is it?  If so what's the benefit of that over using git if you 
ever want to know that?

-Andy

> Cc: stable@dpdk.org
> 

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

[De Lara Guarch, Pablo]

> -----Original Message-----
> From: Andy Green [mailto:andy@warmcat.com]
> Sent: Friday, May 11, 2018 11:48 AM
> To: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>; dev@dpdk.org
> Cc: stable@dpdk.org; Mody, Rasesh <Rasesh.Mody@cavium.com>; Patil, Harish
> <Harish.Patil@cavium.com>; Shahed.Shaikh@cavium.com
> Subject: Re: [dpdk-dev] [PATCH v4 09/18] net/qede: strncpy length constant and
> NUL
> 
> 
> 
> On 05/11/2018 06:43 PM, De Lara Guarch, Pablo wrote:
> >
> >
> >> -----Original Message-----
> >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Andy Green
> >> Sent: Friday, May 11, 2018 2:46 AM
> >> To: dev@dpdk.org
> >> Subject: [dpdk-dev] [PATCH v4 09/18] net/qede: strncpy length
> >> constant and NUL
> >>
> >> Signed-off-by: Andy Green <andy@warmcat.com>
> >> ---
> >>   drivers/net/qede/base/ecore_int.c |    8 +++++---
> >>   1 file changed, 5 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/net/qede/base/ecore_int.c
> >> b/drivers/net/qede/base/ecore_int.c
> >> index f43781ba4..d9e22b5ed 100644
> >> --- a/drivers/net/qede/base/ecore_int.c
> >> +++ b/drivers/net/qede/base/ecore_int.c
> >> @@ -6,6 +6,8 @@
> >>    * See LICENSE.qede_pmd for copyright and licensing details.
> >>    */
> >>
> >> +#include <rte_string_fns.h>
> >> +
> >>   #include "bcm_osal.h"
> >>   #include "ecore.h"
> >>   #include "ecore_spq.h"
> >> @@ -1104,9 +1106,9 @@ static enum _ecore_status_t
> >> ecore_int_deassertion(struct ecore_hwfn *p_hwfn,
> >>   							      p_aeu->bit_name,
> >>   							      num);
> >>   					else
> >> -						OSAL_STRNCPY(bit_name,
> >> -							     p_aeu->bit_name,
> >> -							     30);
> >> +						strlcpy(bit_name,
> >> +							p_aeu->bit_name,
> >> +							sizeof(bit_name));
> >>
> >>   					/* We now need to pass bitmask in its
> >>   					 * correct position.
> >
> > I'd say it should be better to change OSAL_STRNCPY to OSAL_STRLCPY and
> > modify the macro to use strlcpy, so we avoid further uses of that strlcpy.
> >
> > However, this modifies base driver code, so it is up to the maintainers to make
> that decision.
> > (CC'ing maintainers here).
> 
> There's no value for any OSAL_* that simply defines itself to be the same as the
> direct api, as does OSAL_STRNCPY.
> 
> It's better to just remove any OSAL_* that calls straight through since all it does
> is obfuscate what the code does, for no benefit.

I agree. Since this is modifying base driver code,
the maintainers can decide what to do with this.

> 
> > Also, missing fixes line and CC stable.
> >
> > Fixes: 8427c6647964 ("net/qede/base: add attention formatting string")
> 
> The idea of this "Fixes" thing is to look up in git blame what is being edited is it?
> If so what's the benefit of that over using git if you ever want to know that?
> 

This is important mainly to see which releases needed this patch backported.
I am replying to your patches with this info, to save you some time
(I know you are feeling the pain of this overhead :P).

> -Andy
> 
> > Cc: stable@dpdk.org
> >

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

[Andy Green]

On 05/11/2018 08:48 PM, De Lara Guarch, Pablo wrote:
> 
> 
>> -----Original Message-----
>> From: Andy Green [mailto:andy@warmcat.com]
>> Sent: Friday, May 11, 2018 11:48 AM
>> To: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>; dev@dpdk.org
>> Cc: stable@dpdk.org; Mody, Rasesh <Rasesh.Mody@cavium.com>; Patil, Harish
>> <Harish.Patil@cavium.com>; Shahed.Shaikh@cavium.com
>> Subject: Re: [dpdk-dev] [PATCH v4 09/18] net/qede: strncpy length constant and
>> NUL
>>
>>
>>
>> On 05/11/2018 06:43 PM, De Lara Guarch, Pablo wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Andy Green
>>>> Sent: Friday, May 11, 2018 2:46 AM
>>>> To: dev@dpdk.org
>>>> Subject: [dpdk-dev] [PATCH v4 09/18] net/qede: strncpy length
>>>> constant and NUL
>>>>
>>>> Signed-off-by: Andy Green <andy@warmcat.com>
>>>> ---
>>>>    drivers/net/qede/base/ecore_int.c |    8 +++++---
>>>>    1 file changed, 5 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/net/qede/base/ecore_int.c
>>>> b/drivers/net/qede/base/ecore_int.c
>>>> index f43781ba4..d9e22b5ed 100644
>>>> --- a/drivers/net/qede/base/ecore_int.c
>>>> +++ b/drivers/net/qede/base/ecore_int.c
>>>> @@ -6,6 +6,8 @@
>>>>     * See LICENSE.qede_pmd for copyright and licensing details.
>>>>     */
>>>>
>>>> +#include <rte_string_fns.h>
>>>> +
>>>>    #include "bcm_osal.h"
>>>>    #include "ecore.h"
>>>>    #include "ecore_spq.h"
>>>> @@ -1104,9 +1106,9 @@ static enum _ecore_status_t
>>>> ecore_int_deassertion(struct ecore_hwfn *p_hwfn,
>>>>    							      p_aeu->bit_name,
>>>>    							      num);
>>>>    					else
>>>> -						OSAL_STRNCPY(bit_name,
>>>> -							     p_aeu->bit_name,
>>>> -							     30);
>>>> +						strlcpy(bit_name,
>>>> +							p_aeu->bit_name,
>>>> +							sizeof(bit_name));
>>>>
>>>>    					/* We now need to pass bitmask in its
>>>>    					 * correct position.
>>>
>>> I'd say it should be better to change OSAL_STRNCPY to OSAL_STRLCPY and
>>> modify the macro to use strlcpy, so we avoid further uses of that strlcpy.
>>>
>>> However, this modifies base driver code, so it is up to the maintainers to make
>> that decision.
>>> (CC'ing maintainers here).
>>
>> There's no value for any OSAL_* that simply defines itself to be the same as the
>> direct api, as does OSAL_STRNCPY.
>>
>> It's better to just remove any OSAL_* that calls straight through since all it does
>> is obfuscate what the code does, for no benefit.
> 
> I agree. Since this is modifying base driver code,
> the maintainers can decide what to do with this.
> 
>>
>>> Also, missing fixes line and CC stable.
>>>
>>> Fixes: 8427c6647964 ("net/qede/base: add attention formatting string")
>>
>> The idea of this "Fixes" thing is to look up in git blame what is being edited is it?
>> If so what's the benefit of that over using git if you ever want to know that?
>>
> 
> This is important mainly to see which releases needed this patch backported.
> I am replying to your patches with this info, to save you some time
> (I know you are feeling the pain of this overhead :P).

Yeah: I appreciate the help.  Some of the current rules make assumptions 
about burning time for no real benefit that assume the contributor has 
no choice but to comply.  But that is simply not true for all potential 
contributors.

I will integrate the comments tomorrow morning (ie, +12h) and push 
again.  I will look closer at the missing include thing, but since build 
stops, telling me it can't find the include file, and the patch fixes 
it, there are a limited amount of possible root causes.

-Andy

> 
>> -Andy
>>
>>> Cc: stable@dpdk.org
>>>

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

[De Lara Guarch, Pablo]

> -----Original Message-----
> From: Andy Green [mailto:andy@warmcat.com]
> Sent: Friday, May 11, 2018 2:39 PM
> To: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>; dev@dpdk.org
> Cc: stable@dpdk.org; Mody, Rasesh <Rasesh.Mody@cavium.com>; Patil, Harish
> <Harish.Patil@cavium.com>; Shahed.Shaikh@cavium.com
> Subject: Re: [dpdk-dev] [PATCH v4 09/18] net/qede: strncpy length constant and
> NUL
> 
> 
> 
> On 05/11/2018 08:48 PM, De Lara Guarch, Pablo wrote:
> >
> >
> >> -----Original Message-----
> >> From: Andy Green [mailto:andy@warmcat.com]
> >> Sent: Friday, May 11, 2018 11:48 AM
> >> To: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>;
> >> dev@dpdk.org
> >> Cc: stable@dpdk.org; Mody, Rasesh <Rasesh.Mody@cavium.com>; Patil,
> >> Harish <Harish.Patil@cavium.com>; Shahed.Shaikh@cavium.com
> >> Subject: Re: [dpdk-dev] [PATCH v4 09/18] net/qede: strncpy length
> >> constant and NUL
> >>
> >>
> >>
> >> On 05/11/2018 06:43 PM, De Lara Guarch, Pablo wrote:
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Andy Green
> >>>> Sent: Friday, May 11, 2018 2:46 AM
> >>>> To: dev@dpdk.org
> >>>> Subject: [dpdk-dev] [PATCH v4 09/18] net/qede: strncpy length
> >>>> constant and NUL
> >>>>
> >>>> Signed-off-by: Andy Green <andy@warmcat.com>
> >>>> ---
> >>>>    drivers/net/qede/base/ecore_int.c |    8 +++++---
> >>>>    1 file changed, 5 insertions(+), 3 deletions(-)
> >>>>
> >>>> diff --git a/drivers/net/qede/base/ecore_int.c
> >>>> b/drivers/net/qede/base/ecore_int.c
> >>>> index f43781ba4..d9e22b5ed 100644
> >>>> --- a/drivers/net/qede/base/ecore_int.c
> >>>> +++ b/drivers/net/qede/base/ecore_int.c
> >>>> @@ -6,6 +6,8 @@
> >>>>     * See LICENSE.qede_pmd for copyright and licensing details.
> >>>>     */
> >>>>
> >>>> +#include <rte_string_fns.h>
> >>>> +
> >>>>    #include "bcm_osal.h"
> >>>>    #include "ecore.h"
> >>>>    #include "ecore_spq.h"
> >>>> @@ -1104,9 +1106,9 @@ static enum _ecore_status_t
> >>>> ecore_int_deassertion(struct ecore_hwfn *p_hwfn,
> >>>>    							      p_aeu->bit_name,
> >>>>    							      num);
> >>>>    					else
> >>>> -						OSAL_STRNCPY(bit_name,
> >>>> -							     p_aeu->bit_name,
> >>>> -							     30);
> >>>> +						strlcpy(bit_name,
> >>>> +							p_aeu->bit_name,
> >>>> +							sizeof(bit_name));
> >>>>
> >>>>    					/* We now need to pass bitmask in its
> >>>>    					 * correct position.
> >>>
> >>> I'd say it should be better to change OSAL_STRNCPY to OSAL_STRLCPY
> >>> and modify the macro to use strlcpy, so we avoid further uses of that
> strlcpy.
> >>>
> >>> However, this modifies base driver code, so it is up to the
> >>> maintainers to make
> >> that decision.
> >>> (CC'ing maintainers here).
> >>
> >> There's no value for any OSAL_* that simply defines itself to be the
> >> same as the direct api, as does OSAL_STRNCPY.
> >>
> >> It's better to just remove any OSAL_* that calls straight through
> >> since all it does is obfuscate what the code does, for no benefit.
> >
> > I agree. Since this is modifying base driver code, the maintainers can
> > decide what to do with this.
> >
> >>
> >>> Also, missing fixes line and CC stable.
> >>>
> >>> Fixes: 8427c6647964 ("net/qede/base: add attention formatting
> >>> string")
> >>
> >> The idea of this "Fixes" thing is to look up in git blame what is being edited is
> it?
> >> If so what's the benefit of that over using git if you ever want to know that?
> >>
> >
> > This is important mainly to see which releases needed this patch backported.
> > I am replying to your patches with this info, to save you some time (I
> > know you are feeling the pain of this overhead :P).
> 
> Yeah: I appreciate the help.  Some of the current rules make assumptions about
> burning time for no real benefit that assume the contributor has no choice but
> to comply.  But that is simply not true for all potential contributors.
> 
> I will integrate the comments tomorrow morning (ie, +12h) and push again.  I
> will look closer at the missing include thing, but since build stops, telling me it
> can't find the include file, and the patch fixes it, there are a limited amount of
> possible root causes.

Thanks for your time, Andy.

Pablo

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

[Shaikh, Shahed]

> > >>
> > >> +#include <rte_string_fns.h>
> > >> +
> > >>   #include "bcm_osal.h"
> > >>   #include "ecore.h"
> > >>   #include "ecore_spq.h"
> > >> @@ -1104,9 +1106,9 @@ static enum _ecore_status_t
> > >> ecore_int_deassertion(struct ecore_hwfn *p_hwfn,
> > >>   							      p_aeu->bit_name,
> > >>   							      num);
> > >>   					else
> > >> -						OSAL_STRNCPY(bit_name,
> > >> -							     p_aeu->bit_name,
> > >> -							     30);
> > >> +						strlcpy(bit_name,
> > >> +							p_aeu->bit_name,
> > >> +							sizeof(bit_name));
> > >>
> > >>   					/* We now need to pass bitmask in its
> > >>   					 * correct position.
> > >
> > > I'd say it should be better to change OSAL_STRNCPY to OSAL_STRLCPY
> > > and modify the macro to use strlcpy, so we avoid further uses of that strlcpy.
> > >
> > > However, this modifies base driver code, so it is up to the
> > > maintainers to make
> > that decision.
> > > (CC'ing maintainers here).
> >
> > There's no value for any OSAL_* that simply defines itself to be the
> > same as the direct api, as does OSAL_STRNCPY.
> >
> > It's better to just remove any OSAL_* that calls straight through
> > since all it does is obfuscate what the code does, for no benefit.
> 
> I agree. Since this is modifying base driver code, the maintainers can decide
> what to do with this.

Hi,

For this series, you can continue with s/OSAL_STRNCPY/strlcpy/ for this instance.
I will send a patch to cleanup OSAL_* once your series gets applied.

Thanks,
Shahed

[dpdk-dev] [PATCH v4 10/18] net/qede: solve 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);
    ~~~~~~~~~~~~~~~~~~~~~~~~~

Signed-off-by: Andy Green <andy@warmcat.com>
---
 drivers/net/qede/qede_main.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/net/qede/qede_main.c b/drivers/net/qede/qede_main.c
index 2333ca073..fcfc32d0d 100644
--- a/drivers/net/qede/qede_main.c
+++ b/drivers/net/qede/qede_main.c
@@ -9,6 +9,7 @@
 #include <limits.h>
 #include <time.h>
 #include <rte_alarm.h>
+#include <rte_string_fns.h>
 
 #include "qede_ethdev.h"
 
@@ -303,9 +304,9 @@ static int qed_slowpath_start(struct ecore_dev *edev,
 		drv_version.version = (params->drv_major << 24) |
 		    (params->drv_minor << 16) |
 		    (params->drv_rev << 8) | (params->drv_eng);
-		/* TBD: strlcpy() */
-		strncpy((char *)drv_version.name, (const char *)params->name,
-			MCP_DRV_VER_STR_SIZE - 4);
+		strlcpy((char *)drv_version.name, (const char *)params->name,
+			sizeof(drv_version.name));
+		drv_version.name[sizeof(drv_version.name) - 1] = '\0';
 		rc = ecore_mcp_send_drv_version(hwfn, hwfn->p_main_ptt,
 						&drv_version);
 		if (rc) {

Re: [dpdk-dev] [PATCH v4 10/18] net/qede: solve broken strncpy

[De Lara Guarch, Pablo]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Andy Green
> Sent: Friday, May 11, 2018 2:46 AM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH v4 10/18] net/qede: solve broken strncpy
> 
> /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);
>     ~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> Signed-off-by: Andy Green <andy@warmcat.com>
> ---
>  drivers/net/qede/qede_main.c |    7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/qede/qede_main.c b/drivers/net/qede/qede_main.c
> index 2333ca073..fcfc32d0d 100644
> --- a/drivers/net/qede/qede_main.c
> +++ b/drivers/net/qede/qede_main.c
> @@ -9,6 +9,7 @@
>  #include <limits.h>
>  #include <time.h>
>  #include <rte_alarm.h>
> +#include <rte_string_fns.h>
> 
>  #include "qede_ethdev.h"
> 
> @@ -303,9 +304,9 @@ static int qed_slowpath_start(struct ecore_dev *edev,
>  		drv_version.version = (params->drv_major << 24) |
>  		    (params->drv_minor << 16) |
>  		    (params->drv_rev << 8) | (params->drv_eng);
> -		/* TBD: strlcpy() */
> -		strncpy((char *)drv_version.name, (const char *)params->name,
> -			MCP_DRV_VER_STR_SIZE - 4);
> +		strlcpy((char *)drv_version.name, (const char *)params->name,
> +			sizeof(drv_version.name));
> +		drv_version.name[sizeof(drv_version.name) - 1] = '\0';

Strlcpy already terminates the buffer with NULL character, so this last line is not needed.

Also, missing fix line and CC stable

Fixes: 86a2265e59d7 ("qede: add SRIOV support")
Cc: stable@dpdk.org

[dpdk-dev] [PATCH v4 11/18] net/sfc: correct strncpy length

[Andy Green]

Signed-off-by: Andy Green <andy@warmcat.com>
---
 drivers/net/sfc/sfc_ethdev.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/sfc/sfc_ethdev.c b/drivers/net/sfc/sfc_ethdev.c
index e42d55350..ef5e9ecb2 100644
--- a/drivers/net/sfc/sfc_ethdev.c
+++ b/drivers/net/sfc/sfc_ethdev.c
@@ -13,6 +13,7 @@
 #include <rte_pci.h>
 #include <rte_bus_pci.h>
 #include <rte_errno.h>
+#include <rte_string_fns.h>
 
 #include "efx.h"
 
@@ -741,9 +742,8 @@ sfc_xstats_get_names_by_id(struct rte_eth_dev *dev,
 		if ((ids == NULL) || (ids[nb_written] == nb_supported)) {
 			char *name = xstats_names[nb_written++].name;
 
-			strncpy(name, efx_mac_stat_name(sa->nic, i),
+			strlcpy(name, efx_mac_stat_name(sa->nic, i),
 				sizeof(xstats_names[0].name));
-			name[sizeof(xstats_names[0].name) - 1] = '\0';
 		}
 
 		++nb_supported;

Re: [dpdk-dev] [PATCH v4 11/18] net/sfc: correct strncpy length

[Andrew Rybchenko]

On 05/11/2018 04:46 AM, Andy Green wrote:
> Signed-off-by: Andy Green <andy@warmcat.com>
> ---
>   drivers/net/sfc/sfc_ethdev.c |    4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/sfc/sfc_ethdev.c b/drivers/net/sfc/sfc_ethdev.c
> index e42d55350..ef5e9ecb2 100644
> --- a/drivers/net/sfc/sfc_ethdev.c
> +++ b/drivers/net/sfc/sfc_ethdev.c
> @@ -13,6 +13,7 @@
>   #include <rte_pci.h>
>   #include <rte_bus_pci.h>
>   #include <rte_errno.h>
> +#include <rte_string_fns.h>
>   
>   #include "efx.h"
>   
> @@ -741,9 +742,8 @@ sfc_xstats_get_names_by_id(struct rte_eth_dev *dev,
>   		if ((ids == NULL) || (ids[nb_written] == nb_supported)) {
>   			char *name = xstats_names[nb_written++].name;
>   
> -			strncpy(name, efx_mac_stat_name(sa->nic, i),
> +			strlcpy(name, efx_mac_stat_name(sa->nic, i),
>   				sizeof(xstats_names[0].name));
> -			name[sizeof(xstats_names[0].name) - 1] = '\0';
>   		}
>   
>   		++nb_supported;
>

Acked-by: Andrew Rybchenko <arybchenko@oktetlabs.ru>

Re: [dpdk-dev] [PATCH v4 11/18] net/sfc: correct strncpy length

[De Lara Guarch, Pablo]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Andy Green
> Sent: Friday, May 11, 2018 2:46 AM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH v4 11/18] net/sfc: correct strncpy length
> 
> Signed-off-by: Andy Green <andy@warmcat.com>
> ---
>  drivers/net/sfc/sfc_ethdev.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/sfc/sfc_ethdev.c b/drivers/net/sfc/sfc_ethdev.c index
> e42d55350..ef5e9ecb2 100644
> --- a/drivers/net/sfc/sfc_ethdev.c
> +++ b/drivers/net/sfc/sfc_ethdev.c
> @@ -13,6 +13,7 @@
>  #include <rte_pci.h>
>  #include <rte_bus_pci.h>
>  #include <rte_errno.h>
> +#include <rte_string_fns.h>
> 
>  #include "efx.h"
> 
> @@ -741,9 +742,8 @@ sfc_xstats_get_names_by_id(struct rte_eth_dev *dev,
>  		if ((ids == NULL) || (ids[nb_written] == nb_supported)) {
>  			char *name = xstats_names[nb_written++].name;
> 
> -			strncpy(name, efx_mac_stat_name(sa->nic, i),
> +			strlcpy(name, efx_mac_stat_name(sa->nic, i),
>  				sizeof(xstats_names[0].name));

Shouldn't this be "sizeof(name)"? Although probably it is the same.

Missing fixes line and CC stable.

Fixes: 73280c1e4ff2 ("net/sfc: support xstats retrieval by ID")
Cc: stable@dpdk.org

Re: [dpdk-dev] [PATCH v4 11/18] net/sfc: correct strncpy length

[Andy Green]

On 05/11/2018 06:51 PM, De Lara Guarch, Pablo wrote:
> 
> 
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Andy Green
>> Sent: Friday, May 11, 2018 2:46 AM
>> To: dev@dpdk.org
>> Subject: [dpdk-dev] [PATCH v4 11/18] net/sfc: correct strncpy length
>>
>> Signed-off-by: Andy Green <andy@warmcat.com>
>> ---
>>   drivers/net/sfc/sfc_ethdev.c |    4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/sfc/sfc_ethdev.c b/drivers/net/sfc/sfc_ethdev.c index
>> e42d55350..ef5e9ecb2 100644
>> --- a/drivers/net/sfc/sfc_ethdev.c
>> +++ b/drivers/net/sfc/sfc_ethdev.c
>> @@ -13,6 +13,7 @@
>>   #include <rte_pci.h>
>>   #include <rte_bus_pci.h>
>>   #include <rte_errno.h>
>> +#include <rte_string_fns.h>
>>
>>   #include "efx.h"
>>
>> @@ -741,9 +742,8 @@ sfc_xstats_get_names_by_id(struct rte_eth_dev *dev,
>>   		if ((ids == NULL) || (ids[nb_written] == nb_supported)) {
>>   			char *name = xstats_names[nb_written++].name;
>>
>> -			strncpy(name, efx_mac_stat_name(sa->nic, i),
>> +			strlcpy(name, efx_mac_stat_name(sa->nic, i),
>>   				sizeof(xstats_names[0].name));
> 
> Shouldn't this be "sizeof(name)"? Although probably it is the same.

name is just a pointer, so sizeof that is 4 or 8.

-Andy

> Missing fixes line and CC stable.
> 
> Fixes: 73280c1e4ff2 ("net/sfc: support xstats retrieval by ID")
> Cc: stable@dpdk.org
> 

[dpdk-dev] [PATCH v4 12/18] net/sfc: solve strncpy size and NUL

[Andy Green]

Signed-off-by: Andy Green <andy@warmcat.com>
---
 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 ef5e9ecb2..a8c0f8e19 100644
--- a/drivers/net/sfc/sfc_ethdev.c
+++ b/drivers/net/sfc/sfc_ethdev.c
@@ -664,7 +664,7 @@ 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)
-				strncpy(xstats_names[nstats].name,
+				strlcpy(xstats_names[nstats].name,
 					efx_mac_stat_name(sa->nic, i),
 					sizeof(xstats_names[0].name));
 			nstats++;

Re: [dpdk-dev] [PATCH v4 12/18] net/sfc: solve strncpy size and NUL

[Andrew Rybchenko]

On 05/11/2018 04:46 AM, Andy Green wrote:
> Signed-off-by: Andy Green <andy@warmcat.com>
> ---
>   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 ef5e9ecb2..a8c0f8e19 100644
> --- a/drivers/net/sfc/sfc_ethdev.c
> +++ b/drivers/net/sfc/sfc_ethdev.c
> @@ -664,7 +664,7 @@ 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)
> -				strncpy(xstats_names[nstats].name,
> +				strlcpy(xstats_names[nstats].name,
>   					efx_mac_stat_name(sa->nic, i),
>   					sizeof(xstats_names[0].name));
>   			nstats++;
>

I'd suggest:
net/sfc: make sure that copied stats name is NUL-terminated

Acked-by: Andrew Rybchenko <arybchenko@oktetlabs.ru>

Re: [dpdk-dev] [PATCH v4 12/18] net/sfc: solve strncpy size and NUL

[De Lara Guarch, Pablo]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Andy Green
> Sent: Friday, May 11, 2018 2:46 AM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH v4 12/18] net/sfc: solve strncpy size and NUL
> 
> Signed-off-by: Andy Green <andy@warmcat.com>
> ---
>  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
> ef5e9ecb2..a8c0f8e19 100644
> --- a/drivers/net/sfc/sfc_ethdev.c
> +++ b/drivers/net/sfc/sfc_ethdev.c
> @@ -664,7 +664,7 @@ 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)
> -				strncpy(xstats_names[nstats].name,
> +				strlcpy(xstats_names[nstats].name,
>  					efx_mac_stat_name(sa->nic, i),
>  					sizeof(xstats_names[0].name));
>  			nstats++;

I'd  say this patch could be squashed with the previous one, as they are solving the same issue
in the same file.
It also needs an extra fixes line (so the final patch would have two fixes lines) and CC stable too.

Fixes: 7b9891769f4b ("net/sfc: support extended statistics")
Cc: stable@dpdk.org

Re: [dpdk-dev] [PATCH v4 12/18] net/sfc: solve strncpy size and NUL

[Andy Green]

On 05/11/2018 06:55 PM, De Lara Guarch, Pablo wrote:
> 
> 
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Andy Green
>> Sent: Friday, May 11, 2018 2:46 AM
>> To: dev@dpdk.org
>> Subject: [dpdk-dev] [PATCH v4 12/18] net/sfc: solve strncpy size and NUL
>>
>> Signed-off-by: Andy Green <andy@warmcat.com>
>> ---
>>   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
>> ef5e9ecb2..a8c0f8e19 100644
>> --- a/drivers/net/sfc/sfc_ethdev.c
>> +++ b/drivers/net/sfc/sfc_ethdev.c
>> @@ -664,7 +664,7 @@ 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)
>> -				strncpy(xstats_names[nstats].name,
>> +				strlcpy(xstats_names[nstats].name,
>>   					efx_mac_stat_name(sa->nic, i),
>>   					sizeof(xstats_names[0].name));
>>   			nstats++;
> 
> I'd  say this patch could be squashed with the previous one, as they are solving the same issue
> in the same file.
> It also needs an extra fixes line (so the final patch would have two fixes lines) and CC stable too.

OK it's adapted accordingly.

-Andy

> Fixes: 7b9891769f4b ("net/sfc: support extended statistics")
> Cc: stable@dpdk.org
> 

[dpdk-dev] [PATCH v4 13/18] net/vdev_netvsc: 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);
  ^~~

Signed-off-by: Andy Green <andy@warmcat.com>
---
 drivers/net/vdev_netvsc/vdev_netvsc.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/net/vdev_netvsc/vdev_netvsc.c b/drivers/net/vdev_netvsc/vdev_netvsc.c
index c321a9f1b..dca25761d 100644
--- a/drivers/net/vdev_netvsc/vdev_netvsc.c
+++ b/drivers/net/vdev_netvsc/vdev_netvsc.c
@@ -327,12 +327,14 @@ 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(in) - 1, "/sys/class/net/%s/%s",
+		       if_name, relpath);
+	if (ret == -1 || (size_t)ret >= sizeof(in) - 1)
 		return -ENOBUFS;
-	ret = readlink(buf, buf, size);
+	ret = readlink(in, buf, size);
 	if (ret == -1)
 		return -errno;
 	if ((size_t)ret >= size - 1)

Re: [dpdk-dev] [PATCH v4 13/18] net/vdev_netvsc: readlink inputs cannot be aliased

[De Lara Guarch, Pablo]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Andy Green
> Sent: Friday, May 11, 2018 2:46 AM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH v4 13/18] net/vdev_netvsc: readlink inputs cannot
> be aliased
> 
> /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);
>   ^~~
> 
> Signed-off-by: Andy Green <andy@warmcat.com>

Missing fixes line and CC stable:

Fixes: e7dc5d7becc5 ("net/vdev_netvsc: implement core functionality")

Acked-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>

[dpdk-dev] [PATCH v4 14/18] net/vdev_netvsc: 3 x strncpy misuse

[Andy Green]

Signed-off-by: Andy Green <andy@warmcat.com>
---
 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 dca25761d..f1d036152 100644
--- a/drivers/net/vdev_netvsc/vdev_netvsc.c
+++ b/drivers/net/vdev_netvsc/vdev_netvsc.c
@@ -35,6 +35,7 @@
 #include <rte_hypervisor.h>
 #include <rte_kvargs.h>
 #include <rte_log.h>
+#include <rte_string_fns.h>
 
 #define VDEV_NETVSC_DRIVER net_vdev_netvsc
 #define VDEV_NETVSC_DRIVER_NAME RTE_STR(VDEV_NETVSC_DRIVER)
@@ -182,7 +183,7 @@ 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));
+		strlcpy(req.ifr_name, iface[i].if_name, sizeof(req.ifr_name));
 		if (ioctl(s, SIOCGIFHWADDR, &req) == -1) {
 			DRV_LOG(WARNING, "cannot retrieve information about"
 					 " interface \"%s\": %s",
@@ -384,7 +385,7 @@ 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));
+		strlcpy(ctx->if_name, iface->if_name, sizeof(ctx->if_name));
 		return 0;
 	}
 	if (!is_same_ether_addr(eth_addr, &ctx->if_addr))
@@ -582,7 +583,7 @@ 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));
+	strlcpy(ctx->if_name, iface->if_name, sizeof(ctx->if_name));
 	ctx->if_index = iface->if_index;
 	ctx->if_addr = *eth_addr;
 	ctx->pipe[0] = -1;

Re: [dpdk-dev] [PATCH v4 14/18] net/vdev_netvsc: 3 x strncpy misuse

[De Lara Guarch, Pablo]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Andy Green
> Sent: Friday, May 11, 2018 2:46 AM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH v4 14/18] net/vdev_netvsc: 3 x strncpy misuse
> 
> Signed-off-by: Andy Green <andy@warmcat.com>

Missing fixes line and CC stable:

Fixes: e7dc5d7becc5 ("net/vdev_netvsc: implement core functionality")
Cc: stable@dpdk.org

Acked-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>

[dpdk-dev] [PATCH v4 15/18] app: 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>
          ^~~~~~~~~~~~~~~~

Signed-off-by: Andy Green <andy@warmcat.com>
---
 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

Re: [dpdk-dev] [PATCH v4 15/18] app: can't find include

[De Lara Guarch, Pablo]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Andy Green
> Sent: Friday, May 11, 2018 2:47 AM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH v4 15/18] app: can't find include
> 
> /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>
>           ^~~~~~~~~~~~~~~~
> 
> Signed-off-by: Andy Green <andy@warmcat.com>

As I said in the v3, I think this patch is not needed.

Re: [dpdk-dev] [PATCH v4 15/18] app: can't find include

[Andy Green]

On 05/11/2018 07:04 PM, De Lara Guarch, Pablo wrote:
> 
> 
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Andy Green
>> Sent: Friday, May 11, 2018 2:47 AM
>> To: dev@dpdk.org
>> Subject: [dpdk-dev] [PATCH v4 15/18] app: can't find include
>>
>> /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>
>>            ^~~~~~~~~~~~~~~~
>>
>> Signed-off-by: Andy Green <andy@warmcat.com>
> 
> As I said in the v3, I think this patch is not needed.
> 

Do you have an idea why the actual error pasted in the patch is coming 
without this patch?

-Andy

Re: [dpdk-dev] [PATCH v4 15/18] app: can't find include

[De Lara Guarch, Pablo]

> -----Original Message-----
> From: Andy Green [mailto:andy@warmcat.com]
> Sent: Friday, May 11, 2018 12:13 PM
> To: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v4 15/18] app: can't find include
> 
> 
> 
> On 05/11/2018 07:04 PM, De Lara Guarch, Pablo wrote:
> >
> >
> >> -----Original Message-----
> >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Andy Green
> >> Sent: Friday, May 11, 2018 2:47 AM
> >> To: dev@dpdk.org
> >> Subject: [dpdk-dev] [PATCH v4 15/18] app: can't find include
> >>
> >> /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>
> >>            ^~~~~~~~~~~~~~~~
> >>
> >> Signed-off-by: Andy Green <andy@warmcat.com>
> >
> > As I said in the v3, I think this patch is not needed.
> >
> 
> Do you have an idea why the actual error pasted in the patch is coming without
> this patch?

Without this patch, do you see the file in ./build/include?
The file should be copied in there. If it is not there, it wasn't built and that won't fix the issue.
DPDK builds ok for me on Fedora 28/gcc 8 without this patch.

> 
> -Andy

Re: [dpdk-dev] [PATCH v4 15/18] app: can't find include

[Andy Green]

On 05/11/2018 09:20 PM, De Lara Guarch, Pablo wrote:
> 
> 
>> -----Original Message-----
>> From: Andy Green [mailto:andy@warmcat.com]
>> Sent: Friday, May 11, 2018 12:13 PM
>> To: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>; dev@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH v4 15/18] app: can't find include
>>
>>
>>
>> On 05/11/2018 07:04 PM, De Lara Guarch, Pablo wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Andy Green
>>>> Sent: Friday, May 11, 2018 2:47 AM
>>>> To: dev@dpdk.org
>>>> Subject: [dpdk-dev] [PATCH v4 15/18] app: can't find include
>>>>
>>>> /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>
>>>>             ^~~~~~~~~~~~~~~~
>>>>
>>>> Signed-off-by: Andy Green <andy@warmcat.com>
>>>
>>> As I said in the v3, I think this patch is not needed.
>>>
>>
>> Do you have an idea why the actual error pasted in the patch is coming without
>> this patch?
> 
> Without this patch, do you see the file in ./build/include?
> The file should be copied in there. If it is not there, it wasn't built and that won't fix the issue.
> DPDK builds ok for me on Fedora 28/gcc 8 without this patch.

Hm... so it was there

$ ls -l build/include/rte_pmd_dpaa.h
lrwxrwxrwx. 1 agreen agreen 37 May 11 09:41 build/include/rte_pmd_dpaa.h 
-> ../../drivers/net/dpaa/rte_pmd_dpaa.h

I removed build/include and recooked it, the symlink was regenerated.

As far as I can tell, you're right it doesn't need the patch.  But it 
did earlier in the week for whatever reason...

-Andy


>>
>> -Andy

[dpdk-dev] [PATCH v4 16/18] app/proc-info: 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]);

Signed-off-by: Andy Green <andy@warmcat.com>
---
 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 {

Re: [dpdk-dev] [PATCH v4 16/18] app/proc-info: sprintf overrun bug

[De Lara Guarch, Pablo]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Andy Green
> Sent: Friday, May 11, 2018 2:47 AM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH v4 16/18] app/proc-info: sprintf overrun bug
> 
> /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]);
> 
> Signed-off-by: Andy Green <andy@warmcat.com>
> ---
>  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';

No need to NULL terminate, since snprintf already does it.

> +			if (n > sizeof(buf) - 1)
> +				n = sizeof(buf) - 1;

If (n > sizeof(buf) -1 ), it means that there wasn't enough space to write all the data,
So shouldn't we return an error here?

> +			ret = write(stdout_fd, buf, n);
>  			if (ret < 0)
>  				goto err;
>  		} else {

Missing fixes line and CC stable.

Fixes: 2deb6b5246d7 ("app/procinfo: add collectd format and host id")
Cc: stable@dpdk.org

Re: [dpdk-dev] [PATCH v4 16/18] app/proc-info: sprintf overrun bug

[Andy Green]

On 05/11/2018 08:26 PM, De Lara Guarch, Pablo wrote:
> 
> 
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Andy Green
>> Sent: Friday, May 11, 2018 2:47 AM
>> To: dev@dpdk.org
>> Subject: [dpdk-dev] [PATCH v4 16/18] app/proc-info: sprintf overrun bug
>>
>> /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]);
>>
>> Signed-off-by: Andy Green <andy@warmcat.com>
>> ---
>>   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';
> 
> No need to NULL terminate, since snprintf already does it.

OK.

>> +			if (n > sizeof(buf) - 1)
>> +				n = sizeof(buf) - 1;
> 
> If (n > sizeof(buf) -1 ), it means that there wasn't enough space to write all the data,
> So shouldn't we return an error here?

It's just logging stuff, policy about truncated overlength logs could go 
either way.

Prior to this patch its policy was to corrupt your stack if overlength 
;-) so I think it's moving it on in the right direction merely 
truncating it.

-Andy

>> +			ret = write(stdout_fd, buf, n);
>>   			if (ret < 0)
>>   				goto err;
>>   		} else {
> 
> Missing fixes line and CC stable.
> 
> Fixes: 2deb6b5246d7 ("app/procinfo: add collectd format and host id")
> Cc: stable@dpdk.org
> 

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

[Andy Green]

Signed-off-by: Andy Green <andy@warmcat.com>
---
 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 a37e35f4d..c574f2135 100644
--- a/app/test-bbdev/test_bbdev_vector.c
+++ b/app/test-bbdev/test_bbdev_vector.c
@@ -892,7 +892,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] == ','

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

[De Lara Guarch, Pablo]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Andy Green
> Sent: Friday, May 11, 2018 2:47 AM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH v4 17/18] app/test-bbdev: strcpy ok for allocated
> string
> 
> Signed-off-by: Andy Green <andy@warmcat.com>
> ---
>  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 a37e35f4d..c574f2135 100644
> --- a/app/test-bbdev/test_bbdev_vector.c
> +++ b/app/test-bbdev/test_bbdev_vector.c
> @@ -892,7 +892,7 @@ test_bbdev_vector_read(const char *filename,
>  		}
> 
>  		memset(entry, 0, strlen(line) + 1);
> -		strncpy(entry, line, strlen(line));
> +		strcpy(entry, line);

Better to use strlcpy(entry,line,sizeof(entry)) and remove the memset call.

Missing fixes line and CC stable:

Fixes: f714a18885a6 ("app/testbbdev: add test application for bbdev")
Cc: stable@dpdk.org

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

[Andy Green]

Signed-off-by: Andy Green <andy@warmcat.com>
---
 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 c574f2135..4a3ddcffe 100644
--- a/app/test-bbdev/test_bbdev_vector.c
+++ b/app/test-bbdev/test_bbdev_vector.c
@@ -914,7 +914,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;

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

[De Lara Guarch, Pablo]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Andy Green
> Sent: Friday, May 11, 2018 2:47 AM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH v4 18/18] app/test-bbdev: strcpy ok for allocated
> string
> 
> Signed-off-by: Andy Green <andy@warmcat.com>
> ---
>  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 c574f2135..4a3ddcffe 100644
> --- a/app/test-bbdev/test_bbdev_vector.c
> +++ b/app/test-bbdev/test_bbdev_vector.c
> @@ -914,7 +914,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 memset is removed in the previous patch, then we'll need to use strlcpy
here, to ensure NULL termination.

Missing fixes line and CC stable:

Fixes: f714a18885a6 ("app/testbbdev: add test application for bbdev")
Cc: stable@dpdk.org

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

[Andy Green]

On 05/11/2018 09:02 PM, De Lara Guarch, Pablo wrote:
> 
> 
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Andy Green
>> Sent: Friday, May 11, 2018 2:47 AM
>> To: dev@dpdk.org
>> Subject: [dpdk-dev] [PATCH v4 18/18] app/test-bbdev: strcpy ok for allocated
>> string
>>
>> Signed-off-by: Andy Green <andy@warmcat.com>
>> ---
>>   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 c574f2135..4a3ddcffe 100644
>> --- a/app/test-bbdev/test_bbdev_vector.c
>> +++ b/app/test-bbdev/test_bbdev_vector.c
>> @@ -914,7 +914,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 memset is removed in the previous patch, then we'll need to use strlcpy
> here, to ensure NULL termination.

No... the destination has been allocated dynamically so there is no case 
where the source length can exceed the destination.

I think for that reason it's OK to remove the memset() here, because for 
the same reason it will always exactly fill the dynamically sized string 
anyway.

-Andy

> Missing fixes line and CC stable:
> 
> Fixes: f714a18885a6 ("app/testbbdev: add test application for bbdev")
> Cc: stable@dpdk.org
> 

Re: [dpdk-dev] [PATCH v4 00/18] Fix default build on gcc8.0.1

[Neil Horman]

On Fri, May 11, 2018 at 09:45:15AM +0800, Andy Green wrote:
> This series allows dpdk master to build on Fedora 28,
> with the x86_64 default config.
> 
> ---
> 
> Andy Green (18):
>       devtools/check-git: provide more generic grep pattern
>       net/nfp: solve buffer overflow
>       bus/pci: replace strncpy dangerous code
>       bus/dpaa: solve inconsistent struct alignment
>       net/axgbe: solve broken eeprom string comp
>       net/nfp/nfpcore: solve strncpy misuse
>       net/nfp/nfpcore: off-by-one and no NUL on strncpy use
>       net/nfp: don't memcpy out of source range
>       net/qede: strncpy length constant and NUL
>       net/qede: solve broken strncpy
>       net/sfc: correct strncpy length
>       net/sfc: solve strncpy size and NUL
>       net/vdev_netvsc: readlink inputs cannot be aliased
>       net/vdev_netvsc: 3 x strncpy misuse
>       app: can't find include
>       app/proc-info: sprintf overrun bug
>       app/test-bbdev: strcpy ok for allocated string
>       app/test-bbdev: strcpy ok for allocated string
> 
> 
>  app/proc-info/main.c                       |    9 +++++++--
>  app/test-bbdev/test_bbdev_vector.c         |    5 +++--
>  app/test-pmd/Makefile                      |    1 +
>  devtools/check-git-log.sh                  |    4 ++--
>  drivers/bus/dpaa/base/qbman/qman.c         |   14 +++++++-------
>  drivers/bus/dpaa/include/fsl_qman.h        |   24 +++++++++++++-----------
>  drivers/bus/pci/linux/pci.c                |    2 +-
>  drivers/net/axgbe/axgbe_phy_impl.c         |    4 ++--
>  drivers/net/nfp/nfp_net.c                  |    6 +++---
>  drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c |    4 +++-
>  drivers/net/nfp/nfpcore/nfp_resource.c     |   10 ++++++----
>  drivers/net/qede/base/ecore_int.c          |    8 +++++---
>  drivers/net/qede/qede_main.c               |    7 ++++---
>  drivers/net/sfc/sfc_ethdev.c               |    6 +++---
>  drivers/net/vdev_netvsc/vdev_netvsc.c      |   15 +++++++++------
>  15 files changed, 69 insertions(+), 50 deletions(-)
> 
> --
> Signature
> 
Series
Acked-by: Neil Horman <nhorman@tuxdriver.com>