Re: [PATCH] kni: unregister an unregisterd net_device could cause a kernel crash

Re: [PATCH] kni: unregister an unregisterd net_device could cause a kernel crash

[Stephen Hemminger]

On Fri,  9 Sep 2016 10:46:07 +0800
zhouyangchao <zhouyates@gmail.com> wrote:

> Signed-off-by: zhouyangchao <zhouyates@gmail.com>
> ---
>  lib/librte_eal/linuxapp/kni/kni_misc.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/lib/librte_eal/linuxapp/kni/kni_misc.c b/lib/librte_eal/linuxapp/kni/kni_misc.c
> index 67e9b7d..ad4e603 100644
> --- a/lib/librte_eal/linuxapp/kni/kni_misc.c
> +++ b/lib/librte_eal/linuxapp/kni/kni_misc.c
> @@ -361,7 +361,9 @@ kni_dev_remove(struct kni_dev *dev)
>  		igb_kni_remove(dev->pci_dev);
>  
>  	if (dev->net_dev) {
> -		unregister_netdev(dev->net_dev);
> +		if (dev->netdev->reg_state == NETREG_REGISTERED){
----------------------------------------------------------------^
Incorrect whitespace. But then again the whole KNI driver fails completely when
running kernel style check.

Re: [PATCH] kni: unregister an unregisterd net_device could cause a kernel crash

[Ferruh Yigit]

On 9/9/2016 3:46 AM, zhouyangchao wrote:
> Signed-off-by: zhouyangchao <zhouyates@gmail.com>
> ---
>  lib/librte_eal/linuxapp/kni/kni_misc.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/lib/librte_eal/linuxapp/kni/kni_misc.c b/lib/librte_eal/linuxapp/kni/kni_misc.c
> index 67e9b7d..ad4e603 100644
> --- a/lib/librte_eal/linuxapp/kni/kni_misc.c
> +++ b/lib/librte_eal/linuxapp/kni/kni_misc.c
> @@ -361,7 +361,9 @@ kni_dev_remove(struct kni_dev *dev)
>  		igb_kni_remove(dev->pci_dev);
>  
>  	if (dev->net_dev) {
> -		unregister_netdev(dev->net_dev);
> +		if (dev->netdev->reg_state == NETREG_REGISTERED){

Although this will work, I believe we shouldn't know more about netdev
internals unless we don't have to, and for this case we don't need to
know it. More Linux internal knowledge means more ways to be broken in
the future.
Also I am not sure if reg_state intended to be used by network drivers.

I am for first version of your patch, with missing free_netdev() added
into rollback code.

pseudo code:
net_dev = alloc_netdev()
...
ret = register_netdev()
if (ret)
	kni_dev_remove()
	free_netdev()
	return
kni->net_dev = net_dev


OR if don't want to move where kni->net_dev assigned

net_dev = alloc_netdev()
kni->net_dev = net_dev
...
ret = register_netdev()
if (ret)
	kni->net_dev = NULL
	kni_dev_remove()
	free_netdev()
	return




> +			unregister_netdev(dev->net_dev);
> +		}
>  		free_netdev(dev->net_dev);
>  	}
>  
> 

Re: [PATCH] kni: unregister an unregisterd net_device could cause a kernel crash

[Ferruh Yigit]

On 9/8/2016 5:44 PM, Stephen Hemminger wrote:

...

> But then again the whole KNI driver fails completely when
> running kernel style check.
> 

Yes, it generates lots of warnings.
I can fix them (excluding ethtool/*), that wouldn't take much time but
how syntax only patches welcomed? Another concern is it trashes git blame.

Regards,
ferruh

[PATCH] kni: unregister an unregisterd net_device could cause a kernel crash

[zhouyangchao]

Signed-off-by: zhouyangchao <zhouyates@gmail.com>
---
 lib/librte_eal/linuxapp/kni/kni_misc.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/lib/librte_eal/linuxapp/kni/kni_misc.c b/lib/librte_eal/linuxapp/kni/kni_misc.c
index 67e9b7d..ad4e603 100644
--- a/lib/librte_eal/linuxapp/kni/kni_misc.c
+++ b/lib/librte_eal/linuxapp/kni/kni_misc.c
@@ -361,7 +361,9 @@ kni_dev_remove(struct kni_dev *dev)
 		igb_kni_remove(dev->pci_dev);
 
 	if (dev->net_dev) {
-		unregister_netdev(dev->net_dev);
+		if (dev->netdev->reg_state == NETREG_REGISTERED){
+			unregister_netdev(dev->net_dev);
+		}
 		free_netdev(dev->net_dev);
 	}
 
-- 
1.7.1

Re: [PATCH] kni: unregister an unregisterd net_device could cause a kernel crash

[Thomas Monjalon]

2016-09-08 18:15, Ferruh Yigit:
> On 9/8/2016 5:44 PM, Stephen Hemminger wrote:
> 
> ...
> 
> > But then again the whole KNI driver fails completely when
> > running kernel style check.
> > 
> 
> Yes, it generates lots of warnings.
> I can fix them (excluding ethtool/*), that wouldn't take much time but
> how syntax only patches welcomed? Another concern is it trashes git blame.

You ask a question and give the answer ;)
I think it depends just on the balance of the pros/cons - to be evaluated.

Re: [PATCH] kni: unregister an unregisterd net_device could cause a kernel crash

[Mcnamara, John]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
> Sent: Friday, September 9, 2016 1:40 PM
> To: Yigit, Ferruh <ferruh.yigit@intel.com>
> Cc: Stephen Hemminger <stephen@networkplumber.org>; zhouyangchao
> <zhouyates@gmail.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] kni: unregister an unregisterd net_device
> could cause a kernel crash
> 
> 2016-09-08 18:15, Ferruh Yigit:
> > On 9/8/2016 5:44 PM, Stephen Hemminger wrote:
> >
> > ...
> >
> > > But then again the whole KNI driver fails completely when running
> > > kernel style check.
> > >
> >
> > Yes, it generates lots of warnings.
> > I can fix them (excluding ethtool/*), that wouldn't take much time but
> > how syntax only patches welcomed? Another concern is it trashes git
> blame.
> 
> You ask a question and give the answer ;) I think it depends just on the
> balance of the pros/cons - to be evaluated.

Hi,

I think in general we would prefer to avoid any large scale code beautification since, as pointed out, it breaks the option to git blame.

However, in the case of the KNI code the main author in git is "Intel" so git blame doesn't tell you a lot. Also, Ferruh is the maintainer, has made most of the recent changes, and is actively maintaining/improving it. So I think if the syntax fix came from him it would be okay. At least it would allow us to apply the checkpatch checks.


John

Re: [PATCH] kni: unregister an unregisterd net_device could cause a kernel crash

[Thomas Monjalon]

2016-09-09 14:33, Mcnamara, John:
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
> > 2016-09-08 18:15, Ferruh Yigit:
> > > On 9/8/2016 5:44 PM, Stephen Hemminger wrote:
> > >
> > > ...
> > >
> > > > But then again the whole KNI driver fails completely when running
> > > > kernel style check.
> > > >
> > >
> > > Yes, it generates lots of warnings.
> > > I can fix them (excluding ethtool/*), that wouldn't take much time but
> > > how syntax only patches welcomed? Another concern is it trashes git
> > blame.
> > 
> > You ask a question and give the answer ;) I think it depends just on the
> > balance of the pros/cons - to be evaluated.
> 
> Hi,
> 
> I think in general we would prefer to avoid any large scale code beautification since, as pointed out, it breaks the option to git blame.
> 
> However, in the case of the KNI code the main author in git is "Intel" so git blame doesn't tell you a lot. Also, Ferruh is the maintainer, has made most of the recent changes, and is actively maintaining/improving it. So I think if the syntax fix came from him it would be okay. At least it would allow us to apply the checkpatch checks.

Yes seems reasonnable

[PATCH] kni: unregister an unregisterd net_device could cause a kernel crash

[zhouyangchao]

Signed-off-by: zhouyangchao <zhouyates@gmail.com>
---
 lib/librte_eal/linuxapp/kni/kni_misc.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/librte_eal/linuxapp/kni/kni_misc.c b/lib/librte_eal/linuxapp/kni/kni_misc.c
index 67e9b7d..5c58a83 100644
--- a/lib/librte_eal/linuxapp/kni/kni_misc.c
+++ b/lib/librte_eal/linuxapp/kni/kni_misc.c
@@ -545,7 +545,9 @@ kni_ioctl_create(struct net *net,
 	if (ret) {
 		KNI_ERR("error %i registering device \"%s\"\n",
 					ret, dev_info.name);
+		kni->net_dev = NULL;
 		kni_dev_remove(kni);
+		free_netdev(net_dev);
 		return -ENODEV;
 	}
 
-- 
2.10.0

Re: [PATCH] kni: unregister an unregisterd net_device could cause a kernel crash

[Ferruh Yigit]

On 9/11/2016 10:59 AM, zhouyangchao wrote:
> Signed-off-by: zhouyangchao <zhouyates@gmail.com>
> ---

There is a typo in the patch subject, I suggest:
kni: fix error rollback kernel crash

Fixes: 9c61145ff6f9 ("kni: allow multiple threads")

Acked-by: Ferruh Yigit <ferruh.yigit@intel.com>

Re: [PATCH] kni: unregister an unregisterd net_device could cause a kernel crash

[Thomas Monjalon]

2016-09-13 14:13, Ferruh Yigit:
> On 9/11/2016 10:59 AM, zhouyangchao wrote:
> > Signed-off-by: zhouyangchao <zhouyates@gmail.com>
> 
> There is a typo in the patch subject, I suggest:
> kni: fix error rollback kernel crash
> 
> Fixes: 9c61145ff6f9 ("kni: allow multiple threads")
> 
> Acked-by: Ferruh Yigit <ferruh.yigit@intel.com>

Applied, thanks

[PATCH] kni: unregister an unregisterd net_device could cause a kernel crash

[zhouyangchao]

Signed-off-by: zhouyangchao <zhouyates@gmail.com>
---
 lib/librte_eal/linuxapp/kni/kni_misc.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/lib/librte_eal/linuxapp/kni/kni_misc.c b/lib/librte_eal/linuxapp/kni/kni_misc.c
index 67e9b7d..17b6d7a 100644
--- a/lib/librte_eal/linuxapp/kni/kni_misc.c
+++ b/lib/librte_eal/linuxapp/kni/kni_misc.c
@@ -361,6 +361,9 @@ kni_dev_remove(struct kni_dev *dev)
 		igb_kni_remove(dev->pci_dev);
 
 	if (dev->net_dev) {
+		if (dev->net_dev->state == NETREG_REGISTERED) {
+			unregister_netdev(dev->net_dev);
+		}
 		unregister_netdev(dev->net_dev);
 		free_netdev(dev->net_dev);
 	}
-- 
1.7.1

Re: [PATCH] kni: unregister an unregisterd net_device could cause a kernel crash

[Stephen Hemminger]

On Fri,  9 Sep 2016 10:42:16 +0800
zhouyangchao <zhouyates@gmail.com> wrote:

> Signed-off-by: zhouyangchao <zhouyates@gmail.com>
> ---
>  lib/librte_eal/linuxapp/kni/kni_misc.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/lib/librte_eal/linuxapp/kni/kni_misc.c b/lib/librte_eal/linuxapp/kni/kni_misc.c
> index 67e9b7d..17b6d7a 100644
> --- a/lib/librte_eal/linuxapp/kni/kni_misc.c
> +++ b/lib/librte_eal/linuxapp/kni/kni_misc.c
> @@ -361,6 +361,9 @@ kni_dev_remove(struct kni_dev *dev)
>  		igb_kni_remove(dev->pci_dev);
>  
>  	if (dev->net_dev) {
> +		if (dev->net_dev->state == NETREG_REGISTERED) {
> +			unregister_netdev(dev->net_dev);
> +		}
>  		unregister_netdev(dev->net_dev);
>  		free_netdev(dev->net_dev);
>  	}

The real problem is kni_dev_remove should not be called when register_netdevice
fails. Why not just fix that unwind path.