patches for DPDK stable branches
 help / color / mirror / Atom feed
* [dpdk-stable] [PATCH] kni: restrict bifurcated device support
@ 2021-10-08 23:58 Ferruh Yigit
  2021-10-09  2:03 ` Stephen Hemminger
  0 siblings, 1 reply; 2+ messages in thread
From: Ferruh Yigit @ 2021-10-08 23:58 UTC (permalink / raw)
  To: Elad Nachman
  Cc: Ferruh Yigit, dev, stable, Igor Ryzhov, Eric Christian,
	Stephen Hemminger

To enable bifurcated device support, rtnl_lock is released before calling
userspace callbacks and asynchronous requests are enabled.

But these changes caused more issues, like bug #809, #816. To reduce the
scope of the problems, the bifurcated device support related changes are
only enabled when it is requested explicitly with new 'enable_bifurcated'
module parameter.
And bifurcated device support is disabled by default.

So the bifurcated device related problems are isolated and they can be
fixed without impacting all use cases.

Bugzilla ID: 816
Fixes: 631217c76135 ("kni: fix kernel deadlock with bifurcated device")
Cc: stable@dpdk.org

Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
Cc: Igor Ryzhov <iryzhov@nfware.com>
Cc: Elad Nachman <eladv6@gmail.com>
Cc: Eric Christian <erclists@gmail.com>
Cc: Stephen Hemminger <stephen@networkplumber.org>
---
 .../prog_guide/kernel_nic_interface.rst       | 28 +++++++++++++
 kernel/linux/kni/kni_dev.h                    |  3 ++
 kernel/linux/kni/kni_misc.c                   | 36 ++++++++++++++++
 kernel/linux/kni/kni_net.c                    | 42 +++++++++++--------
 4 files changed, 92 insertions(+), 17 deletions(-)

diff --git a/doc/guides/prog_guide/kernel_nic_interface.rst b/doc/guides/prog_guide/kernel_nic_interface.rst
index 1ce03ec1a374..771c7d7fdac4 100644
--- a/doc/guides/prog_guide/kernel_nic_interface.rst
+++ b/doc/guides/prog_guide/kernel_nic_interface.rst
@@ -56,6 +56,12 @@ can be specified when the module is loaded to control its behavior:
                     off   Interfaces will be created with carrier state set to off.
                     on    Interfaces will be created with carrier state set to on.
                      (charp)
+    parm:           enable_bifurcated: Enable request processing support for
+                    bifurcated drivers, which means releasing rtnl_lock before calling
+                    userspace callback and supporting async requests (default=off):
+                    on    Enable request processing support for bifurcated drivers.
+                     (charp)
+
 
 Loading the ``rte_kni`` kernel module without any optional parameters is
 the typical way a DPDK application gets packets into and out of the kernel
@@ -174,6 +180,28 @@ To set the default carrier state to *off*:
 If the ``carrier`` parameter is not specified, the default carrier state
 of KNI interfaces will be set to *off*.
 
+.. _kni_bifurcated_device_support:
+
+Bifurcated Device Support
+~~~~~~~~~~~~~~~~~~~~~~~~~
+
+User callbacks are executed while kernel module holds the ``rtnl`` lock, this
+causes a deadlock when callbacks run control commands on another Linux kernel
+network interface.
+
+Bifurcated devices has kernel network driver part and to prevent deadlock for
+them ``enable_bifurcated`` is used.
+
+To enable bifurcated device support:
+
+.. code-block:: console
+
+    # insmod <build_dir>/kernel/linux/kni/rte_kni.ko enable_bifurcated=on
+
+Enabling bifurcated device support releases ``rtnl`` lock before calling
+callback and locks it back after callback. Also enables asynchronous request to
+support callbacks that requires rtnl lock to work (interface down).
+
 KNI Creation and Deletion
 -------------------------
 
diff --git a/kernel/linux/kni/kni_dev.h b/kernel/linux/kni/kni_dev.h
index c15da311ba25..e8633486eeb8 100644
--- a/kernel/linux/kni/kni_dev.h
+++ b/kernel/linux/kni/kni_dev.h
@@ -34,6 +34,9 @@
 /* Default carrier state for created KNI network interfaces */
 extern uint32_t kni_dflt_carrier;
 
+/* Request processing support for bifurcated drivers. */
+extern uint32_t bifurcated_support;
+
 /**
  * A structure describing the private information for a kni device.
  */
diff --git a/kernel/linux/kni/kni_misc.c b/kernel/linux/kni/kni_misc.c
index 2b464c438113..aae977c187a9 100644
--- a/kernel/linux/kni/kni_misc.c
+++ b/kernel/linux/kni/kni_misc.c
@@ -41,6 +41,10 @@ static uint32_t multiple_kthread_on;
 static char *carrier;
 uint32_t kni_dflt_carrier;
 
+/* Request processing support for bifurcated drivers. */
+static char *enable_bifurcated;
+uint32_t bifurcated_support;
+
 #define KNI_DEV_IN_USE_BIT_NUM 0 /* Bit number for device in use */
 
 static int kni_net_id;
@@ -568,6 +572,22 @@ kni_parse_carrier_state(void)
 	return 0;
 }
 
+static int __init
+kni_parse_bifurcated_support(void)
+{
+	if (!enable_bifurcated) {
+		bifurcated_support = 0;
+		return 0;
+	}
+
+	if (strcmp(enable_bifurcated, "on") == 0)
+		bifurcated_support = 1;
+	else
+		return -1;
+
+	return 0;
+}
+
 static int __init
 kni_init(void)
 {
@@ -593,6 +613,13 @@ kni_init(void)
 	else
 		pr_debug("Default carrier state set to on.\n");
 
+	if (kni_parse_bifurcated_support() < 0) {
+		pr_err("Invalid parameter for bifurcated support\n");
+		return -EINVAL;
+	}
+	if (bifurcated_support == 1)
+		pr_debug("bifurcated support is enabled.\n");
+
 #ifdef HAVE_SIMPLIFIED_PERNET_OPERATIONS
 	rc = register_pernet_subsys(&kni_net_ops);
 #else
@@ -659,3 +686,12 @@ MODULE_PARM_DESC(carrier,
 "\t\ton    Interfaces will be created with carrier state set to on.\n"
 "\t\t"
 );
+
+module_param(enable_bifurcated, charp, 0644);
+MODULE_PARM_DESC(enable_bifurcated,
+"Enable request processing support for bifurcated drivers, "
+"which means releasing rtnl_lock before calling userspace callback and "
+"supporting async requests (default=off):\n"
+"\t\ton    Enable request processing support for bifurcated drivers.\n"
+"\t\t"
+);
diff --git a/kernel/linux/kni/kni_net.c b/kernel/linux/kni/kni_net.c
index 611719b5ee27..29e5b9e21f9e 100644
--- a/kernel/linux/kni/kni_net.c
+++ b/kernel/linux/kni/kni_net.c
@@ -113,12 +113,14 @@ kni_net_process_request(struct net_device *dev, struct rte_kni_request *req)
 
 	ASSERT_RTNL();
 
-	/* If we need to wait and RTNL mutex is held
-	 * drop the mutex and hold reference to keep device
-	 */
-	if (req->async == 0) {
-		dev_hold(dev);
-		rtnl_unlock();
+	if (bifurcated_support) {
+		/* If we need to wait and RTNL mutex is held
+		 * drop the mutex and hold reference to keep device
+		 */
+		if (req->async == 0) {
+			dev_hold(dev);
+			rtnl_unlock();
+		}
 	}
 
 	mutex_lock(&kni->sync_lock);
@@ -132,12 +134,14 @@ kni_net_process_request(struct net_device *dev, struct rte_kni_request *req)
 		goto fail;
 	}
 
-	/* No result available since request is handled
-	 * asynchronously. set response to success.
-	 */
-	if (req->async != 0) {
-		req->result = 0;
-		goto async;
+	if (bifurcated_support) {
+		/* No result available since request is handled
+		 * asynchronously. set response to success.
+		 */
+		if (req->async != 0) {
+			req->result = 0;
+			goto async;
+		}
 	}
 
 	ret_val = wait_event_interruptible_timeout(kni->wq,
@@ -160,9 +164,11 @@ kni_net_process_request(struct net_device *dev, struct rte_kni_request *req)
 
 fail:
 	mutex_unlock(&kni->sync_lock);
-	if (req->async == 0) {
-		rtnl_lock();
-		dev_put(dev);
+	if (bifurcated_support) {
+		if (req->async == 0) {
+			rtnl_lock();
+			dev_put(dev);
+		}
 	}
 	return ret;
 }
@@ -207,8 +213,10 @@ kni_net_release(struct net_device *dev)
 	/* Setting if_up to 0 means down */
 	req.if_up = 0;
 
-	/* request async because of the deadlock problem */
-	req.async = 1;
+	if (bifurcated_support) {
+		/* request async because of the deadlock problem */
+		req.async = 1;
+	}
 
 	ret = kni_net_process_request(dev, &req);
 
-- 
2.31.1


^ permalink raw reply	[flat|nested] 2+ messages in thread

* Re: [dpdk-stable] [PATCH] kni: restrict bifurcated device support
  2021-10-08 23:58 [dpdk-stable] [PATCH] kni: restrict bifurcated device support Ferruh Yigit
@ 2021-10-09  2:03 ` Stephen Hemminger
  0 siblings, 0 replies; 2+ messages in thread
