[dpdk-dev] [PATCH] kni: unregister an unregisterd net_device could cause a kernel crash
On 9/11/2016 10:59 AM, zhouyangchao wrote:
> Signed-off-by: zhouyangchao
> ---
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
[dpdk-dev] [PATCH] kni: unregister an unregisterd net_device could cause a kernel crash
Signed-off-by: zhouyangchao
---
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
[dpdk-dev] [PATCH] kni: unregister an unregisterd net_device could cause a kernel crash
2016-09-09 14:33, Mcnamara, John: > From: dev [mailto:dev-bounces at 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
[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.
[dpdk-dev] [PATCH] kni: unregister an unregisterd net_device could cause a kernel crash
> -Original Message- > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Thomas Monjalon > Sent: Friday, September 9, 2016 1:40 PM > To: Yigit, Ferruh > Cc: Stephen Hemminger ; zhouyangchao > ; dev at 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
[dpdk-dev] [PATCH] kni: unregister an unregisterd net_device could cause a kernel crash
Signed-off-by: zhouyangchao
---
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
[dpdk-dev] [PATCH] kni: unregister an unregisterd net_device could cause a kernel crash
Signed-off-by: zhouyangchao
---
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
[dpdk-dev] [PATCH] kni: unregister an unregisterd net_device could cause a kernel crash
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
[dpdk-dev] [PATCH] kni: unregister an unregisterd net_device could cause a kernel crash
On 9/9/2016 3:46 AM, zhouyangchao wrote:
> Signed-off-by: zhouyangchao
> ---
> 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);
> }
>
>
[dpdk-dev] [PATCH] kni: unregister an unregisterd net_device could cause a kernel crash
On Fri, 9 Sep 2016 10:42:16 +0800
zhouyangchao wrote:
> Signed-off-by: zhouyangchao
> ---
> 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.
[dpdk-dev] [PATCH] kni: unregister an unregisterd net_device could cause a kernel crash
On Fri, 9 Sep 2016 10:46:07 +0800
zhouyangchao wrote:
> Signed-off-by: zhouyangchao
> ---
> 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.
