[PATCH] Unmatched decrementing of net device reference count

[PATCH] Unmatched decrementing of net device reference count

[Glauber de Oliveira Costa]

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

Hello,

This bug was found when heavy stressing the netfront 
attach/detach mechanism with the following script:
 
   for i in $(seq 200); 
   do 
     xm network-attach <domid>;  
     xm network-detach <domid> $i;
   done

Guest kernel shows the following messages:

unregister_netdevice: waiting for eth1 to become free. Usage count = -1

After this patch, it ran okay in multiple iterations

-- 
Glauber de Oliveira Costa
Red Hat Inc.
"Free as in Freedom"

[-- Attachment #2: xen-netfront-refcnt.patch --]
[-- Type: text/plain, Size: 1505 bytes --]

# HG changeset patch
# User gcosta@redhat.com
# Date 1166523892 7200
# Node ID 815165a1368b7e8292a2ef390b43a1cde7f0b1b0
# Parent  b3cd9c4ac0561a89e13c7c74a9dc990d102a6080
[LINUX] Avoid reference counter going below zero.

After calling unregister_netdev, the device may be freed by
the underlying layer. There can be situations in which the free'd
address was already taken by another netdev, and between the two
events, a linkwatch event was fired, putting the device in the
linkwatch event list.

When the network device layer goes through its event list, it finds
the older reference, and calls dev_put() one extra time. Generally
speaking, it is not safe to call unregister_netdev when we still plan
work to be done, so we should call it after netif_disconnect_backend().

Signed-off-by: Glauber de Oliveira Costa <gcosta@redhat.com>

diff -r b3cd9c4ac056 -r 815165a1368b linux-2.6-xen-sparse/drivers/xen/netfront/netfront.c
--- a/linux-2.6-xen-sparse/drivers/xen/netfront/netfront.c	Tue Dec 19 10:53:17 2006 +0000
+++ b/linux-2.6-xen-sparse/drivers/xen/netfront/netfront.c	Tue Dec 19 08:24:52 2006 -0200
@@ -2022,6 +2022,7 @@ static int __devexit netfront_remove(str
 	DPRINTK("%s\n", dev->nodename);
 
 	netif_disconnect_backend(info);
+	unregister_netdev(info->netdev);
 	free_netdev(info->netdev);
 
 	return 0;
@@ -2055,7 +2056,6 @@ static void close_netdev(struct netfront
 	del_timer_sync(&info->rx_refill_timer);
 
 	xennet_sysfs_delif(info->netdev);
-	unregister_netdev(info->netdev);
 }
 
 

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH] Unmatched decrementing of net device reference count

[Herbert Xu]

Glauber de Oliveira Costa <gcosta@redhat.com> wrote:
> 
> This bug was found when heavy stressing the netfront 
> attach/detach mechanism with the following script:
> 
>   for i in $(seq 200); 
>   do 
>     xm network-attach <domid>;  
>     xm network-detach <domid> $i;
>   done
> 
> Guest kernel shows the following messages:
> 
> unregister_netdevice: waiting for eth1 to become free. Usage count = -1
> 
> After this patch, it ran okay in multiple iterations

Could you please use in-line patches? It's much easier to comment on.

Your patch description doesn't make sense.  unregister_netdev()
cannot possibly cause the device to be freed.  Otherwise the
subsequent free_netdev() call which you kept would be wrong.

So most likely what's happening is that free_netdev() is occuring
without a preceding unregister_netdev(), which implies that there
is a bug in the frontend state transition.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] Unmatched decrementing of net device reference count

[Glauber de Oliveira Costa]

On Wed, Dec 20, 2006 at 10:58:22AM +1100, Herbert Xu wrote:
> Glauber de Oliveira Costa <gcosta@redhat.com> wrote:
> > 
> > This bug was found when heavy stressing the netfront 
> > attach/detach mechanism with the following script:
> > 
> >   for i in $(seq 200); 
> >   do 
> >     xm network-attach <domid>;  
> >     xm network-detach <domid> $i;
> >   done
> > 
> > Guest kernel shows the following messages:
> > 
> > unregister_netdevice: waiting for eth1 to become free. Usage count = -1
> > 
> > After this patch, it ran okay in multiple iterations
> 
> Could you please use in-line patches? It's much easier to comment on.
It is. I could swear I inlined it, but maybe I forgot.
 
> Your patch description doesn't make sense.  unregister_netdev()
> cannot possibly cause the device to be freed.  Otherwise the
> subsequent free_netdev() call which you kept would be wrong.

In fact. I read it again, and it was confusing (I myself was confused).

I'll try to rephrase: ( I digged more, cleared things up, and it'll be
more precise now)

