From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thomas Monjalon Subject: Re: [PATCH] eal: fix floating device argument pointer Date: Wed, 24 Oct 2018 16:43:45 +0200 Message-ID: <6647495.inI2yHHxz0@xps> References: <20181022054932.39052-1-qi.z.zhang@intel.com> <1576298.HKmtsfqzoT@xps> <20181023223931.kmro2zfyp4c4wbqm@bidouze.vm.6wind.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Cc: dev@dpdk.org, Qi Zhang To: =?ISO-8859-1?Q?Ga=EBtan?= Rivet Return-path: Received: from out3-smtp.messagingengine.com (out3-smtp.messagingengine.com [66.111.4.27]) by dpdk.org (Postfix) with ESMTP id 8029C5592 for ; Wed, 24 Oct 2018 16:43:43 +0200 (CEST) In-Reply-To: <20181023223931.kmro2zfyp4c4wbqm@bidouze.vm.6wind.com> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 24/10/2018 00:39, Ga=EBtan Rivet: > Hi, >=20 > On Mon, Oct 22, 2018 at 09:25:22AM +0200, Thomas Monjalon wrote: > > 22/10/2018 07:49, Qi Zhang: > > > After we insert a devargs into devargs_list, following bus->scan may > > > destroy it due to another rte_devargs_insert. Its better not to use > > > a devargs pointer after it has been inserted. > >=20 >=20 > A bus scan calls rte_devargs_insert? Mapping devargs to device is the > responsibility of the bus scan, if it calls potentially destructive > functions, it must rebuild the map. >=20 > > I think the problem is in: > >=20 > > rte_devargs_insert(struct rte_devargs *da) > > { > > int ret; > >=20 > > ret =3D rte_devargs_remove(da); = =20 > > if (ret < 0) > > return ret; > > TAILQ_INSERT_TAIL(&devargs_list, da, next); > > return 0; > > } > >=20 > > We insert a structure which is freed! >=20 > Not usually, I hope! >=20 > >=20 > > See http://git.dpdk.org/dpdk/commit/?id=3D55744d83d525 > >=20 > > Gaetan, what can be the fix? >=20 > 1. rte_devargs_insert is misdefined. > It is designed as a function that can never fail. > The function should return void instead. >=20 > 2. rte_devargs_remove(da), will not remove da itself. > It will remove whichever rte_devargs matches da within the internal > list. If da does not match any in the list, it does nothing. > As da is a newly-callocated structure, it is actually safe to > continue using it after having called rte_devargs_remove(), because > it cannot possibly have been inserted in the meantime (so would not > have been freed, even if another devargs matched it). If the devargs pointer passed in parameter is the same as the one in the list, it will be freed. > The actual issue is that the matching rte_devargs within the list > would be referenced by a device after a successful scan, meaning that > this reference is not safe if someone attemps to insert the same > device after the bus->scan(). If my understanding is correct, the above > fix is not necessary, but probing should be guarded against > re-entrancy. We may want to probe again with different parameters. > 3. To fix this bug, one should check that the device one attempts to > hotplug does not already exists as a probed rte_device. > An existing rte_devargs is not sufficient, because a blacklisted > device would have an rte_devargs without having a probed rte_device, > and the current behavior is to supersede the current blacklist and > forcibly insert the new device, as if it was newly whitelisted. > This check can only happen at rte_dev level. >=20 > 4. Your confusion about rte_devargs_remove is understandable, the API is > muddy. The reason for these quirks is because I wanted a user > to be able to remove any devargs, even without having a direct > reference to it: you only had to define the bus and the device id > (name), and it would find it and remove it. It might be preferrable > to force the user to find the rte_device, and from it, use the actual > rte_devargs reference to remove it, but then, it would be impossible > to remove devargs for non-existing devices (spoiler: that's the > blacklisted ones). >=20 > 5. It bears repeating: blacklisted mode is horrible and should be removed. > It is all-around abominable, forces unsightly designs to exist and be > used, makes managers ask questions about "why do you add this quirky > `-w 00:00.0` parameter to your command line and what is your timeline > for not needing it?", makes at least one team integrating OVS ask > themselves "why not --no-pci? but then why can't I hotplug PCI ports?", > and I would not be surprised if it killed puppies as a hobby. >=20 > So far, I was able to collect "but it simplifies testing bot > configuration" as a plus, which I do not agree with. >=20 > And anyone trying to package DPDK on their platform, expecting users > not to know or care about it, would be better off developping a > proper autoconf tool, instead of baking it in the entrails of the > EAL, which are ugly enough as it is. /rant Nice rant :)