DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH v1 0/2] pci: a comment and a minor fix
@ 2020-05-13 10:47 Gaetan Rivet
  2020-05-13 10:47 ` [dpdk-dev] [PATCH v1 1/2] pci: fix allowing underflow when parsing PCI id Gaetan Rivet
  2020-05-13 10:47 ` [dpdk-dev] [PATCH v1 2/2] pci: explain how empty strings are rejected in DBDF Gaetan Rivet
  0 siblings, 2 replies; 7+ messages in thread
From: Gaetan Rivet @ 2020-05-13 10:47 UTC (permalink / raw)
  To: dev

Add a clarifying comment and fix the PCI addr parser behavior for
some edge-case.

Gaetan Rivet (2):
  pci: fix allowing underflow when parsing PCI id
  pci: explain how empty strings are rejected in DBDF

 lib/librte_pci/rte_pci.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

-- 
2.26.2


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [dpdk-dev] [PATCH v1 1/2] pci: fix allowing underflow when parsing PCI id
  2020-05-13 10:47 [dpdk-dev] [PATCH v1 0/2] pci: a comment and a minor fix Gaetan Rivet
@ 2020-05-13 10:47 ` Gaetan Rivet
  2020-05-14  8:52   ` Stojaczyk, Dariusz
  2020-05-19  9:17   ` David Marchand
  2020-05-13 10:47 ` [dpdk-dev] [PATCH v1 2/2] pci: explain how empty strings are rejected in DBDF Gaetan Rivet
  1 sibling, 2 replies; 7+ messages in thread
From: Gaetan Rivet @ 2020-05-13 10:47 UTC (permalink / raw)
  To: dev; +Cc: stable

The function strtoul will not return ERANGE if the input is negative, as
one might expect.

   0000:-FFFFFFFFFFFFFFFB:00.0

is not a better way to write 0000:05:00.0.
To simplify checking for '-', forbid using spaces before the field value.

   0000: 00:   2c.0

Should not be accepted.

Fixes: af75078fece3 ("first public release")
Cc: stable@dpdk.org
Signed-off-by: Gaetan Rivet <grive@u256.net>
---
 lib/librte_pci/rte_pci.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/lib/librte_pci/rte_pci.c b/lib/librte_pci/rte_pci.c
index d1ab6b414..e4ecdc32f 100644
--- a/lib/librte_pci/rte_pci.c
+++ b/lib/librte_pci/rte_pci.c
@@ -35,6 +35,12 @@ get_u8_pciaddr_field(const char *in, void *_u8, char dlm)
 	if (*in == '\0')
 		return NULL;
 