unregister_netdev() works as a barrier in this case. The call to
netif_disconnect_backend() introduces a new carrier watch, which hold()s a
reference to be put()'d in a future time. If we call free right after that, 
it might be the case that put() is called after free. Nothing in this
case prevents this memory region to have been allocated again to another
device. 

unregister_netdev() holds the rntl lock. It means that when the lock is
released, netdev_run_todo() (which is setup by unregister_netdev()
itself, with net_set_todo() ), will call netdev_wait_allrefs(), which 
takes care of the linkwatch_runqueue. Calling unregister_netdev()
between the carrier watch and free_netdev() guarantees that the device
will be only free'd when the watches were already handled.

There would most probably be other ways to guarantee that, such as,
calling linkwatch_runqueue() directly. But I think that we lose nothing
by calling unregister_netdev() in the middle, and gain serialization for
free. 


> So most likely what's happening is that free_netdev() is occuring
> without a preceding unregister_netdev(), which implies that there
> is a bug in the frontend state transition.

It is not the case, see above.
 
-- 
Glauber de Oliveira Costa
Red Hat Inc.
"Free as in Freedom"

Re: [PATCH] Unmatched decrementing of net device reference count

[Herbert Xu]

On Wed, Dec 20, 2006 at 11:21:51AM -0200, Glauber de Oliveira Costa wrote:
> 
> unregister_netdev() works as a barrier in this case. The call to
> netif_disconnect_backend() introduces a new carrier watch, which hold()s a
> reference to be put()'d in a future time. If we call free right after that, 
> it might be the case that put() is called after free. Nothing in this
> case prevents this memory region to have been allocated again to another
> device. 

Thanks for the explanation.  I understand the problem now.  However,
I think your patch isn't adequate because the closing of the backend
no longer shuts down the transmitter in the frontend.

Looking at this again it comes down to an asymmetry in the setup
and tear-down processes.  On startup, we have two stages:

1) netfront_probe => create_netdev
		  => open_netdev => register_netdev;
2) network_connect => sets up IRQ/ring buffer/etc.

On tear-down, things occur in the wrong order:

1) netfront_closing => close_netdev => unregister_netdev;
2) netfront_remove => kills IRQ/ring buffer and free_netdev.

The tear-down order should be the opposite of the setup, i.e.,

1) netfront_closing => kills IRQ/ring buffer;
2) netfront_remove => close_netdev => unregister_netdev
		   => free_netdev.

So I suggest we move the netif_disconnct_backend call to
netfront_closing and close_netdev to netfront_remove.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] Unmatched decrementing of net device reference count

[Keir Fraser]

On 21/12/06 08:49, "Herbert Xu" <herbert.xu@redhat.com> wrote:

> Thanks for the explanation.  I understand the problem now.  However,
> I think your patch isn't adequate because the closing of the backend
> no longer shuts down the transmitter in the frontend.

Changeset 13100:e99ba0c6c is what I checked in to fix this issue. I think it
goes far enough, but please do take a look and check.

 -- Keir

Re: [PATCH] Unmatched decrementing of net device reference count

[Herbert Xu]

On Thu, Dec 21, 2006 at 11:03:50AM +0000, Keir Fraser wrote:
> On 21/12/06 08:49, "Herbert Xu" <herbert.xu@redhat.com> wrote:
> 
> > Thanks for the explanation.  I understand the problem now.  However,
> > I think your patch isn't adequate because the closing of the backend
> > no longer shuts down the transmitter in the frontend.
> 
> Changeset 13100:e99ba0c6c is what I checked in to fix this issue. I think it
> goes far enough, but please do take a look and check.

Yes it's OK for now because the frontend doesn't hold any resources
from the backend.  So even if the frontend tries to xmit after the
backend starts closing, it won't cause any problems.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] Unmatched decrementing of net device reference count

[Glauber de Oliveira Costa]

On Thu, Dec 21, 2006 at 11:40:24PM +1100, Herbert Xu wrote:
> On Thu, Dec 21, 2006 at 11:03:50AM +0000, Keir Fraser wrote:
> > On 21/12/06 08:49, "Herbert Xu" <herbert.xu@redhat.com> wrote:
> > 
> > > Thanks for the explanation.  I understand the problem now.  However,
> > > I think your patch isn't adequate because the closing of the backend
> > > no longer shuts down the transmitter in the frontend.
> > 
> > Changeset 13100:e99ba0c6c is what I checked in to fix this issue. I think it
> > goes far enough, but please do take a look and check.
> 
> Yes it's OK for now because the frontend doesn't hold any resources
> from the backend.  So even if the frontend tries to xmit after the
> backend starts closing, it won't cause any problems.
 
It indeed looks sane, and passes my tests.

Thanks!

-- 
Glauber de Oliveira Costa
Red Hat Inc.
"Free as in Freedom"