Re: [dpdk-stable] patch 'eal: fix devargs reference after probing failure' has been queued to LTS release 17.11.5

[Yongseok Koh <yskoh@mellanox.com>]

Hi,

This patch is being removed from stable/17.11 as it was mistakenly merged.
Patches having 'fix' keyword in the title were merged even though those don't
have "Cc: stable@dpdk.org" tag in the commit message.

If you think this patch is still needed for stable/17.11, please let me know.
Then I'll take it back.


Thanks,
Yongseok


> On Nov 29, 2018, at 3:12 PM, Yongseok Koh <yskoh@mellanox.com> wrote:
> 
> Hi,
> 
> FYI, your patch has been queued to LTS release 17.11.5
> 
> Note it hasn't been pushed to https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdpdk.org%2Fbrowse%2Fdpdk-stable&amp;data=02%7C01%7Cyskoh%40mellanox.com%7Cdc933a8b74424a5630d408d65650aa3a%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636791301781669112&amp;sdata=LnFDOibDoeFtUuqnCBRLFqXaUWSZMw56A0NVOzq9TWs%3D&amp;reserved=0 yet.
> It will be pushed if I get no objections before 12/01/18. So please
> shout if anyone has objections.
> 
> Also note that after the patch there's a diff of the upstream commit vs the patch applied
> to the branch. If the code is different (ie: not only metadata diffs), due for example to
> a change in context or macro names, please double check it.
> 
> Thanks.
> 
> Yongseok
> 
> ---
> From abdb0f9a8b0fe291ef411fff1a44d0f1d49a40f6 Mon Sep 17 00:00:00 2001
> From: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
> Date: Fri, 23 Nov 2018 16:43:28 +0100
> Subject: [PATCH] eal: fix devargs reference after probing failure
> 
> [ upstream commit 161419983da296f329039e15b88e11ea31b15702 ]
> 
> 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")
> 
> Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
> Acked-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> Acked-by: Thomas Monjalon <thomas@monjalon.net>
> ---
> lib/librte_eal/common/eal_common_dev.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/librte_eal/common/eal_common_dev.c b/lib/librte_eal/common/eal_common_dev.c
> index dda8f5835..582cf3a31 100644
> --- a/lib/librte_eal/common/eal_common_dev.c
> +++ b/lib/librte_eal/common/eal_common_dev.c
> @@ -183,12 +183,16 @@ int rte_eal_hotplug_add(const char *busname, const char *devname,
> 		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 = bus->plug(dev);
> 	if (ret) {
> 		RTE_LOG(ERR, EAL, "Driver cannot attach the device (%s)\n",
> 			dev->name);
> -		goto err_devarg;
> +		return ret;
> 	}
> 	free(name);
> 	return 0;
> -- 
> 2.11.0
> 
> ---
>  Diff of the applied patch vs upstream commit (please double-check if non-empty:
> ---
> --- -	2018-11-29 15:01:50.787743434 -0800
> +++ 0128-eal-fix-devargs-reference-after-probing-failure.patch	2018-11-29 15:01:45.337956000 -0800
> @@ -1,8 +1,10 @@
> -From 161419983da296f329039e15b88e11ea31b15702 Mon Sep 17 00:00:00 2001
> +From abdb0f9a8b0fe291ef411fff1a44d0f1d49a40f6 Mon Sep 17 00:00:00 2001
> From: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
> Date: Fri, 23 Nov 2018 16:43:28 +0100
> Subject: [PATCH] eal: fix devargs reference after probing failure
> 
> +[ upstream commit 161419983da296f329039e15b88e11ea31b15702 ]
> +
> 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
> @@ -41,10 +43,10 @@
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/librte_eal/common/eal_common_dev.c b/lib/librte_eal/common/eal_common_dev.c
> -index a08dc085f..fd7f5ca7d 100644
> +index dda8f5835..582cf3a31 100644
> --- a/lib/librte_eal/common/eal_common_dev.c
> +++ b/lib/librte_eal/common/eal_common_dev.c
> -@@ -166,12 +166,16 @@ local_dev_probe(const char *devargs, struct rte_device **new_dev)
> +@@ -183,12 +183,16 @@ int rte_eal_hotplug_add(const char *busname, const char *devname,
>  		ret = -ENODEV;
>  		goto err_devarg;
>  	}
> @@ -53,15 +55,15 @@
> +	 * those devargs shouldn't be removed manually anymore.
> +	 */
> 
> - 	ret = dev->bus->plug(dev);
> - 	if (ret && !rte_dev_is_probed(dev)) { /* if hasn't ever succeeded */
> + 	ret = bus->plug(dev);
> + 	if (ret) {
>  		RTE_LOG(ERR, EAL, "Driver cannot attach the device (%s)\n",
>  			dev->name);
> -		goto err_devarg;
> +		return ret;
>  	}
> - 
> - 	*new_dev = dev;
> + 	free(name);
> + 	return 0;
> -- 
> 2.11.0
>