From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 45A91A09EF; Wed, 16 Dec 2020 13:15:05 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 82AD1C9B4; Wed, 16 Dec 2020 13:15:02 +0100 (CET) Received: from szxga04-in.huawei.com (szxga04-in.huawei.com [45.249.212.190]) by dpdk.org (Postfix) with ESMTP id 4CFFAC996; Wed, 16 Dec 2020 13:15:00 +0100 (CET) Received: from DGGEMS413-HUB.china.huawei.com (unknown [172.30.72.59]) by szxga04-in.huawei.com (SkyGuard) with ESMTP id 4CwvDv5ZDlzkq0Q; Wed, 16 Dec 2020 20:14:07 +0800 (CST) Received: from [10.67.103.128] (10.67.103.128) by DGGEMS413-HUB.china.huawei.com (10.3.19.213) with Microsoft SMTP Server id 14.3.498.0; Wed, 16 Dec 2020 20:14:53 +0800 To: =?UTF-8?Q?Ga=c3=abtan_Rivet?= CC: , , , , References: <1606357496-8650-1-git-send-email-humin29@huawei.com> <20201207140711.3mjklkmkjrfwts5g@u256.net> From: "Min Hu (Connor)" Message-ID: <447435c6-e04f-ab63-695b-8d46827a141b@huawei.com> Date: Wed, 16 Dec 2020 20:14:53 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.3.1 MIME-Version: 1.0 In-Reply-To: <20201207140711.3mjklkmkjrfwts5g@u256.net> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.67.103.128] X-CFilter-Loop: Reflected Subject: Re: [dpdk-dev] [v1, 2/2] bonding: fix PCI address comparison on non-pci ports X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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) 在 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, >