patches for DPDK stable branches
 help / color / mirror / Atom feed
* [dpdk-stable] [PATCH v1 1/2] bonding: fix port id validity check on parsing
       [not found] <cover.1587137703.git.grive@u256.net>
@ 2020-04-17 16:42 ` Gaetan Rivet
  2020-11-26  2:45   ` [dpdk-stable] [dpdk-dev][PATCH v1, " Min Hu (Connor)
  2020-04-17 16:42 ` [dpdk-stable] [PATCH v1 2/2] bonding: fix PCI address comparison on non-pci ports Gaetan Rivet
  1 sibling, 1 reply; 8+ messages in thread
From: Gaetan Rivet @ 2020-04-17 16:42 UTC (permalink / raw)
  To: dev; +Cc: stable, Chas Williams

If the port_id is equal to RTE_MAX_ETHPORTS, it should be considered
invalid. Additionally, UNUSED ports are also not valid port ids to be
used afterward.

To simplify following the ethdev API rules, use the exposed function
checking whether a port id is valid.

Fixes: 2efb58cbab6e ("bond: new link bonding library")
Cc: stable@dpdk.org

Cc: Chas Williams <chas3@att.com>
Signed-off-by: Gaetan Rivet <grive@u256.net>
---
 drivers/net/bonding/rte_eth_bond_args.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/net/bonding/rte_eth_bond_args.c b/drivers/net/bonding/rte_eth_bond_args.c
index abdf55261..35616fb8b 100644
--- a/drivers/net/bonding/rte_eth_bond_args.c
+++ b/drivers/net/bonding/rte_eth_bond_args.c
@@ -108,9 +108,8 @@ parse_port_id(const char *port_str)
 		}
 	}
 
-	if (port_id < 0 || port_id > RTE_MAX_ETHPORTS) {
-		RTE_BOND_LOG(ERR, "Slave port specified (%s) outside expected range",
-				port_str);
+	if (!rte_eth_dev_is_valid_port(port_id)) {
+		RTE_BOND_LOG(ERR, "Specified port (%s) is invalid", port_str);
 		return -1;
 	}
 	return port_id;
-- 
2.26.0


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

* [dpdk-stable] [PATCH v1 2/2] bonding: fix PCI address comparison on non-pci ports
       [not found] <cover.1587137703.git.grive@u256.net>
  2020-04-17 16:42 ` [dpdk-stable] [PATCH v1 1/2] bonding: fix port id validity check on parsing Gaetan Rivet
@ 2020-04-17 16:42 ` Gaetan Rivet
  2020-11-26  2:24   ` [dpdk-stable] [v1, " Min Hu (Connor)
  1 sibling, 1 reply; 8+ messages in thread
From: Gaetan Rivet @ 2020-04-17 16:42 UTC (permalink / raw)
  To: dev; +Cc: stable, Chas Williams, Declan Doherty, Gowrishankar Muthukrishnan

The bonding PMD will iterate over all available ETH ports and for each,
compare a chunk of bytes at an offset that would correspond to the PCI
address in an rte_pci_device.

This is incorrect and unsafe. Also, the rte_device using this PCI
address is already found, no need to compare again the PCI address of
all eth devices.

Refactoring the code to fix this, the initial check to find the PCI bus
is out of scope.

Fixes: c848b518bbc7 ("net/bonding: support bifurcated driver in eal")
Cc: stable@dpdk.org

Cc: Chas Williams <chas3@att.com>
Cc: Declan Doherty <declan.doherty@intel.com>
Cc: Gowrishankar Muthukrishnan <gowrishankar.m@linux.vnet.ibm.com>

Signed-off-by: Gaetan Rivet <grive@u256.net>
---
 drivers/net/bonding/rte_eth_bond_args.c | 58 +++++++++++--------------
 1 file changed, 25 insertions(+), 33 deletions(-)

diff --git a/drivers/net/bonding/rte_eth_bond_args.c b/drivers/net/bonding/rte_eth_bond_args.c
index 35616fb8b..8c5f90dc6 100644
--- a/drivers/net/bonding/rte_eth_bond_args.c
+++ b/drivers/net/bonding/rte_eth_bond_args.c
@@ -22,23 +22,37 @@ const char *pmd_bond_init_valid_arguments[] = {
 	NULL
 };
 
+static inline int
+bond_pci_addr_cmp(const struct rte_device *dev, const void *_pci_addr)
+{
+	const struct rte_pci_device *pdev = RTE_DEV_TO_PCI_CONST(dev);
+	const struct rte_pci_addr *paddr = _pci_addr;
+
+	return rte_pci_addr_cmp(&pdev->addr, paddr);
+}
+
 static inline int
 find_port_id_by_pci_addr(const struct rte_pci_addr *pci_addr)
 {
-	struct rte_pci_device *pci_dev;
-	struct rte_pci_addr *eth_pci_addr;
+	struct rte_bus *pci_bus;
+	struct rte_device *dev;
 	unsigned i;
 
-	RTE_ETH_FOREACH_DEV(i) {
-		pci_dev = RTE_ETH_DEV_TO_PCI(&rte_eth_devices[i]);
-		eth_pci_addr = &pci_dev->addr;
+	pci_bus = rte_bus_find_by_name("pci");
+	if (pci_bus == NULL) {
+		RTE_BOND_LOG(ERR, "No PCI bus found");
+		return -1;
+	}
 
-		if (pci_addr->bus == eth_pci_addr->bus &&
-			pci_addr->devid == eth_pci_addr->devid &&
-			pci_addr->domain == eth_pci_addr->domain &&
-			pci_addr->function == eth_pci_addr->function)
-			return i;
+	dev = pci_bus->find_device(NULL, bond_pci_addr_cmp, pci_addr);
+	if (dev == NULL) {
+		RTE_BOND_LOG(ERR, "unable to find PCI device");
+		return -1;
 	}
+
+	RTE_ETH_FOREACH_DEV(i)
+		if (rte_eth_devices[i].device == dev)
+			return i;
 	return -1;
 }
 
@@ -57,15 +71,6 @@ find_port_id_by_dev_name(const char *name)
 	return -1;
 }
 
-static inline int
-bond_pci_addr_cmp(const struct rte_device *dev, const void *_pci_addr)
-{
-	const struct rte_pci_device *pdev = RTE_DEV_TO_PCI_CONST(dev);
-	const struct rte_pci_addr *paddr = _pci_addr;
-
-	return rte_pci_addr_cmp(&pdev->addr, paddr);
-}
-
 /**
  * Parses a port identifier string to a port id by pci address, then by name,
  * and finally port id.
@@ -74,23 +79,10 @@ static inline int
 parse_port_id(const char *port_str)
 {
 	struct rte_pci_addr dev_addr;
-	struct rte_bus *pci_bus;
-	struct rte_device *dev;
 	int port_id;
 
-	pci_bus = rte_bus_find_by_name("pci");
-	if (pci_bus == NULL) {
-		RTE_BOND_LOG(ERR, "unable to find PCI bus\n");
-		return -1;
-	}
-
 	/* try parsing as pci address, physical devices */
-	if (pci_bus->parse(port_str, &dev_addr) == 0) {
-		dev = pci_bus->find_device(NULL, bond_pci_addr_cmp, &dev_addr);
-		if (dev == NULL) {
-			RTE_BOND_LOG(ERR, "unable to find PCI device");
-			return -1;
-		}
+	if (rte_pci_addr_parse(port_str, &dev_addr) == 0) {
 		port_id = find_port_id_by_pci_addr(&dev_addr);
 		if (port_id < 0)
 			return -1;
-- 
2.26.0


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

* Re: [dpdk-stable] [v1, 2/2] bonding: fix PCI address comparison on non-pci ports
  2020-04-17 16:42 ` [dpdk-stable] [PATCH v1 2/2] bonding: fix PCI address comparison on non-pci ports Gaetan Rivet
@ 2020-11-26  2:24   ` Min Hu (Connor)
  2020-12-07 14:07     ` Gaëtan Rivet
  0 siblings, 1 reply; 8+ messages in thread
From: Min Hu (Connor) @ 2020-11-26  2:24 UTC (permalink / raw)
  To: grive; +Cc: chas3, declan.doherty, dev, gowrishankar.m, stable

what scenarios may cause bugs in old ways.
Could you give an example, thanks.

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

* Re: [dpdk-stable] [dpdk-dev][PATCH v1, 1/2] bonding: fix port id validity check on parsing
  2020-04-17 16:42 ` [dpdk-stable] [PATCH v1 1/2] bonding: fix port id validity check on parsing Gaetan Rivet
@ 2020-11-26  2:45   ` Min Hu (Connor)
  2020-12-07 13:34     ` [dpdk-stable] [dpdk-dev] [PATCH " Ferruh Yigit
  0 siblings, 1 reply; 8+ messages in thread
From: Min Hu (Connor) @ 2020-11-26  2:45 UTC (permalink / raw)
  To: grive; +Cc: chas3, dev, stable

It looks good to me, thanks.

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

* Re: [dpdk-stable] [dpdk-dev] [PATCH v1, 1/2] bonding: fix port id validity check on parsing
  2020-11-26  2:45   ` [dpdk-stable] [dpdk-dev][PATCH v1, " Min Hu (Connor)
@ 2020-12-07 13:34     ` Ferruh Yigit
  0 siblings, 0 replies; 8+ messages in thread
From: Ferruh Yigit @ 2020-12-07 13:34 UTC (permalink / raw)
  To: Min Hu (Connor), grive; +Cc: chas3, dev, stable

On 11/26/2020 2:45 AM, Min Hu (Connor) wrote:
> It looks good to me, thanks.
> 

Converting it to an explicit ack:
Acked-by: Min Hu (Connor) <humin29@huawei.com>

Applied to dpdk-next-net/main, thanks.

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

* Re: [dpdk-stable] [v1, 2/2] bonding: fix PCI address comparison on non-pci ports
  2020-11-26  2:24   ` [dpdk-stable] [v1, " Min Hu (Connor)
@ 2020-12-07 14:07     ` Gaëtan Rivet
  2020-12-16 12:14       ` Min Hu (Connor)
  0 siblings, 1 reply; 8+ messages in thread
From: Gaëtan Rivet @ 2020-12-07 14:07 UTC (permalink / raw)
  To: Min Hu (Connor); +Cc: chas3, declan.doherty, dev, gowrishankar.m, stable

On 26/11/20 10:24 +0800, Min Hu (Connor) wrote:
> what scenarios may cause bugs in old ways.
> Could you give an example, thanks.

Hello,

For example in the following code:

-       RTE_ETH_FOREACH_DEV(i) {
-               pci_dev = RTE_ETH_DEV_TO_PCI(&rte_eth_devices[i]);
-               eth_pci_addr = &pci_dev->addr;

All ethdev are iterated, before reading their supposed PCI addresses, and
comparing those to the one passed in arguments.

But not all ethdev will be PCI devices, so the cast is incorrect. It will do
a containerof() on a structure that it supposes contains an rte_device at the
offset 16 (two pointers accounting for the TAILQ_ENTRY()), but nothing prevents any
other bus from implementing their device with any other layout.

So the cast is wrong, but generally it will give out readable memory at least.
Then the field ((eth_dev)->device)->addr will be read and compared against the input,
with arbitrary data here.

A scenario that could cause bug would be when another bus implements a device
in such a way that it will write plausible binary rep of PCI addresses at the
same offset, and match a request from a user. The bonding PMD would take the
device over without properly checking that it is actually a PCI device.


There have been APIs introduced in the EAL to simplify the iteration of
devices, especially restricted to buses. Those APIs should be used instead.
The current implementation is inefficient and wrong. It will work in most cases
but can still trigger weird issues for users, especially in cases that bonding
PMD devs won't generally encounter (with setups where multiple buses are used
with a variety of devices).

Regards,
-- 
Gaëtan

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

* Re: [dpdk-stable] [v1, 2/2] bonding: fix PCI address comparison on non-pci ports
  2020-12-07 14:07     ` Gaëtan Rivet
@ 2020-12-16 12:14       ` Min Hu (Connor)
  2020-12-17 10:53         ` Ferruh Yigit
  0 siblings, 1 reply; 8+ messages in thread
From: Min Hu (Connor) @ 2020-12-16 12:14 UTC (permalink / raw)
  To: Gaëtan Rivet; +Cc: chas3, declan.doherty, dev, gowrishankar.m, stable

Hi,
     sorry for late reply.
     I know what you mean. But "find_port_id_by_pci_addr"
is one static type funtion. it is only used in the function
"parse_port_id". what you modified in "find_port_id_by_pci_addr"
is totally done before "find_port_id_by_pci_addr" in the function
"parse_port_id". That is, it first find pci_bus, then find
rte_pci_device, at last get port id.
So the bug you described will not happen in current version,
unless others directly use the function "find_port_id_by_pci_addr"
without considering that rte_eth_devices[] may contain non-pci devices.

So, I think the patch is OK to me.

Acked-by: Min Hu (Connor) <humin29@huawei.com>


在 2020/12/7 22:07, Gaëtan Rivet 写道:
> On 26/11/20 10:24 +0800, Min Hu (Connor) wrote:
>> what scenarios may cause bugs in old ways.
>> Could you give an example, thanks.
> 
> Hello,
> 
> For example in the following code:
> 
> -       RTE_ETH_FOREACH_DEV(i) {
> -               pci_dev = RTE_ETH_DEV_TO_PCI(&rte_eth_devices[i]);
> -               eth_pci_addr = &pci_dev->addr;
> 
> All ethdev are iterated, before reading their supposed PCI addresses, and
> comparing those to the one passed in arguments.
> 
> But not all ethdev will be PCI devices, so the cast is incorrect. It will do
> a containerof() on a structure that it supposes contains an rte_device at the
> offset 16 (two pointers accounting for the TAILQ_ENTRY()), but nothing prevents any
> other bus from implementing their device with any other layout.
> 
> So the cast is wrong, but generally it will give out readable memory at least.
> Then the field ((eth_dev)->device)->addr will be read and compared against the input,
> with arbitrary data here.
> 
> A scenario that could cause bug would be when another bus implements a device
> in such a way that it will write plausible binary rep of PCI addresses at the
> same offset, and match a request from a user. The bonding PMD would take the
> device over without properly checking that it is actually a PCI device.
> 
> 
> There have been APIs introduced in the EAL to simplify the iteration of
> devices, especially restricted to buses. Those APIs should be used instead.
> The current implementation is inefficient and wrong. It will work in most cases
> but can still trigger weird issues for users, especially in cases that bonding
> PMD devs won't generally encounter (with setups where multiple buses are used
> with a variety of devices).
> 
> Regards,
> 

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

* Re: [dpdk-stable] [v1, 2/2] bonding: fix PCI address comparison on non-pci ports
  2020-12-16 12:14       ` Min Hu (Connor)
@ 2020-12-17 10:53         ` Ferruh Yigit
  0 siblings, 0 replies; 8+ messages in thread
From: Ferruh Yigit @ 2020-12-17 10:53 UTC (permalink / raw)
  To: Min Hu (Connor), Gaëtan Rivet
  Cc: chas3, declan.doherty, dev, gowrishankar.m, stable

On 12/16/2020 12:14 PM, Min Hu (Connor) wrote:
> Hi,
>      sorry for late reply.
>      I know what you mean. But "find_port_id_by_pci_addr"
> is one static type funtion. it is only used in the function
> "parse_port_id". what you modified in "find_port_id_by_pci_addr"
> is totally done before "find_port_id_by_pci_addr" in the function
> "parse_port_id". That is, it first find pci_bus, then find
> rte_pci_device, at last get port id.
> So the bug you described will not happen in current version,
> unless others directly use the function "find_port_id_by_pci_addr"
> without considering that rte_eth_devices[] may contain non-pci devices.
> 
> So, I think the patch is OK to me.
> 
> Acked-by: Min Hu (Connor) <humin29@huawei.com>
> 

Applied to dpdk-next-net/main, thanks.

> 
> 在 2020/12/7 22:07, Gaëtan Rivet 写道:
>> On 26/11/20 10:24 +0800, Min Hu (Connor) wrote:
>>> what scenarios may cause bugs in old ways.
>>> Could you give an example, thanks.
>>
>> Hello,
>>
>> For example in the following code:
>>
>> -       RTE_ETH_FOREACH_DEV(i) {
>> -               pci_dev = RTE_ETH_DEV_TO_PCI(&rte_eth_devices[i]);
>> -               eth_pci_addr = &pci_dev->addr;
>>
>> All ethdev are iterated, before reading their supposed PCI addresses, and
>> comparing those to the one passed in arguments.
>>
>> But not all ethdev will be PCI devices, so the cast is incorrect. It will do
>> a containerof() on a structure that it supposes contains an rte_device at the
>> offset 16 (two pointers accounting for the TAILQ_ENTRY()), but nothing 
>> prevents any
>> other bus from implementing their device with any other layout.
>>
>> So the cast is wrong, but generally it will give out readable memory at least.
>> Then the field ((eth_dev)->device)->addr will be read and compared against the 
>> input,
>> with arbitrary data here.
>>
>> A scenario that could cause bug would be when another bus implements a device
>> in such a way that it will write plausible binary rep of PCI addresses at the
>> same offset, and match a request from a user. The bonding PMD would take the
>> device over without properly checking that it is actually a PCI device.
>>
>>
>> There have been APIs introduced in the EAL to simplify the iteration of
>> devices, especially restricted to buses. Those APIs should be used instead.
>> The current implementation is inefficient and wrong. It will work in most cases
>> but can still trigger weird issues for users, especially in cases that bonding
>> PMD devs won't generally encounter (with setups where multiple buses are used
>> with a variety of devices).
>>
>> Regards,
>>


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

end of thread, other threads:[~2020-12-17 10:53 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <cover.1587137703.git.grive@u256.net>
2020-04-17 16:42 ` [dpdk-stable] [PATCH v1 1/2] bonding: fix port id validity check on parsing Gaetan Rivet
2020-11-26  2:45   ` [dpdk-stable] [dpdk-dev][PATCH v1, " Min Hu (Connor)
2020-12-07 13:34     ` [dpdk-stable] [dpdk-dev] [PATCH " Ferruh Yigit
2020-04-17 16:42 ` [dpdk-stable] [PATCH v1 2/2] bonding: fix PCI address comparison on non-pci ports Gaetan Rivet
2020-11-26  2:24   ` [dpdk-stable] [v1, " Min Hu (Connor)
2020-12-07 14:07     ` Gaëtan Rivet
2020-12-16 12:14       ` Min Hu (Connor)
2020-12-17 10:53         ` Ferruh Yigit

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).