[dpdk-stable] [PATCH] kni: fix kernel deadlock when using mlx devices

[dpdk-stable] [PATCH] kni: fix kernel deadlock when using mlx devices

[Stephen Hemminger]

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;
@@ -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);
 }
-- 
2.20.1

Re: [dpdk-stable] [PATCH] kni: fix kernel deadlock when using mlx devices

[Ferruh Yigit]

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.

Re: [dpdk-stable] [dpdk-dev] [PATCH] kni: fix kernel deadlock when using mlx devices

[Thomas Monjalon]

17/01/2020 17:43, Ferruh Yigit:
> 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>
> > ---
> 
> 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.

Ferruh, did you have a chance to check what is hanging?
Stephen, is there any news on your side?

Re: [dpdk-stable] [dpdk-dev] [PATCH] kni: fix kernel deadlock when using mlx devices

[Stephen Hemminger]

On Wed, 18 Mar 2020 16:17:57 +0100
Thomas Monjalon <thomas@monjalon.net> wrote:

> 17/01/2020 17:43, Ferruh Yigit:
> > 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>
> > > ---  
> > 
> > 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.  
> 
> Ferruh, did you have a chance to check what is hanging?
> Stephen, is there any news on your side?
> 
> 

It did not hang when I tested it. The bug report is still open

Re: [dpdk-stable] [dpdk-dev] [PATCH] kni: fix kernel deadlock when using mlx devices

[Ferruh Yigit]

On 5/6/2020 1:14 AM, Stephen Hemminger wrote:
> On Wed, 18 Mar 2020 16:17:57 +0100
> Thomas Monjalon <thomas@monjalon.net> wrote:
> 
>> 17/01/2020 17:43, Ferruh Yigit:
>>> 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>
>>>> ---  
>>>
>>> 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.  
>>
>> Ferruh, did you have a chance to check what is hanging?
>> Stephen, is there any news on your side?
>>
>>
> 
> It did not hang when I tested it. The bug report is still open
> 

Sorry for the delay, since I am working remotely I was worried about loosing the
connection to my server, finally I did create a virtual environment to test again.

I confirm the hang observed %100 when two different process updates the kni
interface, like two different process sets the mtu. Without this patch this
works fine.

I understand the motivation of the patch, but with change there is a possibility
to hang the server, which we can't allow, need to find another way. Can updating
mlx interface wait KNI interface operation to complete?

Re: [dpdk-stable] [dpdk-dev] [PATCH] kni: fix kernel deadlock when using mlx devices

[Stephen Hemminger]

On Mon, 27 Jul 2020 18:33:08 +0100
Ferruh Yigit <ferruh.yigit@intel.com> wrote:

> On 5/6/2020 1:14 AM, Stephen Hemminger wrote:
> > On Wed, 18 Mar 2020 16:17:57 +0100
> > Thomas Monjalon <thomas@monjalon.net> wrote:
> >   
> >> 17/01/2020 17:43, Ferruh Yigit:  
> >>> 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>
> >>>> ---    
> >>>
> >>> 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.    
> >>
> >> Ferruh, did you have a chance to check what is hanging?
> >> Stephen, is there any news on your side?
> >>
> >>  
> > 
> > It did not hang when I tested it. The bug report is still open
> >   
> 
> Sorry for the delay, since I am working remotely I was worried about loosing the
> connection to my server, finally I did create a virtual environment to test again.
> 
> I confirm the hang observed %100 when two different process updates the kni
> interface, like two different process sets the mtu. Without this patch this
> works fine.
> 
> I understand the motivation of the patch, but with change there is a possibility
> to hang the server, which we can't allow, need to find another way. Can updating
> mlx interface wait KNI interface operation to complete?

Still KNI driver is broken. Calling userspace with RTNL held is fundamentally
broken design. If KNI were to be incorporated in upstream kernel, then the netdev
developer would see this.

What ever solution you think is best.
I will continue to recommend against anyone using KNI.

Re: [dpdk-stable] [dpdk-dev] [PATCH] kni: fix kernel deadlock when using mlx devices

[Igor Ryzhov]

Hi,

Didn't see this patch previously, but we came up with the same idea
internally and also faced a hang during the application shutdown.
We didn't dig deep, but it occurred in kni_release function.

Igor

On Mon, Jul 27, 2020 at 8:53 PM Stephen Hemminger <
stephen@networkplumber.org> wrote:

> On Mon, 27 Jul 2020 18:33:08 +0100
> Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>
> > On 5/6/2020 1:14 AM, Stephen Hemminger wrote:
> > > On Wed, 18 Mar 2020 16:17:57 +0100
> > > Thomas Monjalon <thomas@monjalon.net> wrote:
> > >
> > >> 17/01/2020 17:43, Ferruh Yigit:
> > >>> 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>
> > >>>> ---
> > >>>
> > >>> 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.
> > >>
> > >> Ferruh, did you have a chance to check what is hanging?
> > >> Stephen, is there any news on your side?
> > >>
> > >>
> > >
> > > It did not hang when I tested it. The bug report is still open
> > >
> >
> > Sorry for the delay, since I am working remotely I was worried about
> loosing the
> > connection to my server, finally I did create a virtual environment to
> test again.
> >
> > I confirm the hang observed %100 when two different process updates the
> kni
> > interface, like two different process sets the mtu. Without this patch
> this
> > works fine.
> >
> > I understand the motivation of the patch, but with change there is a
> possibility
> > to hang the server, which we can't allow, need to find another way. Can
> updating
> > mlx interface wait KNI interface operation to complete?
>
> Still KNI driver is broken. Calling userspace with RTNL held is
> fundamentally
> broken design. If KNI were to be incorporated in upstream kernel, then the
> netdev
> developer would see this.
>
> What ever solution you think is best.
> I will continue to recommend against anyone using KNI.
>