* Re: [dpdk-stable] [dpdk-dev] [PATCH v3] dev: don't remove devargs that are still referenced [not found] ` <FBE7E039FA50BF47A673AD0BD3CD56A846234DA3@HASMSX105.ger.corp.intel.com> @ 2018-11-27 11:40 ` Kevin Traynor 2018-11-27 13:03 ` Stojaczyk, Dariusz 0 siblings, 1 reply; 3+ messages in thread From: Kevin Traynor @ 2018-11-27 11:40 UTC (permalink / raw) To: Stojaczyk, Dariusz, Maxime Coquelin; +Cc: gaetan.rivet, thomas, stable On 11/23/2018 09:45 PM, Stojaczyk, Dariusz wrote: > > >> -----Original Message----- >> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com] >> Sent: Friday, November 23, 2018 6:05 PM >> To: Stojaczyk, Dariusz <dariusz.stojaczyk@intel.com>; dev@dpdk.org >> Cc: gaetan.rivet@6wind.com; thomas@monjalon.net >> Subject: Re: [dpdk-dev] [PATCH v3] dev: don't remove devargs that are still >> referenced >> >> Hi, >> >> On 11/23/18 4:43 PM, Darek Stojaczyk wrote: >>> Even if a device failed to plug, it's still a device >>> object that references the devargs. Those devargs will >>> be freed automatically together with the device, but >>> freeing them any earlier - like it's done in the hotplug >>> error handling path right now - will give us a dangling >>> pointer and a segfault scenario. >>> >>> Consider the following case: >>> * secondary process receives the hotplug request IPC message >>> * devargs are either created or updated >>> * the bus is scanned >>> * a new device object is created with the latest devargs >>> * the device can't be plugged for whatever reason, >>> bus->plug returns error >>> * the devargs are freed, even though they're still referenced >>> by the device object on the bus >>> >>> For PCI devices, the generic device name comes from >>> a buffer within the devargs. Freeing those will make >>> EAL segfault whenever the device name is checked. >>> >>> This patch just prevents the hotplug error handling >>> path from removing the devargs when there's a device >>> that references them. This is done by simply exiting >>> early from the hotplug function. As mentioned in the >>> beginning, those devargs will be freed later, together >>> with the device itself. >>> >>> Fixes: 7e8b26650146 ("eal: fix hotplug add / remove") >> >> Should you also cc stable? >> Above commit is in since v17.08. >> > > Hi Maxime, > > Stable could use a similar patch, but not exactly this one as it is now. I'll resubmit for stable once the one here gets approved. > Hi Darek, feel free to send patch to stable@dpdk.org with [18.08] subject prefix, now that this is applied on master, thanks. > Thank you, > D. > >>> Cc: gaetan.rivet@6wind.com >>> Cc: thomas@monjalon.net >>> >>> Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com> >>> --- >>> Changes since v2: >>> * added an extra comment (Gaetan) >>> >>> Changes since v1: >>> * described the failing scenario in commit msg (Thomas) >>> >>> lib/librte_eal/common/eal_common_dev.c | 13 ++++++++----- >>> 1 file changed, 8 insertions(+), 5 deletions(-) >>> >>> diff --git a/lib/librte_eal/common/eal_common_dev.c >> b/lib/librte_eal/common/eal_common_dev.c >>> index 1fdc9ab17..d7950bc9a 100644 >>> --- a/lib/librte_eal/common/eal_common_dev.c >>> +++ b/lib/librte_eal/common/eal_common_dev.c >>> @@ -166,14 +166,17 @@ local_dev_probe(const char *devargs, struct >> rte_device **new_dev) >>> ret = -ENODEV; >>> goto err_devarg; >>> } >>> + /* Since there is a matching device, it is now its responsibility >>> + * to manage the devargs we've just inserted. From this point >>> + * those devargs shouldn't be removed manually anymore. >>> + */ >>> >>> ret = dev->bus->plug(dev); >>> if (ret) { >>> - if (rte_dev_is_probed(dev)) /* if already succeeded earlier >> */ >>> - return ret; /* no rollback */ >>> - RTE_LOG(ERR, EAL, "Driver cannot attach the device (%s)\n", >>> - dev->name); >>> - goto err_devarg; >>> + if (!rte_dev_is_probed(dev)) /* if hasn't succeeded earlier */ >>> + RTE_LOG(ERR, EAL, "Driver cannot attach the device >> (%s)\n", >>> + dev->name); >>> + return ret; >>> } >>> >>> *new_dev = dev; >>> >> >> Other than that, it looks good to me: >> Acked-by: Maxime Coquelin <maxime.coquelin@redhat.com> >> >> Regards, >> Maxime ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [dpdk-stable] [dpdk-dev] [PATCH v3] dev: don't remove devargs that are still referenced 2018-11-27 11:40 ` [dpdk-stable] [dpdk-dev] [PATCH v3] dev: don't remove devargs that are still referenced Kevin Traynor @ 2018-11-27 13:03 ` Stojaczyk, Dariusz 2018-11-27 13:25 ` Kevin Traynor 0 siblings, 1 reply; 3+ messages in thread From: Stojaczyk, Dariusz @ 2018-11-27 13:03 UTC (permalink / raw) To: Kevin Traynor; +Cc: gaetan.rivet, thomas, stable, Maxime Coquelin > -----Original Message----- > From: Kevin Traynor [mailto:ktraynor@redhat.com] > Sent: Tuesday, November 27, 2018 12:40 PM > To: Stojaczyk, Dariusz <dariusz.stojaczyk@intel.com>; Maxime Coquelin > <maxime.coquelin@redhat.com> > Cc: gaetan.rivet@6wind.com; thomas@monjalon.net; stable@dpdk.org > Subject: Re: [dpdk-dev] [PATCH v3] dev: don't remove devargs that are still > referenced > > On 11/23/2018 09:45 PM, Stojaczyk, Dariusz wrote: > > > > > >> -----Original Message----- > >> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com] > >> Sent: Friday, November 23, 2018 6:05 PM > >> To: Stojaczyk, Dariusz <dariusz.stojaczyk@intel.com>; dev@dpdk.org > >> Cc: gaetan.rivet@6wind.com; thomas@monjalon.net > >> Subject: Re: [dpdk-dev] [PATCH v3] dev: don't remove devargs that are > still > >> referenced > >> > >> Hi, > >> > >> On 11/23/18 4:43 PM, Darek Stojaczyk wrote: > >>> Even if a device failed to plug, it's still a device > >>> object that references the devargs. Those devargs will > >>> be freed automatically together with the device, but > >>> freeing them any earlier - like it's done in the hotplug > >>> error handling path right now - will give us a dangling > >>> pointer and a segfault scenario. > >>> > >>> Consider the following case: > >>> * secondary process receives the hotplug request IPC message > >>> * devargs are either created or updated > >>> * the bus is scanned > >>> * a new device object is created with the latest devargs > >>> * the device can't be plugged for whatever reason, > >>> bus->plug returns error > >>> * the devargs are freed, even though they're still referenced > >>> by the device object on the bus > >>> > >>> For PCI devices, the generic device name comes from > >>> a buffer within the devargs. Freeing those will make > >>> EAL segfault whenever the device name is checked. > >>> > >>> This patch just prevents the hotplug error handling > >>> path from removing the devargs when there's a device > >>> that references them. This is done by simply exiting > >>> early from the hotplug function. As mentioned in the > >>> beginning, those devargs will be freed later, together > >>> with the device itself. > >>> > >>> Fixes: 7e8b26650146 ("eal: fix hotplug add / remove") > >> > >> Should you also cc stable? > >> Above commit is in since v17.08. > >> > > > > Hi Maxime, > > > > Stable could use a similar patch, but not exactly this one as it is now. I'll > resubmit for stable once the one here gets approved. > > > > Hi Darek, feel free to send patch to stable@dpdk.org with [18.08] > subject prefix, now that this is applied on master, thanks. Kevin, I just tried rebasing this patch on 18.08, but the relevant code in 18.08 is just too much of a mess and I prefer not to touch it. Sorry, D. > > > > Thank you, > > D. > > > >>> Cc: gaetan.rivet@6wind.com > >>> Cc: thomas@monjalon.net > >>> > >>> Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com> > >>> --- > >>> Changes since v2: > >>> * added an extra comment (Gaetan) > >>> > >>> Changes since v1: > >>> * described the failing scenario in commit msg (Thomas) > >>> > >>> lib/librte_eal/common/eal_common_dev.c | 13 ++++++++----- > >>> 1 file changed, 8 insertions(+), 5 deletions(-) > >>> > >>> diff --git a/lib/librte_eal/common/eal_common_dev.c > >> b/lib/librte_eal/common/eal_common_dev.c > >>> index 1fdc9ab17..d7950bc9a 100644 > >>> --- a/lib/librte_eal/common/eal_common_dev.c > >>> +++ b/lib/librte_eal/common/eal_common_dev.c > >>> @@ -166,14 +166,17 @@ local_dev_probe(const char *devargs, struct > >> rte_device **new_dev) > >>> ret = -ENODEV; > >>> goto err_devarg; > >>> } > >>> + /* Since there is a matching device, it is now its responsibility > >>> + * to manage the devargs we've just inserted. From this point > >>> + * those devargs shouldn't be removed manually anymore. > >>> + */ > >>> > >>> ret = dev->bus->plug(dev); > >>> if (ret) { > >>> - if (rte_dev_is_probed(dev)) /* if already succeeded earlier > >> */ > >>> - return ret; /* no rollback */ > >>> - RTE_LOG(ERR, EAL, "Driver cannot attach the device (%s)\n", > >>> - dev->name); > >>> - goto err_devarg; > >>> + if (!rte_dev_is_probed(dev)) /* if hasn't succeeded earlier */ > >>> + RTE_LOG(ERR, EAL, "Driver cannot attach the device > >> (%s)\n", > >>> + dev->name); > >>> + return ret; > >>> } > >>> > >>> *new_dev = dev; > >>> > >> > >> Other than that, it looks good to me: > >> Acked-by: Maxime Coquelin <maxime.coquelin@redhat.com> > >> > >> Regards, > >> Maxime ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [dpdk-stable] [dpdk-dev] [PATCH v3] dev: don't remove devargs that are still referenced 2018-11-27 13:03 ` Stojaczyk, Dariusz @ 2018-11-27 13:25 ` Kevin Traynor 0 siblings, 0 replies; 3+ messages in thread From: Kevin Traynor @ 2018-11-27 13:25 UTC (permalink / raw) To: Stojaczyk, Dariusz; +Cc: gaetan.rivet, thomas, stable, Maxime Coquelin On 11/27/2018 01:03 PM, Stojaczyk, Dariusz wrote: > > >> -----Original Message----- >> From: Kevin Traynor [mailto:ktraynor@redhat.com] >> Sent: Tuesday, November 27, 2018 12:40 PM >> To: Stojaczyk, Dariusz <dariusz.stojaczyk@intel.com>; Maxime Coquelin >> <maxime.coquelin@redhat.com> >> Cc: gaetan.rivet@6wind.com; thomas@monjalon.net; stable@dpdk.org >> Subject: Re: [dpdk-dev] [PATCH v3] dev: don't remove devargs that are still >> referenced >> >> On 11/23/2018 09:45 PM, Stojaczyk, Dariusz wrote: >>> >>> >>>> -----Original Message----- >>>> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com] >>>> Sent: Friday, November 23, 2018 6:05 PM >>>> To: Stojaczyk, Dariusz <dariusz.stojaczyk@intel.com>; dev@dpdk.org >>>> Cc: gaetan.rivet@6wind.com; thomas@monjalon.net >>>> Subject: Re: [dpdk-dev] [PATCH v3] dev: don't remove devargs that are >> still >>>> referenced >>>> >>>> Hi, >>>> >>>> On 11/23/18 4:43 PM, Darek Stojaczyk wrote: >>>>> Even if a device failed to plug, it's still a device >>>>> object that references the devargs. Those devargs will >>>>> be freed automatically together with the device, but >>>>> freeing them any earlier - like it's done in the hotplug >>>>> error handling path right now - will give us a dangling >>>>> pointer and a segfault scenario. >>>>> >>>>> Consider the following case: >>>>> * secondary process receives the hotplug request IPC message >>>>> * devargs are either created or updated >>>>> * the bus is scanned >>>>> * a new device object is created with the latest devargs >>>>> * the device can't be plugged for whatever reason, >>>>> bus->plug returns error >>>>> * the devargs are freed, even though they're still referenced >>>>> by the device object on the bus >>>>> >>>>> For PCI devices, the generic device name comes from >>>>> a buffer within the devargs. Freeing those will make >>>>> EAL segfault whenever the device name is checked. >>>>> >>>>> This patch just prevents the hotplug error handling >>>>> path from removing the devargs when there's a device >>>>> that references them. This is done by simply exiting >>>>> early from the hotplug function. As mentioned in the >>>>> beginning, those devargs will be freed later, together >>>>> with the device itself. >>>>> >>>>> Fixes: 7e8b26650146 ("eal: fix hotplug add / remove") >>>> >>>> Should you also cc stable? >>>> Above commit is in since v17.08. >>>> >>> >>> Hi Maxime, >>> >>> Stable could use a similar patch, but not exactly this one as it is now. I'll >> resubmit for stable once the one here gets approved. >>> >> >> Hi Darek, feel free to send patch to stable@dpdk.org with [18.08] >> subject prefix, now that this is applied on master, thanks. > > Kevin, I just tried rebasing this patch on 18.08, but the relevant code in 18.08 is just too much of a mess and I prefer not to touch it. No, problem, thanks for checking it, Kevin. > Sorry, > D. > >> >> >>> Thank you, >>> D. >>> >>>>> Cc: gaetan.rivet@6wind.com >>>>> Cc: thomas@monjalon.net >>>>> >>>>> Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com> >>>>> --- >>>>> Changes since v2: >>>>> * added an extra comment (Gaetan) >>>>> >>>>> Changes since v1: >>>>> * described the failing scenario in commit msg (Thomas) >>>>> >>>>> lib/librte_eal/common/eal_common_dev.c | 13 ++++++++----- >>>>> 1 file changed, 8 insertions(+), 5 deletions(-) >>>>> >>>>> diff --git a/lib/librte_eal/common/eal_common_dev.c >>>> b/lib/librte_eal/common/eal_common_dev.c >>>>> index 1fdc9ab17..d7950bc9a 100644 >>>>> --- a/lib/librte_eal/common/eal_common_dev.c >>>>> +++ b/lib/librte_eal/common/eal_common_dev.c >>>>> @@ -166,14 +166,17 @@ local_dev_probe(const char *devargs, struct >>>> rte_device **new_dev) >>>>> ret = -ENODEV; >>>>> goto err_devarg; >>>>> } >>>>> + /* Since there is a matching device, it is now its responsibility >>>>> + * to manage the devargs we've just inserted. From this point >>>>> + * those devargs shouldn't be removed manually anymore. >>>>> + */ >>>>> >>>>> ret = dev->bus->plug(dev); >>>>> if (ret) { >>>>> - if (rte_dev_is_probed(dev)) /* if already succeeded earlier >>>> */ >>>>> - return ret; /* no rollback */ >>>>> - RTE_LOG(ERR, EAL, "Driver cannot attach the device (%s)\n", >>>>> - dev->name); >>>>> - goto err_devarg; >>>>> + if (!rte_dev_is_probed(dev)) /* if hasn't succeeded earlier */ >>>>> + RTE_LOG(ERR, EAL, "Driver cannot attach the device >>>> (%s)\n", >>>>> + dev->name); >>>>> + return ret; >>>>> } >>>>> >>>>> *new_dev = dev; >>>>> >>>> >>>> Other than that, it looks good to me: >>>> Acked-by: Maxime Coquelin <maxime.coquelin@redhat.com> >>>> >>>> Regards, >>>> Maxime > ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-11-27 13:25 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <20181121193827.62540-1-dariusz.stojaczyk@intel.com> [not found] ` <20181123154328.97021-1-dariusz.stojaczyk@intel.com> [not found] ` <db643517-a8d3-2341-79e4-45e76fb3f090@redhat.com> [not found] ` <FBE7E039FA50BF47A673AD0BD3CD56A846234DA3@HASMSX105.ger.corp.intel.com> 2018-11-27 11:40 ` [dpdk-stable] [dpdk-dev] [PATCH v3] dev: don't remove devargs that are still referenced Kevin Traynor 2018-11-27 13:03 ` Stojaczyk, Dariusz 2018-11-27 13:25 ` Kevin Traynor
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).