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 C7BC6A09F0; Thu, 17 Dec 2020 11:53:18 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 2E607CA08; Thu, 17 Dec 2020 11:53:17 +0100 (CET) Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by dpdk.org (Postfix) with ESMTP id ED792C996; Thu, 17 Dec 2020 11:53:14 +0100 (CET) IronPort-SDR: hHWCW0zBBasJ7SlOL1dlZy/w4vQ49JLCq/TwiUJ8STHiVLaxS+/TyeyxRAgBBBO+z3Rp+zCPrr VKuf105gJ24Q== X-IronPort-AV: E=McAfee;i="6000,8403,9837"; a="172660851" X-IronPort-AV: E=Sophos;i="5.78,426,1599548400"; d="scan'208";a="172660851" Received: from orsmga008.jf.intel.com ([10.7.209.65]) by fmsmga104.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 17 Dec 2020 02:53:13 -0800 IronPort-SDR: 717agdLDRyMDVHjURJUxyShQa8IyvAmCabDzewfHgoZO1klNTGaH81znKRIfCkmusm7obSBgcC XMQriokOMJfg== X-IronPort-AV: E=Sophos;i="5.78,426,1599548400"; d="scan'208";a="369784422" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.213.233.164]) ([10.213.233.164]) by orsmga008-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 17 Dec 2020 02:53:11 -0800 To: "Min Hu (Connor)" , =?UTF-8?Q?Ga=c3=abtan_Rivet?= Cc: chas3@att.com, declan.doherty@intel.com, dev@dpdk.org, gowrishankar.m@linux.vnet.ibm.com, stable@dpdk.org References: <1606357496-8650-1-git-send-email-humin29@huawei.com> <20201207140711.3mjklkmkjrfwts5g@u256.net> <447435c6-e04f-ab63-695b-8d46827a141b@huawei.com> From: Ferruh Yigit Message-ID: <427ad29b-a9cc-3a62-80da-0606b218dbe4@intel.com> Date: Thu, 17 Dec 2020 10:53:07 +0000 MIME-Version: 1.0 In-Reply-To: <447435c6-e04f-ab63-695b-8d46827a141b@huawei.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [dpdk-stable] [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" 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) > 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, >>