[dpdk-dev] [PATCH v1 0/2] bonding: fix port id check and PCI addr cmp

[dpdk-dev] [PATCH v1 0/2] bonding: fix port id check and PCI addr cmp

[Gaetan Rivet]

Found these two bugs while reading the bonding code.

The first bug is mostly usability: if a user gives an incorrect port id
as a slave, it will error-out later. The error will simply be less
clear.

I'm partially responsible for the second one. I don't see why we did not
wrote it properly at the time: the comment about lacking the proper
rte_bus_pci impl was already not correct anymore when the check on the kdrv
was removed. The necessary functions were already available I think.

Gaetan Rivet (2):
  bonding: fix port id validity check on parsing
  bonding: fix PCI address comparison on non-pci ports

 drivers/net/bonding/rte_eth_bond_args.c | 63 +++++++++++--------------
 1 file changed, 27 insertions(+), 36 deletions(-)

-- 
2.26.0

[dpdk-dev] [PATCH v1 1/2] bonding: fix port id validity check on parsing

[Gaetan Rivet]

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

Re: [dpdk-dev] [PATCH v1, 1/2] bonding: fix port id validity check on parsing

[Min Hu (Connor)]

It looks good to me, thanks.

Re: [dpdk-dev] [PATCH v1, 1/2] bonding: fix port id validity check on parsing

[Ferruh Yigit]

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.

[dpdk-dev] [PATCH v1 2/2] bonding: fix PCI address comparison on non-pci ports

[Gaetan Rivet]

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

Re: [dpdk-dev] [v1, 2/2] bonding: fix PCI address comparison on non-pci ports

[Min Hu (Connor)]

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

Re: [dpdk-dev] [v1, 2/2] bonding: fix PCI address comparison on non-pci ports

[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,
-- 
Gaëtan

Re: [dpdk-dev] [v1, 2/2] bonding: fix PCI address comparison on non-pci ports

[Min Hu (Connor)]

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,
> 

Re: [dpdk-dev] [dpdk-stable] [v1, 2/2] bonding: fix PCI address comparison on non-pci ports

[Ferruh Yigit]

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,
>>