Re: [dpdk-dev] [PATCH] eal: fix floating device argument pointer

["Gaëtan Rivet" <gaetan.rivet@6wind.com>]

Hi,

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

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.

> I think the problem is in:
> 
> rte_devargs_insert(struct rte_devargs *da)
> {
>     int ret;
> 
>     ret = rte_devargs_remove(da);                                                                    
>     if (ret < 0)
>         return ret;
>     TAILQ_INSERT_TAIL(&devargs_list, da, next);
>     return 0;
> }
> 
> We insert a structure which is freed!

Not usually, I hope!

> 
> See http://git.dpdk.org/dpdk/commit/?id=55744d83d525
> 
> Gaetan, what can be the fix?

1. rte_devargs_insert is misdefined.
   It is designed as a function that can never fail.
   The function should return void instead.

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).

   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.

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.

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).

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.

   So far, I was able to collect "but it simplifies testing bot
   configuration" as a plus, which I do not agree with.

   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

Regards,
-- 
Gaëtan Rivet
6WIND