* [dpdk-dev] [PATCH] kni: unregister an unregisterd net_device could cause a kernel crash
@ 2016-09-09 2:42 zhouyangchao
2016-09-08 16:47 ` Stephen Hemminger
0 siblings, 1 reply; 12+ messages in thread
From: zhouyangchao @ 2016-09-09 2:42 UTC (permalink / raw)
To: ferruh.yigit; +Cc: dev, 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
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [dpdk-dev] [PATCH] kni: unregister an unregisterd net_device could cause a kernel crash
2016-09-09 2:42 [dpdk-dev] [PATCH] kni: unregister an unregisterd net_device could cause a kernel crash zhouyangchao
@ 2016-09-08 16:47 ` Stephen Hemminger
0 siblings, 0 replies; 12+ messages in thread
From: Stephen Hemminger @ 2016-09-08 16:47 UTC (permalink / raw)
To: zhouyangchao; +Cc: ferruh.yigit, dev
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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* [dpdk-dev] [PATCH] kni: unregister an unregisterd net_device could cause a kernel crash
@ 2016-09-09 2:46 zhouyangchao
2016-09-08 16:44 ` Stephen Hemminger
2016-09-08 17:07 ` Ferruh Yigit
0 siblings, 2 replies; 12+ messages in thread
From: zhouyangchao @ 2016-09-09 2:46 UTC (permalink / raw)
To: ferruh.yigit; +Cc: dev, 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
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [dpdk-dev] [PATCH] kni: unregister an unregisterd net_device could cause a kernel crash
2016-09-09 2:46 zhouyangchao
@ 2016-09-08 16:44 ` Stephen Hemminger
2016-09-08 17:15 ` Ferruh Yigit
2016-09-08 17:07 ` Ferruh Yigit
1 sibling, 1 reply; 12+ messages in thread
From: Stephen Hemminger @ 2016-09-08 16:44 UTC (permalink / raw)
To: zhouyangchao; +Cc: ferruh.yigit, dev
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.
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [dpdk-dev] [PATCH] kni: unregister an unregisterd net_device could cause a kernel crash
2016-09-08 16:44 ` Stephen Hemminger
@ 2016-09-08 17:15 ` Ferruh Yigit
2016-09-09 12:40 ` Thomas Monjalon
0 siblings, 1 reply; 12+ messages in thread
From: Ferruh Yigit @ 2016-09-08 17:15 UTC (permalink / raw)
To: Stephen Hemminger, zhouyangchao; +Cc: dev, Thomas Monjalon
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [dpdk-dev] [PATCH] kni: unregister an unregisterd net_device could cause a kernel crash
2016-09-08 17:15 ` Ferruh Yigit
@ 2016-09-09 12:40 ` Thomas Monjalon
2016-09-09 14:33 ` Mcnamara, John
0 siblings, 1 reply; 12+ messages in thread
From: Thomas Monjalon @ 2016-09-09 12:40 UTC (permalink / raw)
To: Ferruh Yigit; +Cc: Stephen Hemminger, zhouyangchao, dev
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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [dpdk-dev] [PATCH] kni: unregister an unregisterd net_device could cause a kernel crash
2016-09-09 12:40 ` Thomas Monjalon
@ 2016-09-09 14:33 ` Mcnamara, John
2016-09-10 7:57 ` Thomas Monjalon
0 siblings, 1 reply; 12+ messages in thread
From: Mcnamara, John @ 2016-09-09 14:33 UTC (permalink / raw)
To: Thomas Monjalon, Yigit, Ferruh; +Cc: Stephen Hemminger, zhouyangchao, dev
> -----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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [dpdk-dev] [PATCH] kni: unregister an unregisterd net_device could cause a kernel crash
2016-09-09 14:33 ` Mcnamara, John
@ 2016-09-10 7:57 ` Thomas Monjalon
0 siblings, 0 replies; 12+ messages in thread
From: Thomas Monjalon @ 2016-09-10 7:57 UTC (permalink / raw)
To: Mcnamara, John; +Cc: Yigit, Ferruh, Stephen Hemminger, zhouyangchao, dev
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [dpdk-dev] [PATCH] kni: unregister an unregisterd net_device could cause a kernel crash
2016-09-09 2:46 zhouyangchao
2016-09-08 16:44 ` Stephen Hemminger
@ 2016-09-08 17:07 ` Ferruh Yigit
2016-09-11 9:59 ` zhouyangchao
1 sibling, 1 reply; 12+ messages in thread
From: Ferruh Yigit @ 2016-09-08 17:07 UTC (permalink / raw)
To: zhouyangchao; +Cc: dev
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);
> }
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread* [dpdk-dev] [PATCH] kni: unregister an unregisterd net_device could cause a kernel crash
2016-09-08 17:07 ` Ferruh Yigit
@ 2016-09-11 9:59 ` zhouyangchao
2016-09-13 13:13 ` Ferruh Yigit
0 siblings, 1 reply; 12+ messages in thread
From: zhouyangchao @ 2016-09-11 9:59 UTC (permalink / raw)
To: ferruh.yigit; +Cc: dev, 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
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [dpdk-dev] [PATCH] kni: unregister an unregisterd net_device could cause a kernel crash
2016-09-11 9:59 ` zhouyangchao
@ 2016-09-13 13:13 ` Ferruh Yigit
2016-09-21 16:41 ` Thomas Monjalon
0 siblings, 1 reply; 12+ messages in thread
From: Ferruh Yigit @ 2016-09-13 13:13 UTC (permalink / raw)
To: zhouyangchao; +Cc: dev, stable
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>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [dpdk-dev] [PATCH] kni: unregister an unregisterd net_device could cause a kernel crash
2016-09-13 13:13 ` Ferruh Yigit
@ 2016-09-21 16:41 ` Thomas Monjalon
0 siblings, 0 replies; 12+ messages in thread
From: Thomas Monjalon @ 2016-09-21 16:41 UTC (permalink / raw)
To: zhouyangchao; +Cc: dev, Ferruh Yigit, stable
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
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2016-09-21 16:41 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-09 2:42 [dpdk-dev] [PATCH] kni: unregister an unregisterd net_device could cause a kernel crash zhouyangchao
2016-09-08 16:47 ` Stephen Hemminger
2016-09-09 2:46 zhouyangchao
2016-09-08 16:44 ` Stephen Hemminger
2016-09-08 17:15 ` Ferruh Yigit
2016-09-09 12:40 ` Thomas Monjalon
2016-09-09 14:33 ` Mcnamara, John
2016-09-10 7:57 ` Thomas Monjalon
2016-09-08 17:07 ` Ferruh Yigit
2016-09-11 9:59 ` zhouyangchao
2016-09-13 13:13 ` Ferruh Yigit
2016-09-21 16:41 ` Thomas Monjalon
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).