kni: fix kernel deadlock when using mlx devices
diff mbox series

Message ID 20191222175551.17684-1-stephen@networkplumber.org
State Under Review
Delegated to: Ferruh Yigit
Headers show
Series
  • kni: fix kernel deadlock when using mlx devices
Related show

Checks

Context Check Description
ci/Intel-compilation fail Compilation issues
ci/travis-robot success Travis build: passed
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-testing success Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/checkpatch success coding style OK

Commit Message

Stephen Hemminger Dec. 22, 2019, 5:55 p.m. UTC
This fixes a deadlock when using KNI with bifurcated drivers.
Bringing kni device up always times out when using Mellanox
devices.

The kernel KNI driver sends message to userspace to complete
the request. For the case of bifurcated driver, this may involve
an additional request to kernel to change state. This request
would deadlock because KNI was holding the RTNL mutex.

This was a bad design which goes back to the original code.
A workaround is for KNI driver to drop RTNL while waiting.
To prevent the device from disappearing while the operation
is in progress, it needs to hold reference to network device
while waiting.

As an added benefit, an useless error check can also be removed.

Fixes: 3fc5ca2f6352 ("kni: initial import")
Cc: stable@dpdk.org
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 kernel/linux/kni/kni_net.c | 34 ++++++++++++++++++----------------
 1 file changed, 18 insertions(+), 16 deletions(-)

Comments

