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) <humi...@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,