From: Stephen Hemminger @ 2021-10-09  2:03 UTC (permalink / raw)
  To: Ferruh Yigit; +Cc: Elad Nachman, dev, stable, Igor Ryzhov, Eric Christian

On Sat,  9 Oct 2021 00:58:30 +0100
Ferruh Yigit <ferruh.yigit@intel.com> wrote:

> To enable bifurcated device support, rtnl_lock is released before calling
> userspace callbacks and asynchronous requests are enabled.
> 
> But these changes caused more issues, like bug #809, #816. To reduce the
> scope of the problems, the bifurcated device support related changes are
> only enabled when it is requested explicitly with new 'enable_bifurcated'
> module parameter.
> And bifurcated device support is disabled by default.
> 
> So the bifurcated device related problems are isolated and they can be
> fixed without impacting all use cases.
> 
> Bugzilla ID: 816
> Fixes: 631217c76135 ("kni: fix kernel deadlock with bifurcated device")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>

Calling userspace with semaphore held is still risky and buggy.
There is no guarantee that the userspace DPDK application will be well behaved.
And if it is not, the spinning holding RTNL would break any other network management
functions in the kernel.

These are the kind of problems that make me think it there should be a
big "DO NOT USE THIS" onto KNI. Maybe make it print a big nasty message
(see kernel VFIO without IOMMU description) or mark kernel as tainted??

