DPDK patches and discussions
 help / color / mirror / Atom feed
From: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
To: dev@dpdk.org
Cc: Darek Stojaczyk <dariusz.stojaczyk@intel.com>,
	gaetan.rivet@6wind.com, thomas@monjalon.net
Subject: [dpdk-dev] [PATCH v3] dev: don't remove devargs that are still referenced
Date: Fri, 23 Nov 2018 16:43:28 +0100	[thread overview]
Message-ID: <20181123154328.97021-1-dariusz.stojaczyk@intel.com> (raw)
In-Reply-To: <20181121193827.62540-1-dariusz.stojaczyk@intel.com>

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")
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;
-- 
2.17.1

  parent reply	other threads:[~2018-11-23 15:47 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-11-21 18:37 [dpdk-dev] [PATCH] " Darek Stojaczyk
2018-11-21 18:55 ` Thomas Monjalon
2018-11-21 19:38 ` [dpdk-dev] [PATCH v2] " Darek Stojaczyk
2018-11-22  9:54   ` Gaëtan Rivet
2018-11-22 12:54     ` Stojaczyk, Dariusz
2018-11-23 15:43   ` Darek Stojaczyk [this message]
2018-11-23 17:04     ` [dpdk-dev] [PATCH v3] " Maxime Coquelin
2018-11-23 21:45       ` Stojaczyk, Dariusz
2018-11-25 12:46       ` Thomas Monjalon

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20181123154328.97021-1-dariusz.stojaczyk@intel.com \
    --to=dariusz.stojaczyk@intel.com \
    --cc=dev@dpdk.org \
    --cc=gaetan.rivet@6wind.com \
    --cc=thomas@monjalon.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).