network device reference leak with net-next

network device reference leak with net-next

[Stephen Hemminger]

This is a new regression (doesn't exist with 2.6.36)

If I shutdown KVM instance with Virt manager, the virtual
interfaces in the bridge aren't getting cleaned up because
of leftover reference count.


[ 9781.050474] unregister_netdevice: waiting for vnet1 to become free. Usage count = 1
[ 9785.143400] virbr4: port 3(vnet6) entering forwarding state
[ 9785.177194] virbr4: port 3(vnet6) entering disabled state
[ 9785.201129] device vnet6 left promiscuous mode
[ 9785.201135] virbr4: port 3(vnet6) entering disabled state
[ 9791.286950] unregister_netdevice: waiting for vnet1 to become free. Usage count = 1
[ 9795.461526] unregister_netdevice: waiting for vnet6 to become free. Usage count = 1
[ 9801.523398] unregister_netdevice: waiting for vnet1 to become free. Usage count = 1

Re: network device reference leak with net-next

[Eric Dumazet]

Le lundi 15 novembre 2010 à 10:56 -0800, Stephen Hemminger a écrit :
> This is a new regression (doesn't exist with 2.6.36)
> 
> If I shutdown KVM instance with Virt manager, the virtual
> interfaces in the bridge aren't getting cleaned up because
> of leftover reference count.
> 
> 
> [ 9781.050474] unregister_netdevice: waiting for vnet1 to become free. Usage count = 1
> [ 9785.143400] virbr4: port 3(vnet6) entering forwarding state
> [ 9785.177194] virbr4: port 3(vnet6) entering disabled state
> [ 9785.201129] device vnet6 left promiscuous mode
> [ 9785.201135] virbr4: port 3(vnet6) entering disabled state
> [ 9791.286950] unregister_netdevice: waiting for vnet1 to become free. Usage count = 1
> [ 9795.461526] unregister_netdevice: waiting for vnet6 to become free. Usage count = 1
> [ 9801.523398] unregister_netdevice: waiting for vnet1 to become free. Usage count = 1
> --

Is the refcount stay forever to 1, or eventually reaches 0 ?

Re: network device reference leak with net-next

[Stephen Hemminger]

On Mon, 15 Nov 2010 20:04:00 +0100
Eric Dumazet <eric.dumazet@gmail.com> wrote:

> Le lundi 15 novembre 2010 à 10:56 -0800, Stephen Hemminger a écrit :
> > This is a new regression (doesn't exist with 2.6.36)
> > 
> > If I shutdown KVM instance with Virt manager, the virtual
> > interfaces in the bridge aren't getting cleaned up because
> > of leftover reference count.
> > 
> > 
> > [ 9781.050474] unregister_netdevice: waiting for vnet1 to become free. Usage count = 1
> > [ 9785.143400] virbr4: port 3(vnet6) entering forwarding state
> > [ 9785.177194] virbr4: port 3(vnet6) entering disabled state
> > [ 9785.201129] device vnet6 left promiscuous mode
> > [ 9785.201135] virbr4: port 3(vnet6) entering disabled state
> > [ 9791.286950] unregister_netdevice: waiting for vnet1 to become free. Usage count = 1
> > [ 9795.461526] unregister_netdevice: waiting for vnet6 to become free. Usage count = 1
> > [ 9801.523398] unregister_netdevice: waiting for vnet1 to become free. Usage count = 1
> > --
> 
> Is the refcount stay forever to 1, or eventually reaches 0 ?
> 
> 
> 

Stays 1 for as long as I waited about 10 minutes

-- 

Re: network device reference leak with net-next

[John Fastabend]

On 11/15/2010 11:10 AM, Stephen Hemminger wrote:
> On Mon, 15 Nov 2010 20:04:00 +0100
> Eric Dumazet <eric.dumazet@gmail.com> wrote:
> 
>> Le lundi 15 novembre 2010 à 10:56 -0800, Stephen Hemminger a écrit :
>>> This is a new regression (doesn't exist with 2.6.36)
>>>
>>> If I shutdown KVM instance with Virt manager, the virtual
>>> interfaces in the bridge aren't getting cleaned up because
>>> of leftover reference count.
>>>
>>>
>>> [ 9781.050474] unregister_netdevice: waiting for vnet1 to become free. Usage count = 1
>>> [ 9785.143400] virbr4: port 3(vnet6) entering forwarding state
>>> [ 9785.177194] virbr4: port 3(vnet6) entering disabled state
>>> [ 9785.201129] device vnet6 left promiscuous mode
>>> [ 9785.201135] virbr4: port 3(vnet6) entering disabled state
>>> [ 9791.286950] unregister_netdevice: waiting for vnet1 to become free. Usage count = 1
>>> [ 9795.461526] unregister_netdevice: waiting for vnet6 to become free. Usage count = 1
>>> [ 9801.523398] unregister_netdevice: waiting for vnet1 to become free. Usage count = 1
>>> --
>>
>> Is the refcount stay forever to 1, or eventually reaches 0 ?
>>
>>
>>
> 
> Stays 1 for as long as I waited about 10 minutes
> 

Similar refcount error with ixgbe bisected to this patch,

commit 2de795707294972f6c34bae9de713e502c431296
Author: Lorenzo Colitti <lorenzo@google.com>
Date:   Wed Oct 27 18:16:49 2010 +0000

    ipv6: addrconf: don't remove address state on ifdown if the address is being kept

    Currently, addrconf_ifdown does not delete statically configured IPv6
    addresses when the interface is brought down. The intent is that when
    the interface comes back up the address will be usable again. However,
    this doesn't actually work, because the system stops listening on the
    corresponding solicited-node multicast address, so the address cannot
    respond to neighbor solicitations and thus receive traffic. Also, the
    code notifies the rest of the system that the address is being deleted
    (e.g, RTM_DELADDR), even though it is not. Fix it so that none of this
    state is updated if the address is being kept on the interface.

    Tested: Added a statically configured IPv6 address to an interface,
    started ping, brought link down, brought link up again. When link came
    up ping kept on going and "ip -6 maddr" showed that the host was still
    subscribed to there

    Signed-off-by: Lorenzo Colitti <lorenzo@google.com>
    Signed-off-by: David S. Miller <davem@davemloft.net>


Quick glance looks like an in6_ifa_put is missed?

--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -2754,13 +2754,13 @@ Hunk #1, a/net/ipv6/addrconf.c static int addrconf_ifdown(struct net_device *dev, int how)
                        ifa->state = INET6_IFADDR_STATE_DEAD;
                        spin_unlock_bh(&ifa->state_lock);

-                       if (state == INET6_IFADDR_STATE_DEAD) {
-                               in6_ifa_put(ifa);
-                       } else {
+                       if (state != INET6_IFADDR_STATE_DEAD) {
                                __ipv6_ifa_notify(RTM_DELADDR, ifa);
                                atomic_notifier_call_chain(&inet6addr_chain,
                                                           NETDEV_DOWN, ifa);
                        }
+
+                       in6_ifa_put(ifa);
                        write_lock_bh(&idev->lock);
                }
        }

With a quick hack the above seems to resolve the issue but I'll need to review the ipv6 stuff to be sure this is sane.

Thanks,
John.

Re: network device reference leak with net-next

[Eric Dumazet]

Le lundi 15 novembre 2010 à 15:18 -0800, John Fastabend a écrit :
> On 11/15/2010 11:10 AM, Stephen Hemminger wrote:
> > On Mon, 15 Nov 2010 20:04:00 +0100
> > Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > 
> >> Le lundi 15 novembre 2010 à 10:56 -0800, Stephen Hemminger a écrit :
> >>> This is a new regression (doesn't exist with 2.6.36)
> >>>
> >>> If I shutdown KVM instance with Virt manager, the virtual
> >>> interfaces in the bridge aren't getting cleaned up because
> >>> of leftover reference count.
> >>>
> >>>
> >>> [ 9781.050474] unregister_netdevice: waiting for vnet1 to become free. Usage count = 1
> >>> [ 9785.143400] virbr4: port 3(vnet6) entering forwarding state
> >>> [ 9785.177194] virbr4: port 3(vnet6) entering disabled state
> >>> [ 9785.201129] device vnet6 left promiscuous mode
> >>> [ 9785.201135] virbr4: port 3(vnet6) entering disabled state
> >>> [ 9791.286950] unregister_netdevice: waiting for vnet1 to become free. Usage count = 1
> >>> [ 9795.461526] unregister_netdevice: waiting for vnet6 to become free. Usage count = 1
> >>> [ 9801.523398] unregister_netdevice: waiting for vnet1 to become free. Usage count = 1
> >>> --
> >>
> >> Is the refcount stay forever to 1, or eventually reaches 0 ?
> >>
> >>
> >>
> > 
> > Stays 1 for as long as I waited about 10 minutes
> > 
> 
> Similar refcount error with ixgbe bisected to this patch,
> 
> commit 2de795707294972f6c34bae9de713e502c431296
> Author: Lorenzo Colitti <lorenzo@google.com>
> Date:   Wed Oct 27 18:16:49 2010 +0000
> 
>     ipv6: addrconf: don't remove address state on ifdown if the address is being kept
> 
>     Currently, addrconf_ifdown does not delete statically configured IPv6
>     addresses when the interface is brought down. The intent is that when
>     the interface comes back up the address will be usable again. However,
>     this doesn't actually work, because the system stops listening on the
>     corresponding solicited-node multicast address, so the address cannot
>     respond to neighbor solicitations and thus receive traffic. Also, the
>     code notifies the rest of the system that the address is being deleted
>     (e.g, RTM_DELADDR), even though it is not. Fix it so that none of this
>     state is updated if the address is being kept on the interface.
> 
>     Tested: Added a statically configured IPv6 address to an interface,
>     started ping, brought link down, brought link up again. When link came
>     up ping kept on going and "ip -6 maddr" showed that the host was still
>     subscribed to there
> 
>     Signed-off-by: Lorenzo Colitti <lorenzo@google.com>
>     Signed-off-by: David S. Miller <davem@davemloft.net>
> 
> 
> Quick glance looks like an in6_ifa_put is missed?
> 
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -2754,13 +2754,13 @@ Hunk #1, a/net/ipv6/addrconf.c static int addrconf_ifdown(struct net_device *dev, int how)
>                         ifa->state = INET6_IFADDR_STATE_DEAD;
>                         spin_unlock_bh(&ifa->state_lock);
> 
> -                       if (state == INET6_IFADDR_STATE_DEAD) {
> -                               in6_ifa_put(ifa);
> -                       } else {
> +                       if (state != INET6_IFADDR_STATE_DEAD) {
>                                 __ipv6_ifa_notify(RTM_DELADDR, ifa);
>                                 atomic_notifier_call_chain(&inet6addr_chain,
>                                                            NETDEV_DOWN, ifa);
>                         }
> +
> +                       in6_ifa_put(ifa);
>                         write_lock_bh(&idev->lock);
>                 }
>         }
> 
> With a quick hack the above seems to resolve the issue but I'll need to review the ipv6 stuff to be sure this is sane.
> 
> Thanks,
> John.

Your machine is faster than mine ;)

Yes this commit seems buggy, thanks for finding the problem !

Re: network device reference leak with net-next

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Tue, 16 Nov 2010 00:25:43 +0100

> Le lundi 15 novembre 2010 à 15:18 -0800, John Fastabend a écrit :
>> With a quick hack the above seems to resolve the issue but I'll need to review the ipv6 stuff to be sure this is sane.
>> 
>> Thanks,
>> John.
> 
> Your machine is faster than mine ;)
> 
> Yes this commit seems buggy, thanks for finding the problem !

Indeed, thanks John.  Please send a final fix after you've done
your review.

Thanks again!

Re: network device reference leak with net-next

[Lorenzo Colitti]

On Tue, Nov 16, 2010 at 12:18 AM, John Fastabend
<john.r.fastabend@intel.com> wrote:
> Quick glance looks like an in6_ifa_put is missed?

Oops. Sorry about that.

I'm testing your fix, it seems to work so far.