[PATCH] Fix panic in virtnet_remove

[PATCH] Fix panic in virtnet_remove

[Krishna Kumar]

modprobe -r virtio_net panics in free_netdev() as the
dev is already freed in the newly introduced virtnet_free
(commit 3fa2a1df9094). Since virtnet_remove doesn't require
dev after unregister, I am removing the free_netdev call
in virtnet_remove instead of in virtnet_free (which seems
to be the right place to free the dev). Confirmed that
the panic is fixed with this patch.

Signed-off-by: Krishna Kumar <krkumar2@in.ibm.com>
---
 drivers/net/virtio_net.c |    2 --
 1 file changed, 2 deletions(-)

diff -ruNp org/drivers/net/virtio_net.c new/drivers/net/virtio_net.c
--- org/drivers/net/virtio_net.c	2011-07-04 10:38:33.000000000 +0530
+++ new/drivers/net/virtio_net.c	2011-07-15 14:27:48.000000000 +0530
@@ -1121,8 +1121,6 @@ static void __devexit virtnet_remove(str
 
 	while (vi->pages)
 		__free_pages(get_a_page(vi, GFP_KERNEL), 0);
-
-	free_netdev(vi->dev);
 }
 
 static struct virtio_device_id id_table[] = {

Re: [PATCH] Fix panic in virtnet_remove

[David Miller]

From: Krishna Kumar <krkumar2@in.ibm.com>
Date: Fri, 15 Jul 2011 14:46:50 +0530

> modprobe -r virtio_net panics in free_netdev() as the
> dev is already freed in the newly introduced virtnet_free
> (commit 3fa2a1df9094). Since virtnet_remove doesn't require
> dev after unregister, I am removing the free_netdev call
> in virtnet_remove instead of in virtnet_free (which seems
> to be the right place to free the dev). Confirmed that
> the panic is fixed with this patch.
> 
> Signed-off-by: Krishna Kumar <krkumar2@in.ibm.com>

I guess the virtio NET maintainers don't deserve to get CC:'d on a fix
like this? :-(

Michael Tsirkin is who maintains this driver actively and is the one
who will merge this kind of fix to me, therefore if you don't CC:
him it might not get integrated at all.

Re: [PATCH] Fix panic in virtnet_remove

[Krishna Kumar2]

[-- Attachment #1: Type: text/plain, Size: 1034 bytes --]

David Miller <davem@davemloft.net> wrote on 07/15/2011 08:49:09 PM:

> > modprobe -r virtio_net panics in free_netdev() as the
> > dev is already freed in the newly introduced virtnet_free
> > (commit 3fa2a1df9094). Since virtnet_remove doesn't require
> > dev after unregister, I am removing the free_netdev call
> > in virtnet_remove instead of in virtnet_free (which seems
> > to be the right place to free the dev). Confirmed that
> > the panic is fixed with this patch.
> >
> > Signed-off-by: Krishna Kumar <krkumar2@in.ibm.com>
>
> I guess the virtio NET maintainers don't deserve to get CC:'d on a fix
> like this? :-(
>
> Michael Tsirkin is who maintains this driver actively and is the one
> who will merge this kind of fix to me, therefore if you don't CC:
> him it might not get integrated at all.

Sorry, but it was not intentional. I saw that Stephen had
made that change so cc'd him instead.

Michael, I am attaching the patch below, please let me know if
you need it inlined:


(See attached file: patch)

Thanks,

- KK

[-- Attachment #2: patch --]
[-- Type: application/octet-stream, Size: 921 bytes --]

modprobe -r virtio_net panics in free_netdev() as the
dev is already freed in the newly introduced virtnet_free
(commit 3fa2a1df9094). Since virtnet_remove doesn't require
dev after unregister, I am removing the free_netdev call
in virtnet_remove instead of in virtnet_free (which seems
to be the right place to free the dev). Confirmed that
the panic is fixed with this patch.

Signed-off-by: Krishna Kumar <krkumar2@in.ibm.com>
---
 drivers/net/virtio_net.c |    2 --
 1 file changed, 2 deletions(-)

diff -ruNp org/drivers/net/virtio_net.c new/drivers/net/virtio_net.c
--- org/drivers/net/virtio_net.c	2011-07-04 10:38:33.000000000 +0530
+++ new/drivers/net/virtio_net.c	2011-07-15 14:27:48.000000000 +0530
@@ -1121,8 +1121,6 @@ static void __devexit virtnet_remove(str
 
 	while (vi->pages)
 		__free_pages(get_a_page(vi, GFP_KERNEL), 0);
-
-	free_netdev(vi->dev);
 }
 
 static struct virtio_device_id id_table[] = {

Re: [PATCH] Fix panic in virtnet_remove

[Michael S. Tsirkin]

On Fri, Jul 15, 2011 at 02:46:50PM +0530, Krishna Kumar wrote:
> modprobe -r virtio_net panics in free_netdev() as the
> dev is already freed in the newly introduced virtnet_free
> (commit 3fa2a1df9094).

Good catch, thanks!

> Since virtnet_remove doesn't require
> dev after unregister,

I'm not sure that true: vi used just above the line you remove
is I think the struct virtnet_info that is allocated
as part of that structure.


> I am removing the free_netdev call
> in virtnet_remove instead of in virtnet_free (which seems
> to be the right place to free the dev). Confirmed that
> the panic is fixed with this patch.

This might be just because the memory isn't reused.
Try enabling slab poisoning, I think you'll observe some problems.

Do we absolutely have to have a destructor?
Can't we move the per cpu counter free from
virtnet_free to virtnet_remove, and get rid of
virtnet_free completely?


> Signed-off-by: Krishna Kumar <krkumar2@in.ibm.com>
> ---
>  drivers/net/virtio_net.c |    2 --
>  1 file changed, 2 deletions(-)
> 
> diff -ruNp org/drivers/net/virtio_net.c new/drivers/net/virtio_net.c
> --- org/drivers/net/virtio_net.c	2011-07-04 10:38:33.000000000 +0530
> +++ new/drivers/net/virtio_net.c	2011-07-15 14:27:48.000000000 +0530
> @@ -1121,8 +1121,6 @@ static void __devexit virtnet_remove(str
>  
>  	while (vi->pages)
>  		__free_pages(get_a_page(vi, GFP_KERNEL), 0);
> -
> -	free_netdev(vi->dev);
>  }
>  
>  static struct virtio_device_id id_table[] = {
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Fix panic in virtnet_remove

[Krishna Kumar]

"Michael S. Tsirkin" <mst@redhat.com> wrote on 07/17/2011 03:42:15 PM:

> > modprobe -r virtio_net panics in free_netdev() as the
> > dev is already freed in the newly introduced virtnet_free
> > (commit 3fa2a1df9094).
> 
> Good catch, thanks!
> 
> > Since virtnet_remove doesn't require
> > dev after unregister,
> 
> I'm not sure that true: vi used just above the line you remove
> is I think the struct virtnet_info that is allocated
> as part of that structure.

You are right, the dev cannot be freed in the destructor.

> > I am removing the free_netdev call
> > in virtnet_remove instead of in virtnet_free (which seems
> > to be the right place to free the dev). Confirmed that
> > the panic is fixed with this patch.
> 
> This might be just because the memory isn't reused.
> Try enabling slab poisoning, I think you'll observe some problems.
> 
> Do we absolutely have to have a destructor?
> Can't we move the per cpu counter free from
> virtnet_free to virtnet_remove, and get rid of
> virtnet_free completely?

I see some other drivers doing that, e.g. xennet_remove:
	...
	free_percpu(info->stats);
	free_netdev(info->netdev);
	...

How about this patch (compile tested only)?

Signed-off-by: Krishna Kumar <krkumar2@in.ibm.com>
---
 drivers/net/virtio_net.c |   10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff -ruNp org/drivers/net/virtio_net.c new/drivers/net/virtio_net.c
--- org/drivers/net/virtio_net.c	2011-07-18 09:14:02.000000000 +0530
+++ new/drivers/net/virtio_net.c	2011-07-18 09:16:35.000000000 +0530
@@ -705,14 +705,6 @@ static void virtnet_netpoll(struct net_d
 }
 #endif
 
-static void virtnet_free(struct net_device *dev)
-{
-	struct virtnet_info *vi = netdev_priv(dev);
-
-	free_percpu(vi->stats);
-	free_netdev(dev);
-}
-
 static int virtnet_open(struct net_device *dev)
 {
 	struct virtnet_info *vi = netdev_priv(dev);
@@ -959,7 +951,6 @@ static int virtnet_probe(struct virtio_d
 	/* Set up network device as normal. */
 	dev->netdev_ops = &virtnet_netdev;
 	dev->features = NETIF_F_HIGHDMA;
-	dev->destructor = virtnet_free;
 
 	SET_ETHTOOL_OPS(dev, &virtnet_ethtool_ops);
 	SET_NETDEV_DEV(dev, &vdev->dev);
@@ -1122,6 +1113,7 @@ static void __devexit virtnet_remove(str
 	while (vi->pages)
 		__free_pages(get_a_page(vi, GFP_KERNEL), 0);
 
+	free_percpu(vi->stats);
 	free_netdev(vi->dev);
 }
 

Re: [PATCH] Fix panic in virtnet_remove

[Michael S. Tsirkin]

On Mon, Jul 18, 2011 at 09:27:30AM +0530, Krishna Kumar wrote:
> "Michael S. Tsirkin" <mst@redhat.com> wrote on 07/17/2011 03:42:15 PM:
> 
> > > modprobe -r virtio_net panics in free_netdev() as the
> > > dev is already freed in the newly introduced virtnet_free
> > > (commit 3fa2a1df9094).
> > 
> > Good catch, thanks!
> > 
> > > Since virtnet_remove doesn't require
> > > dev after unregister,
> > 
> > I'm not sure that true: vi used just above the line you remove
> > is I think the struct virtnet_info that is allocated
> > as part of that structure.
> 
> You are right, the dev cannot be freed in the destructor.
> 
> > > I am removing the free_netdev call
> > > in virtnet_remove instead of in virtnet_free (which seems
> > > to be the right place to free the dev). Confirmed that
> > > the panic is fixed with this patch.
> > 
> > This might be just because the memory isn't reused.
> > Try enabling slab poisoning, I think you'll observe some problems.
> > 
> > Do we absolutely have to have a destructor?
> > Can't we move the per cpu counter free from
> > virtnet_free to virtnet_remove, and get rid of
> > virtnet_free completely?
> 
> I see some other drivers doing that, e.g. xennet_remove:
> 	...
> 	free_percpu(info->stats);
> 	free_netdev(info->netdev);
> 	...
> 
> How about this patch (compile tested only)?
> 
> Signed-off-by: Krishna Kumar <krkumar2@in.ibm.com>

This is what I had in mind. Pls test it and if OK resubmit
with proper description etc.


> ---
>  drivers/net/virtio_net.c |   10 +---------
>  1 file changed, 1 insertion(+), 9 deletions(-)
> 
> diff -ruNp org/drivers/net/virtio_net.c new/drivers/net/virtio_net.c
> --- org/drivers/net/virtio_net.c	2011-07-18 09:14:02.000000000 +0530
> +++ new/drivers/net/virtio_net.c	2011-07-18 09:16:35.000000000 +0530
> @@ -705,14 +705,6 @@ static void virtnet_netpoll(struct net_d
>  }
>  #endif
>  
> -static void virtnet_free(struct net_device *dev)
> -{
> -	struct virtnet_info *vi = netdev_priv(dev);
> -
> -	free_percpu(vi->stats);
> -	free_netdev(dev);
> -}
> -
>  static int virtnet_open(struct net_device *dev)
>  {
>  	struct virtnet_info *vi = netdev_priv(dev);
> @@ -959,7 +951,6 @@ static int virtnet_probe(struct virtio_d
>  	/* Set up network device as normal. */
>  	dev->netdev_ops = &virtnet_netdev;
>  	dev->features = NETIF_F_HIGHDMA;
> -	dev->destructor = virtnet_free;
>  
>  	SET_ETHTOOL_OPS(dev, &virtnet_ethtool_ops);
>  	SET_NETDEV_DEV(dev, &vdev->dev);
> @@ -1122,6 +1113,7 @@ static void __devexit virtnet_remove(str
>  	while (vi->pages)
>  		__free_pages(get_a_page(vi, GFP_KERNEL), 0);
>  
> +	free_percpu(vi->stats);
>  	free_netdev(vi->dev);
>  }
>  

[PATCH] Fix panic in virtnet_remove

[Krishna Kumar]

Fix a panic in virtnet_remove. unregister_netdev has already
freed up the netdev (and virtnet_info) due to dev->destructor
being set, while virtnet_info is still required. Remove
virtnet_free altogether, and move the freeing of the per-cpu
statistics from virtnet_free to virtnet_remove.

Tested patch below.

Signed-off-by: Krishna Kumar <krkumar2@in.ibm.com>
---
 drivers/net/virtio_net.c |   10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff -ruNp org/drivers/net/virtio_net.c new/drivers/net/virtio_net.c
--- org/drivers/net/virtio_net.c	2011-07-18 09:14:02.000000000 +0530
+++ new/drivers/net/virtio_net.c	2011-07-18 09:16:35.000000000 +0530
@@ -705,14 +705,6 @@ static void virtnet_netpoll(struct net_d
 }
 #endif
 
-static void virtnet_free(struct net_device *dev)
-{
-	struct virtnet_info *vi = netdev_priv(dev);
-
-	free_percpu(vi->stats);
-	free_netdev(dev);
-}
-
 static int virtnet_open(struct net_device *dev)
 {
 	struct virtnet_info *vi = netdev_priv(dev);
@@ -959,7 +951,6 @@ static int virtnet_probe(struct virtio_d
 	/* Set up network device as normal. */
 	dev->netdev_ops = &virtnet_netdev;
 	dev->features = NETIF_F_HIGHDMA;
-	dev->destructor = virtnet_free;
 
 	SET_ETHTOOL_OPS(dev, &virtnet_ethtool_ops);
 	SET_NETDEV_DEV(dev, &vdev->dev);
@@ -1122,6 +1113,7 @@ static void __devexit virtnet_remove(str
 	while (vi->pages)
 		__free_pages(get_a_page(vi, GFP_KERNEL), 0);
 
+	free_percpu(vi->stats);
 	free_netdev(vi->dev);
 }
 

Re: [PATCH] Fix panic in virtnet_remove

[Stephen Hemminger]

On Wed, 20 Jul 2011 13:13:39 +0530
Krishna Kumar <krkumar2@in.ibm.com> wrote:

> Fix a panic in virtnet_remove. unregister_netdev has already
> freed up the netdev (and virtnet_info) due to dev->destructor
> being set, while virtnet_info is still required. Remove
> virtnet_free altogether, and move the freeing of the per-cpu
> statistics from virtnet_free to virtnet_remove.
> 
> Tested patch below.
> 
> Signed-off-by: Krishna Kumar <krkumar2@in.ibm.com>

Need to check veth and xen for same problem.

Acked-by: Stephen Hemminger <shemminger@vyatta.com>