See: https://fedoraproject.org/wiki/KernelStagingPolicy

Something like:

diff --git a/kernel/linux/kni/kni_net.c b/kernel/linux/kni/kni_net.c
index 611719b5ee27..d47fc6133cbe 100644
--- a/kernel/linux/kni/kni_net.c
+++ b/kernel/linux/kni/kni_net.c
@@ -838,6 +838,14 @@ kni_net_init(struct net_device *dev)
 	dev->header_ops      = &kni_net_header_ops;
 	dev->ethtool_ops     = &kni_net_ethtool_ops;
 	dev->watchdog_timeo = WD_TIMEOUT;
+
+	/*
+	 * KNI is unsafe since it requires calling userspace to do
+	 * control operations. And the overall quality according to
+	 * kernel standards is the same as devices in staging.
+	 */
+	add_taint(TAINT_CRAP, LOCKDEP_STILL_OK);
+	netdev_warn(dev, "Adding kernel taint for KNI because it is not safe\n");
 }
 
 void

^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2021-10-09  2:03 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-08 23:58 [dpdk-stable] [PATCH] kni: restrict bifurcated device support Ferruh Yigit
2021-10-09  2:03 ` Stephen Hemminger

patches for DPDK stable branches

This inbox may be cloned and mirrored by anyone:

	git clone --mirror http://inbox.dpdk.org/stable/0 stable/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 stable stable/ http://inbox.dpdk.org/stable \
		stable@dpdk.org
	public-inbox-index stable

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.stable


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git