[dpdk-dev] [PATCH] kni: restrict bifurcated device support

[dpdk-dev] [PATCH] kni: restrict bifurcated device support

[Ferruh Yigit]

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

Re: [dpdk-dev] [PATCH] kni: restrict bifurcated device support

[Stephen Hemminger]

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

Re: [PATCH] kni: restrict bifurcated device support

[Igor Ryzhov]

[-- Attachment #1: Type: text/plain, Size: 8674 bytes --]

Acked-by: Igor Ryzhov <iryzhov@nfware.com>

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
>
>

[-- Attachment #2: Type: text/html, Size: 10908 bytes --]

Re: [PATCH] kni: restrict bifurcated device support

[Ferruh Yigit]

On 10/9/2021 3:03 AM, Stephen Hemminger wrote:
> 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.
> 

Hi Stephen,

Because of what you described above, we tried the option that releases the RTNL
lock before userspace callback,
That caused a deadlock in the ifdown, and we add a workaround for it.

But now we are facing with two more issues because of the rtnl lock release,
- Bugzilla ID: 816, MAC set cause kernel deadlock
- Some requests are overwritten (because of the workaround we put for ifdown)

This patch just converts the default behavior to what it was previously.
Releasing rtnl lock still supported with the module param, but I think it
is not good idea to make it default while it is know that it is buggy.

@Thomas, @David,
Can it be possible to get this patch for -rc4? Since it has potential
to cause a deadlock in kernel as it is.

I can send a new version with documentation update.

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

I am for discussing above separate from this patch, since this patch
restores the behavior that exist since KNI module exists.

Re: [PATCH] kni: restrict bifurcated device support

[Stephen Hemminger]

On Tue, 23 Nov 2021 09:54:01 +0000
Ferruh Yigit <ferruh.yigit@intel.com> wrote:

> On 10/9/2021 3:03 AM, Stephen Hemminger wrote:
> > 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.
> >   
> 
> Hi Stephen,
> 
> Because of what you described above, we tried the option that releases the RTNL
> lock before userspace callback,
> That caused a deadlock in the ifdown, and we add a workaround for it.
> 
> But now we are facing with two more issues because of the rtnl lock release,
> - Bugzilla ID: 816, MAC set cause kernel deadlock
> - Some requests are overwritten (because of the workaround we put for ifdown)
> 
> This patch just converts the default behavior to what it was previously.
> Releasing rtnl lock still supported with the module param, but I think it
> is not good idea to make it default while it is know that it is buggy.
> 
> @Thomas, @David,
> Can it be possible to get this patch for -rc4? Since it has potential
> to cause a deadlock in kernel as it is.
> 
> I can send a new version with documentation update.

Is it possible for userspace to choose the correct behavior instead
of module option. Users will do it wrong.


> 
> > 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");  
> 
> I am for discussing above separate from this patch, since this patch
> restores the behavior that exist since KNI module exists.

Any user of KNI will already get tainted because KNI is out of tree driver.

[PATCH v2] kni: restrict bifurcated device support

[Ferruh Yigit]

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>
Acked-by: Igor Ryzhov <iryzhov@nfware.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>
Cc: Sahithi Singam <sahithi.singam@oracle.com>

v2:
* Updated release note
* Verified the fix with two KNI interface setting MAC address in endless
  loop, and confirmed deadlock is not observed (which was observed
  without this patch).
---
 .../prog_guide/kernel_nic_interface.rst       | 28 +++++++++++++
 doc/guides/rel_notes/release_21_11.rst        |  6 +++
 kernel/linux/kni/kni_dev.h                    |  3 ++
 kernel/linux/kni/kni_misc.c                   | 36 ++++++++++++++++
 kernel/linux/kni/kni_net.c                    | 42 +++++++++++--------
 5 files changed, 98 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/doc/guides/rel_notes/release_21_11.rst b/doc/guides/rel_notes/release_21_11.rst
index 4d8c59472af1..fa2ce760d806 100644
--- a/doc/guides/rel_notes/release_21_11.rst
+++ b/doc/guides/rel_notes/release_21_11.rst
@@ -75,6 +75,12 @@ New Features
     operations.
   * Added multi-process support.
 
+* **Updated default KNI behavior on net devices control callbacks.**
+
+  Updated KNI net devices control callbacks to run with ``rtnl`` kernel lock
+  held by default. A newly added ``enable_bifurcated`` KNI kernel module
+  parameter can be used to run callbacks with ``rtnl`` lock released.
+
 * **Added HiSilicon DMA driver.**
 
   The HiSilicon DMA driver provides device drivers for the Kunpeng's DMA devices.
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 f4944e1ddf33..f10dcd069d00 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;
@@ -565,6 +569,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)
 {
@@ -590,6 +610,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
@@ -656,3 +683,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

Re: [PATCH] kni: restrict bifurcated device support

[Ferruh Yigit]

On 11/23/2021 4:22 PM, Stephen Hemminger wrote:
> On Tue, 23 Nov 2021 09:54:01 +0000
> Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> 
>> On 10/9/2021 3:03 AM, Stephen Hemminger wrote:
>>> 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.
>>>    
>>
>> Hi Stephen,
>>
>> Because of what you described above, we tried the option that releases the RTNL
>> lock before userspace callback,
>> That caused a deadlock in the ifdown, and we add a workaround for it.
>>
>> But now we are facing with two more issues because of the rtnl lock release,
>> - Bugzilla ID: 816, MAC set cause kernel deadlock
>> - Some requests are overwritten (because of the workaround we put for ifdown)
>>
>> This patch just converts the default behavior to what it was previously.
>> Releasing rtnl lock still supported with the module param, but I think it
>> is not good idea to make it default while it is know that it is buggy.
>>
>> @Thomas, @David,
>> Can it be possible to get this patch for -rc4? Since it has potential
>> to cause a deadlock in kernel as it is.
>>
>> I can send a new version with documentation update.
> 
> Is it possible for userspace to choose the correct behavior instead
> of module option. Users will do it wrong.
> 

That is why default is changed. If user will use bifurcated driver, user will
know it and need to explicitly request this support.

I don't think there is a way to know automatically which device/interface
will be used with KNI, that is why user config is required.

Right now KNI is with a defect that can cause a deadlock in the kernel with
its default config, that is why I think we should get this fix first for the
release.

> 
>>
>>> 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");
>>
>> I am for discussing above separate from this patch, since this patch
>> restores the behavior that exist since KNI module exists.
> 
> Any user of KNI will already get tainted because KNI is out of tree driver.
> 

I mean the note and explicit 'add_taint()' you mentioned above, can we separate
it from this bug fix.

Re: [PATCH] kni: restrict bifurcated device support

[Stephen Hemminger]

On Tue, 23 Nov 2021 16:51:27 +0000
Ferruh Yigit <ferruh.yigit@intel.com> wrote:

> >>>
> >>> 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");  
> >>
> >> I am for discussing above separate from this patch, since this patch
> >> restores the behavior that exist since KNI module exists.  
> > 
> > Any user of KNI will already get tainted because KNI is out of tree driver.
> >   
> 
> I mean the note and explicit 'add_taint()' you mentioned above, can we separate
> it from this bug fix.

I changed my mind, the existing taint that happens from out of tree is enough.
Especially since KNI is now deprecated.

Re: [PATCH v2] kni: restrict bifurcated device support

[Thomas Monjalon]

23/11/2021 17:46, Ferruh Yigit:
> 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>
> Acked-by: Igor Ryzhov <iryzhov@nfware.com>

This approach looks pragmatic.
BTW, I'm not sure there is a strong need for KNI with bifurcated device.

Applied, thanks.