[dpdk-dev] kni: unregister an unregisterd net_device could cause a kernel crash

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

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.
  

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

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
  

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.
  

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

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