From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ferruh Yigit Subject: Re: [PATCH] kni: unregister an unregisterd net_device could cause a kernel crash Date: Thu, 8 Sep 2016 18:07:08 +0100 Message-ID: References: <1473389167-2758-1-git-send-email-zhouyates@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Cc: dev@dpdk.org To: zhouyangchao Return-path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id BC0B45939 for ; Thu, 8 Sep 2016 19:07:10 +0200 (CEST) In-Reply-To: <1473389167-2758-1-git-send-email-zhouyates@gmail.com> List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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); > } > >