+	/* PCI field starting with spaces is forbidden.
+	 * Negative wrap-around is not reported as an error by strtoul.
+	 */
+	if (*in == ' ' || *in == '-')
+		return NULL;
+
 	errno = 0;
 	val = strtoul(in, &end, 16);
 	if (errno != 0 || end[0] != dlm || val > UINT8_MAX) {
@@ -70,6 +76,12 @@ pci_dbdf_parse(const char *input, struct rte_pci_addr *dev_addr)
 	unsigned long val;
 	char *end;
 
+	/* PCI id starting with spaces is forbidden.
+	 * Negative wrap-around is not reported as an error by strtoul.
+	 */
+	if (*in == ' ' || *in == '-')
+		return EINVAL;
+
 	errno = 0;
 	val = strtoul(in, &end, 16);
 	if (errno != 0 || end[0] != ':' || val > UINT16_MAX)
-- 
2.26.2


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [dpdk-dev] [PATCH v1 2/2] pci: explain how empty strings are rejected in DBDF
  2020-05-13 10:47 [dpdk-dev] [PATCH v1 0/2] pci: a comment and a minor fix Gaetan Rivet
  2020-05-13 10:47 ` [dpdk-dev] [PATCH v1 1/2] pci: fix allowing underflow when parsing PCI id Gaetan Rivet
@ 2020-05-13 10:47 ` Gaetan Rivet
  2020-05-14  8:53   ` Stojaczyk, Dariusz
  2020-05-19  9:18   ` David Marchand
  1 sibling, 2 replies; 7+ messages in thread
From: Gaetan Rivet @ 2020-05-13 10:47 UTC (permalink / raw)
  To: dev

Empty strings are forbidden as input to rte_pci_addr_parse().
It is explicitly enforced in BDF parsing as parsing the bus
field will immediately fail. The related check is commented.

It is implicitly enforced in DBDF parsing, as the domain would be
parsed to 0 without error, but the check `end[0] != ':'` afterward
will return -EINVAL.

Enforcing consistency between parsers by reading the code is not helped
by this property being implicit. Add a comment to explain.

Signed-off-by: Gaetan Rivet <grive@u256.net>
---
 lib/librte_pci/rte_pci.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/lib/librte_pci/rte_pci.c b/lib/librte_pci/rte_pci.c
index e4ecdc32f..60e6fbae7 100644
--- a/lib/librte_pci/rte_pci.c
+++ b/lib/librte_pci/rte_pci.c
@@ -84,6 +84,10 @@ pci_dbdf_parse(const char *input, struct rte_pci_addr *dev_addr)
 
 	errno = 0;
 	val = strtoul(in, &end, 16);
+	/* Empty string is not an error for strtoul, but the check
+	 *   end[0] != ':'
+	 * will detect the issue.
+	 */
 	if (errno != 0 || end[0] != ':' || val > UINT16_MAX)
 		return -EINVAL;
 	dev_addr->domain = (uint16_t)val;
-- 
2.26.2


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [dpdk-dev] [PATCH v1 1/2] pci: fix allowing underflow when parsing PCI id
  2020-05-13 10:47 ` [dpdk-dev] [PATCH v1 1/2] pci: fix allowing underflow when parsing PCI id Gaetan Rivet
@ 2020-05-14  8:52   ` Stojaczyk, Dariusz
  2020-05-19  9:17   ` David Marchand
  1 sibling, 0 replies; 7+ messages in thread
From: Stojaczyk, Dariusz @ 2020-05-14  8:52 UTC (permalink / raw)
  To: Gaetan Rivet, dev; +Cc: stable

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Gaetan Rivet
> Sent: Wednesday, May 13, 2020 12:48 PM
> To: dev@dpdk.org
> Cc: stable@dpdk.org
> Subject: [dpdk-dev] [PATCH v1 1/2] pci: fix allowing underflow when parsing PCI
> id
> 
> The function strtoul will not return ERANGE if the input is negative, as
> one might expect.
> 
>    0000:-FFFFFFFFFFFFFFFB:00.0
> 
> is not a better way to write 0000:05:00.0.
> To simplify checking for '-', forbid using spaces before the field value.
> 
>    0000: 00:   2c.0
> 
> Should not be accepted.
> 
> Fixes: af75078fece3 ("first public release")
> Cc: stable@dpdk.org
> Signed-off-by: Gaetan Rivet <grive@u256.net>
> ---

Acked-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [dpdk-dev] [PATCH v1 2/2] pci: explain how empty strings are rejected in DBDF
  2020-05-13 10:47 ` [dpdk-dev] [PATCH v1 2/2] pci: explain how empty strings are rejected in DBDF Gaetan Rivet
@ 2020-05-14  8:53   ` Stojaczyk, Dariusz
  2020-05-19  9:18   ` David Marchand
  1 sibling, 0 replies; 7+ messages in thread
From: Stojaczyk, Dariusz @ 2020-05-14  8:53 UTC (permalink / raw)
  To: Gaetan Rivet, dev

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Gaetan Rivet
> Sent: Wednesday, May 13, 2020 12:48 PM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH v1 2/2] pci: explain how empty strings are rejected
> in DBDF
> 
> Empty strings are forbidden as input to rte_pci_addr_parse().
> It is explicitly enforced in BDF parsing as parsing the bus
> field will immediately fail. The related check is commented.
> 
> It is implicitly enforced in DBDF parsing, as the domain would be
> parsed to 0 without error, but the check `end[0] != ':'` afterward
> will return -EINVAL.
> 
> Enforcing consistency between parsers by reading the code is not helped
> by this property being implicit. Add a comment to explain.
> 
> Signed-off-by: Gaetan Rivet <grive@u256.net>
> ---

Acked-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [dpdk-dev] [PATCH v1 1/2] pci: fix allowing underflow when parsing PCI id
  2020-05-13 10:47 ` [dpdk-dev] [PATCH v1 1/2] pci: fix allowing underflow when parsing PCI id Gaetan Rivet
  2020-05-14  8:52   ` Stojaczyk, Dariusz
