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

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

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

Fixes: 911462eb4a5f ("eal: simplify parameters of hotplug functions")

Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
---
 lib/librte_eal/common/eal_common_dev.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_dev.c b/lib/librte_eal/common/eal_common_dev.c
index 8b0844af1..d4b1ea70d 100644
--- a/lib/librte_eal/common/eal_common_dev.c
+++ b/lib/librte_eal/common/eal_common_dev.c
@@ -185,6 +185,7 @@ local_dev_probe(const char *devargs, struct rte_device **new_dev)
 {
 	struct rte_device *dev;
 	struct rte_devargs *da;
+	struct rte_devargs da_cp;
 	int ret;
 
 	*new_dev = NULL;
@@ -203,18 +204,23 @@ local_dev_probe(const char *devargs, struct rte_device **new_dev)
 		goto err_devarg;
 	}
 
+	/**
+	 * its better not to use da after rte_devargs_insert,
+	 * so make a copy here.
+	 */
+	da_cp = *da;
 	ret = rte_devargs_insert(da);
 	if (ret)
 		goto err_devarg;
 
-	ret = da->bus->scan();
+	ret = da_cp.bus->scan();
 	if (ret)
 		goto err_devarg;
 
-	dev = da->bus->find_device(NULL, cmp_dev_name, da->name);
+	dev = da_cp.bus->find_device(NULL, cmp_dev_name, da_cp.name);
 	if (dev == NULL) {
 		RTE_LOG(ERR, EAL, "Cannot find device (%s)\n",
-			da->name);
+			da_cp.name);
 		ret = -ENODEV;
 		goto err_devarg;
 	}
-- 
2.13.6

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

[Thomas Monjalon]

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.

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!

See http://git.dpdk.org/dpdk/commit/?id=55744d83d525

Gaetan, what can be the fix?

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

[Gaëtan Rivet]

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

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

[Thomas Monjalon]

24/10/2018 00:39, Gaëtan Rivet:
> 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).

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

Nice rant :)

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

[Gaëtan Rivet]

On Wed, Oct 24, 2018 at 04:43:45PM +0200, Thomas Monjalon wrote:
> 24/10/2018 00:39, Gaëtan Rivet:
> > 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).
> 
> If the devargs pointer passed in parameter is the same as the one
> in the list, it will be freed.
> 

This would only happen if one did:

    rte_devargs_insert(dev->devargs);

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

Sure, but in this case the fix is to check whether the device is already
probed, and if so remove it before probing it again with the new
devargs.

> 
> Nice rant :)

:)

-- 
Gaëtan Rivet
6WIND

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

[Zhang, Qi Z]

> -----Original Message-----
> From: Gaëtan Rivet [mailto:gaetan.rivet@6wind.com]
> Sent: Wednesday, October 24, 2018 10:34 AM
> To: Thomas Monjalon <thomas@monjalon.net>
> Cc: dev@dpdk.org; Zhang, Qi Z <qi.z.zhang@intel.com>
> Subject: Re: [dpdk-dev] [PATCH] eal: fix floating device argument pointer
> 
> On Wed, Oct 24, 2018 at 04:43:45PM +0200, Thomas Monjalon wrote:
> > 24/10/2018 00:39, Gaëtan Rivet:
> > > 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.

This does happens when try to attach a vdev on secondary, and I think this is the real place need to fix.
I will drop this patch and submit a new fix to prevent unnecessary rte_devargs_insert during the vdev bus scan.

Thanks
Qi

> > >
> > > > 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).
> >
> > If the devargs pointer passed in parameter is the same as the one in
> > the list, it will be freed.
> >
> 
> This would only happen if one did:
> 
>     rte_devargs_insert(dev->devargs);
> 
> > >    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.
> >
> 
> Sure, but in this case the fix is to check whether the device is already probed,
> and if so remove it before probing it again with the new devargs.
> 
> >
> > Nice rant :)
> 
> :)
> 
> --
> Gaëtan Rivet
> 6WIND

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

[Gaëtan Rivet]

On Thu, Oct 25, 2018 at 03:22:11AM +0000, Zhang, Qi Z wrote:
> 
> 
> > -----Original Message-----
> > From: Gaëtan Rivet [mailto:gaetan.rivet@6wind.com]
> > Sent: Wednesday, October 24, 2018 10:34 AM
> > To: Thomas Monjalon <thomas@monjalon.net>
> > Cc: dev@dpdk.org; Zhang, Qi Z <qi.z.zhang@intel.com>
> > Subject: Re: [dpdk-dev] [PATCH] eal: fix floating device argument pointer
> > 
> > On Wed, Oct 24, 2018 at 04:43:45PM +0200, Thomas Monjalon wrote:
> > > 24/10/2018 00:39, Gaëtan Rivet:
> > > > 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.
> 
> This does happens when try to attach a vdev on secondary, and I think this is the real place need to fix.
> I will drop this patch and submit a new fix to prevent unnecessary rte_devargs_insert during the vdev bus scan.
> 

The vdev_init function should call dev_probe instead of reimplementing it.
But looking at the big picture, maybe the real bug is secondary process.

> Thanks
> Qi
> 
> > > >
> > > > > 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).
> > >
> > > If the devargs pointer passed in parameter is the same as the one in
> > > the list, it will be freed.
> > >
> > 
> > This would only happen if one did:
> > 
> >     rte_devargs_insert(dev->devargs);
> > 
> > > >    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.
> > >
> > 
> > Sure, but in this case the fix is to check whether the device is already probed,
> > and if so remove it before probing it again with the new devargs.
> > 
> > >
> > > Nice rant :)
> > 
> > :)
> > 
> > --
> > Gaëtan Rivet
> > 6WIND

-- 
Gaëtan Rivet
6WIND