On Sat, Oct 9, 2021 at 2:58 AM 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>
---
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