@ 2020-05-19  9:17   ` David Marchand
  1 sibling, 0 replies; 7+ messages in thread
From: David Marchand @ 2020-05-19  9:17 UTC (permalink / raw)
  To: Gaetan Rivet; +Cc: dev, dpdk stable, Luca Boccassi, Kevin Traynor

On Wed, May 13, 2020 at 12:48 PM Gaetan Rivet <grive@u256.net> wrote:
>
> The function strtoul will not return ERANGE if the input is negative, as
> one might expect.
>
>    0000:-FFFFFFFFFFFFFFFB:00.0
>
> is not a better way to write 0000:05:00.0.
> To simplify checking for '-', forbid using spaces before the field value.
>
>    0000: 00:   2c.0
>
> Should not be accepted.
>
> Fixes: af75078fece3 ("first public release")
> Cc: stable@dpdk.org

Not sure about backporting this one, will let stable maintainers
reconsider this.


> Signed-off-by: Gaetan Rivet <grive@u256.net>
Acked-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>

> ---
>  lib/librte_pci/rte_pci.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/lib/librte_pci/rte_pci.c b/lib/librte_pci/rte_pci.c
> index d1ab6b414..e4ecdc32f 100644
> --- a/lib/librte_pci/rte_pci.c
> +++ b/lib/librte_pci/rte_pci.c
> @@ -35,6 +35,12 @@ get_u8_pciaddr_field(const char *in, void *_u8, char dlm)
>         if (*in == '\0')
>                 return NULL;
>
> +       /* PCI field starting with spaces is forbidden.
> +        * Negative wrap-around is not reported as an error by strtoul.
> +        */
> +       if (*in == ' ' || *in == '-')
> +               return NULL;
> +
>         errno = 0;
>         val = strtoul(in, &end, 16);
>         if (errno != 0 || end[0] != dlm || val > UINT8_MAX) {
> @@ -70,6 +76,12 @@ pci_dbdf_parse(const char *input, struct rte_pci_addr *dev_addr)
>         unsigned long val;
>         char *end;
>
> +       /* PCI id starting with spaces is forbidden.
> +        * Negative wrap-around is not reported as an error by strtoul.
> +        */
> +       if (*in == ' ' || *in == '-')
> +               return EINVAL;

Should be -EINVAL, fixed.

> +
>         errno = 0;
>         val = strtoul(in, &end, 16);
>         if (errno != 0 || end[0] != ':' || val > UINT16_MAX)
> --
> 2.26.2
>

Applied, thanks.


-- 
David Marchand


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [dpdk-dev] [PATCH v1 2/2] pci: explain how empty strings are rejected in DBDF
  2020-05-13 10:47 ` [dpdk-dev] [PATCH v1 2/2] pci: explain how empty strings are rejected in DBDF Gaetan Rivet
  2020-05-14  8:53   ` Stojaczyk, Dariusz
@ 2020-05-19  9:18   ` David Marchand
  1 sibling, 0 replies; 7+ messages in thread
From: David Marchand @ 2020-05-19  9:18 UTC (permalink / raw)
  To: Gaetan Rivet; +Cc: dev

On Wed, May 13, 2020 at 12:48 PM Gaetan Rivet <grive@u256.net> wrote:
>
> Empty strings are forbidden as input to rte_pci_addr_parse().
> It is explicitly enforced in BDF parsing as parsing the bus
> field will immediately fail. The related check is commented.
>
> It is implicitly enforced in DBDF parsing, as the domain would be
> parsed to 0 without error, but the check `end[0] != ':'` afterward
> will return -EINVAL.
>
> Enforcing consistency between parsers by reading the code is not helped
> by this property being implicit. Add a comment to explain.
>
> Signed-off-by: Gaetan Rivet <grive@u256.net>
Acked-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>


Applied, thanks.

-- 
David Marchand


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2020-05-19  9:18 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-13 10:47 [dpdk-dev] [PATCH v1 0/2] pci: a comment and a minor fix Gaetan Rivet
2020-05-13 10:47 ` [dpdk-dev] [PATCH v1 1/2] pci: fix allowing underflow when parsing PCI id Gaetan Rivet
2020-05-14  8:52   ` Stojaczyk, Dariusz
2020-05-19  9:17   ` David Marchand
2020-05-13 10:47 ` [dpdk-dev] [PATCH v1 2/2] pci: explain how empty strings are rejected in DBDF Gaetan Rivet
2020-05-14  8:53   ` Stojaczyk, Dariusz
2020-05-19  9:18   ` David Marchand

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).