Ferruh Yigit Jan. 17, 2020, 4:43 p.m. UTC | #1
On 12/22/2019 5:55 PM, Stephen Hemminger wrote:
> This fixes a deadlock when using KNI with bifurcated drivers.
> Bringing kni device up always times out when using Mellanox
> devices.
> 
> The kernel KNI driver sends message to userspace to complete
> the request. For the case of bifurcated driver, this may involve
> an additional request to kernel to change state. This request
> would deadlock because KNI was holding the RTNL mutex.
> 
> This was a bad design which goes back to the original code.
> A workaround is for KNI driver to drop RTNL while waiting.
> To prevent the device from disappearing while the operation
> is in progress, it needs to hold reference to network device
> while waiting.
> 
> As an added benefit, an useless error check can also be removed.
> 
> Fixes: 3fc5ca2f6352 ("kni: initial import")
> Cc: stable@dpdk.org
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  kernel/linux/kni/kni_net.c | 34 ++++++++++++++++++----------------
>  1 file changed, 18 insertions(+), 16 deletions(-)
> 
> diff --git a/kernel/linux/kni/kni_net.c b/kernel/linux/kni/kni_net.c
> index 1ba9b1b99f66..b7337c1410b8 100644
> --- a/kernel/linux/kni/kni_net.c
> +++ b/kernel/linux/kni/kni_net.c
> @@ -17,6 +17,7 @@
>  #include <linux/skbuff.h>
>  #include <linux/kthread.h>
>  #include <linux/delay.h>
> +#include <linux/rtnetlink.h>
>  
>  #include <rte_kni_common.h>
>  #include <kni_fifo.h>
> @@ -102,17 +103,15 @@ get_data_kva(struct kni_dev *kni, void *pkt_kva)
>   * It can be called to process the request.
>   */
>  static int
> -kni_net_process_request(struct kni_dev *kni, struct rte_kni_request *req)
> +kni_net_process_request(struct net_device *dev, struct rte_kni_request *req)
>  {
> +	struct kni_dev *kni = netdev_priv(dev);
>  	int ret = -1;
>  	void *resp_va;
>  	uint32_t num;
>  	int ret_val;
>  
> -	if (!kni || !req) {
> -		pr_err("No kni instance or request\n");
> -		return -EINVAL;
> -	}
> +	ASSERT_RTNL();
>  
>  	mutex_lock(&kni->sync_lock);
>  
> @@ -125,8 +124,17 @@ kni_net_process_request(struct kni_dev *kni, struct rte_kni_request *req)
>  		goto fail;
>  	}
>  
> +	/* Since we need to wait and RTNL mutex is held
> +	 * drop the mutex and hold refernce to keep device
> +	 */
> +	dev_hold(dev);
> +	rtnl_unlock();
> +
>  	ret_val = wait_event_interruptible_timeout(kni->wq,
>  			kni_fifo_count(kni->resp_q), 3 * HZ);
> +	rtnl_lock();
> +	dev_put(dev);
> +
>  	if (signal_pending(current) || ret_val <= 0) {
>  		ret = -ETIME;
>  		goto fail;

<...>

This patch cause a hang on my server, not sure what exactly was the problem but
kernel log was continuously printing "Cannot send to req_q". Will dig more.

Patch
diff mbox series

diff --git a/kernel/linux/kni/kni_net.c b/kernel/linux/kni/kni_net.c
index 1ba9b1b99f66..b7337c1410b8 100644
--- a/kernel/linux/kni/kni_net.c
+++ b/kernel/linux/kni/kni_net.c
@@ -17,6 +17,7 @@ 
 #include <linux/skbuff.h>
 #include <linux/kthread.h>
 #include <linux/delay.h>
+#include <linux/rtnetlink.h>
 
 #include <rte_kni_common.h>
 #include <kni_fifo.h>
@@ -102,17 +103,15 @@  get_data_kva(struct kni_dev *kni, void *pkt_kva)
  * It can be called to process the request.
  */
 static int
-kni_net_process_request(struct kni_dev *kni, struct rte_kni_request *req)
+kni_net_process_request(struct net_device *dev, struct rte_kni_request *req)
 {
+	struct kni_dev *kni = netdev_priv(dev);
 	int ret = -1;
 	void *resp_va;
 	uint32_t num;
 	int ret_val;
 
-	if (!kni || !req) {
-		pr_err("No kni instance or request\n");
-		return -EINVAL;
-	}
+	ASSERT_RTNL();
 
 	mutex_lock(&kni->sync_lock);
 
@@ -125,8 +124,17 @@  kni_net_process_request(struct kni_dev *kni, struct rte_kni_request *req)
 		goto fail;
 	}
 
+	/* Since we need to wait and RTNL mutex is held
+	 * drop the mutex and hold refernce to keep device
+	 */
+	dev_hold(dev);
+	rtnl_unlock();
+
 	ret_val = wait_event_interruptible_timeout(kni->wq,
 			kni_fifo_count(kni->resp_q), 3 * HZ);
+	rtnl_lock();
+	dev_put(dev);
+
 	if (signal_pending(current) || ret_val <= 0) {
 		ret = -ETIME;
 		goto fail;
@@ -155,7 +163,6 @@  kni_net_open(struct net_device *dev)
 {
 	int ret;
 	struct rte_kni_request req;
-	struct kni_dev *kni = netdev_priv(dev);
 
 	netif_start_queue(dev);
 	if (dflt_carrier == 1)
@@ -168,7 +175,7 @@  kni_net_open(struct net_device *dev)
 
 	/* Setting if_up to non-zero means up */
 	req.if_up = 1;
-	ret = kni_net_process_request(kni, &req);
+	ret = kni_net_process_request(dev, &req);
 
 	return (ret == 0) ? req.result : ret;
 }
@@ -178,7 +185,6 @@  kni_net_release(struct net_device *dev)
 {
 	int ret;
 	struct rte_kni_request req;
-	struct kni_dev *kni = netdev_priv(dev);
 
 	netif_stop_queue(dev); /* can't transmit any more */
 	netif_carrier_off(dev);
@@ -188,7 +194,7 @@  kni_net_release(struct net_device *dev)
 
 	/* Setting if_up to 0 means down */
 	req.if_up = 0;
-	ret = kni_net_process_request(kni, &req);
+	ret = kni_net_process_request(dev, &req);
 
 	return (ret == 0) ? req.result : ret;
 }
@@ -638,14 +644,13 @@  kni_net_change_mtu(struct net_device *dev, int new_mtu)
 {
 	int ret;
 	struct rte_kni_request req;
-	struct kni_dev *kni = netdev_priv(dev);
 
 	pr_debug("kni_net_change_mtu new mtu %d to be set\n", new_mtu);
 
 	memset(&req, 0, sizeof(req));
 	req.req_id = RTE_KNI_REQ_CHANGE_MTU;
 	req.new_mtu = new_mtu;
-	ret = kni_net_process_request(kni, &req);
+	ret = kni_net_process_request(dev, &req);
 	if (ret == 0 && req.result == 0)
 		dev->mtu = new_mtu;
 
@@ -656,7 +661,6 @@  static void
 kni_net_change_rx_flags(struct net_device *netdev, int flags)
 {
 	struct rte_kni_request req;
-	struct kni_dev *kni = netdev_priv(netdev);
 
 	memset(&req, 0, sizeof(req));
 
@@ -678,7 +682,7 @@  kni_net_change_rx_flags(struct net_device *netdev, int flags)
 			req.promiscusity = 0;
 	}
 
-	kni_net_process_request(kni, &req);
+	kni_net_process_request(netdev, &req);
 }
 
 /*
@@ -737,7 +741,6 @@  kni_net_set_mac(struct net_device *netdev, void *p)
 {
 	int ret;
 	struct rte_kni_request req;
-	struct kni_dev *kni;
 	struct sockaddr *addr = p;
 
 	memset(&req, 0, sizeof(req));
@@ -749,8 +752,7 @@  kni_net_set_mac(struct net_device *netdev, void *p)
 	memcpy(req.mac_addr, addr->sa_data, netdev->addr_len);
 	memcpy(netdev->dev_addr, addr->sa_data, netdev->addr_len);
 
-	kni = netdev_priv(netdev);
-	ret = kni_net_process_request(kni, &req);
+	ret = kni_net_process_request(netdev, &req);
 
 	return (ret == 0 ? req.result : ret);
 }