[PATCH] ipv6: slightly simplify keeping IPv6 addresses on link down

[PATCH] ipv6: slightly simplify keeping IPv6 addresses on link down

[Lorenzo Colitti]

ipv6: slightly simplify keeping IPv6 addresses on link down

When link goes down, all statically-configured (i.e.,
permanent and not link-local) IPv6 addresses are kept on
the interface. Instead of moving addresses to a temporary
keep list and then splicing that back on to the interface
address list, use list_for_each_entry_safe and delete the
ones we don't want.

Tested by configuring two static addresses on an interface
and verifying that pings from the addresses keep working
when bringing link down and up again and when disabling and
re-enabling IPv6 on the interface.

Signed-off-by: Lorenzo Colitti <lorenzo@google.com>

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 23cc8e1..6dfd5c5 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -2663,8 +2663,7 @@ static int addrconf_ifdown(struct net_device
*dev, int how)
 {
 	struct net *net = dev_net(dev);
 	struct inet6_dev *idev;
-	struct inet6_ifaddr *ifa;
-	LIST_HEAD(keep_list);
+	struct inet6_ifaddr *ifa, *ifn;
 	int state;

 	ASSERT_RTNL();
@@ -2719,9 +2718,7 @@ static int addrconf_ifdown(struct net_device
*dev, int how)
 	}
 #endif

-	while (!list_empty(&idev->addr_list)) {
-		ifa = list_first_entry(&idev->addr_list,
-				       struct inet6_ifaddr, if_list);
+	list_for_each_entry_safe(ifa, ifn, &idev->addr_list, if_list) {
 		addrconf_del_timer(ifa);

 		/* If just doing link down, and address is permanent
@@ -2729,15 +2726,13 @@ static int addrconf_ifdown(struct net_device
*dev, int how)
 		if (!how &&
 		    (ifa->flags&IFA_F_PERMANENT) &&
 		    !(ipv6_addr_type(&ifa->addr) & IPV6_ADDR_LINKLOCAL)) {
-			list_move_tail(&ifa->if_list, &keep_list);
-
 			/* If not doing DAD on this address, just keep it. */
 			if ((dev->flags&(IFF_NOARP|IFF_LOOPBACK)) ||
 			    idev->cnf.accept_dad <= 0 ||
 			    (ifa->flags & IFA_F_NODAD))
 				continue;

-			/* If it was tentative already, no need to notify */
+			/* If it was tentative already, no need to do anything */
 			if (ifa->flags & IFA_F_TENTATIVE)
 				continue;

@@ -2769,8 +2764,6 @@ static int addrconf_ifdown(struct net_device
*dev, int how)
 		}
 	}

-	list_splice(&keep_list, &idev->addr_list);
-
 	write_unlock_bh(&idev->lock);

 	/* Step 5: Discard multicast list */

Re: [PATCH] ipv6: slightly simplify keeping IPv6 addresses on link down

[David Miller]

From: Lorenzo Colitti <lorenzo@google.com>
Date: Fri, 26 Nov 2010 10:06:58 -0800

> @@ -2663,8 +2663,7 @@ static int addrconf_ifdown(struct net_device
> *dev, int how)

Please fix your email client to not corrupt patches.

See Documentation/email-clients.txt

Re: [PATCH] ipv6: slightly simplify keeping IPv6 addresses on link down

[Lorenzo Colitti]

ipv6: slightly simplify keeping IPv6 addresses on link down

When link goes down, all statically-configured (i.e.,
permanent and not link-local) IPv6 addresses are kept on
the interface. Instead of moving addresses to a temporary
keep list and then splicing that back on to the interface
address list, use list_for_each_entry_safe and delete the
ones we don't want.

Tested by configuring two static addresses on an interface
and verifying that pings from the addresses keep working
when bringing link down and up again and when disabling and
re-enabling IPv6 on the interface.

Signed-off-by: Lorenzo Colitti <lorenzo@google.com>

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 6dfd5c5..23cc8e1 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -2663,7 +2663,8 @@ static int addrconf_ifdown(struct net_device *dev, int how)
 {
 	struct net *net = dev_net(dev);
 	struct inet6_dev *idev;
-	struct inet6_ifaddr *ifa, *ifn;
+	struct inet6_ifaddr *ifa;
+	LIST_HEAD(keep_list);
 	int state;
 
 	ASSERT_RTNL();
@@ -2718,7 +2719,9 @@ static int addrconf_ifdown(struct net_device *dev, int how)
 	}
 #endif
 
-	list_for_each_entry_safe(ifa, ifn, &idev->addr_list, if_list) {
+	while (!list_empty(&idev->addr_list)) {
+		ifa = list_first_entry(&idev->addr_list,
+				       struct inet6_ifaddr, if_list);
 		addrconf_del_timer(ifa);
 
 		/* If just doing link down, and address is permanent
@@ -2726,13 +2729,15 @@ static int addrconf_ifdown(struct net_device *dev, int how)
 		if (!how &&
 		    (ifa->flags&IFA_F_PERMANENT) &&
 		    !(ipv6_addr_type(&ifa->addr) & IPV6_ADDR_LINKLOCAL)) {
+			list_move_tail(&ifa->if_list, &keep_list);
+
 			/* If not doing DAD on this address, just keep it. */
 			if ((dev->flags&(IFF_NOARP|IFF_LOOPBACK)) ||
 			    idev->cnf.accept_dad <= 0 ||
 			    (ifa->flags & IFA_F_NODAD))
 				continue;
 
-			/* If it was tentative already, don't do anything */
+			/* If it was tentative already, no need to notify */
 			if (ifa->flags & IFA_F_TENTATIVE)
 				continue;
 
@@ -2764,6 +2769,8 @@ static int addrconf_ifdown(struct net_device *dev, int how)
 		}
 	}
 
+	list_splice(&keep_list, &idev->addr_list);
+
 	write_unlock_bh(&idev->lock);
 
 	/* Step 5: Discard multicast list */

Re: [PATCH] ipv6: slightly simplify keeping IPv6 addresses on link down

[Stephen Hemminger]

On Wed, 01 Dec 2010 11:38:58 -0800
Lorenzo Colitti <lorenzo@google.com> wrote:

> ipv6: slightly simplify keeping IPv6 addresses on link down
> 
> When link goes down, all statically-configured (i.e.,
> permanent and not link-local) IPv6 addresses are kept on
> the interface. Instead of moving addresses to a temporary
> keep list and then splicing that back on to the interface
> address list, use list_for_each_entry_safe and delete the
> ones we don't want.
> 
> Tested by configuring two static addresses on an interface
> and verifying that pings from the addresses keep working
> when bringing link down and up again and when disabling and
> re-enabling IPv6 on the interface.
> 
> Signed-off-by: Lorenzo Colitti <lorenzo@google.com>
> 
> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index 6dfd5c5..23cc8e1 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -2663,7 +2663,8 @@ static int addrconf_ifdown(struct net_device *dev, int how)
>  {
>  	struct net *net = dev_net(dev);
>  	struct inet6_dev *idev;
> -	struct inet6_ifaddr *ifa, *ifn;
> +	struct inet6_ifaddr *ifa;
> +	LIST_HEAD(keep_list);
>  	int state;

Your patch is backwards? The existing code is:

static int addrconf_ifdown(struct net_device *dev, int how)
{
	struct net *net = dev_net(dev);
	struct inet6_dev *idev;
	struct inet6_ifaddr *ifa;
	LIST_HEAD(keep_list);
	int state;

	ASSERT_RTNL();

Also, the addrconf_ifdown can race with other updates to idev->addr_list
from addrconf timers etc.  Therefore even list_for_each_entry_safe is not safe.

Re: [PATCH] ipv6: slightly simplify keeping IPv6 addresses on link down

[Lorenzo Colitti]

On Wed, Dec 1, 2010 at 12:22 PM, Stephen Hemminger
<shemminger@vyatta.com> wrote:
> > --- a/net/ipv6/addrconf.c
> > +++ b/net/ipv6/addrconf.c
> > @@ -2663,7 +2663,8 @@ static int addrconf_ifdown(struct net_device *dev, int how)
> >  {
> >       struct net *net = dev_net(dev);
> >       struct inet6_dev *idev;
> > -     struct inet6_ifaddr *ifa, *ifn;
> > +     struct inet6_ifaddr *ifa;
> > +     LIST_HEAD(keep_list);
> >       int state;
>
> Your patch is backwards? The existing code is:

Oops, yes. Wrong order of arguments. Another one coming up.

> Also, the addrconf_ifdown can race with other updates to idev->addr_list
> from addrconf timers etc.  Therefore even list_for_each_entry_safe is not safe.

No, wait... The loop is protected by idev->lock, and the code just
before it that clears the temporary address list is essentially
identical (except it looks over tempaddr_list instead). Wouldn't that
blow up as well?

Re: [PATCH] ipv6: slightly simplify keeping IPv6 addresses on link down

[Stephen Hemminger]

On Wed, 1 Dec 2010 12:52:42 -0800
Lorenzo Colitti <lorenzo@google.com> wrote:

> On Wed, Dec 1, 2010 at 12:22 PM, Stephen Hemminger
> <shemminger@vyatta.com> wrote:
> > > --- a/net/ipv6/addrconf.c
> > > +++ b/net/ipv6/addrconf.c
> > > @@ -2663,7 +2663,8 @@ static int addrconf_ifdown(struct net_device *dev, int how)
> > >  {
> > >       struct net *net = dev_net(dev);
> > >       struct inet6_dev *idev;
> > > -     struct inet6_ifaddr *ifa, *ifn;
> > > +     struct inet6_ifaddr *ifa;
> > > +     LIST_HEAD(keep_list);
> > >       int state;
> >
> > Your patch is backwards? The existing code is:
> 
> Oops, yes. Wrong order of arguments. Another one coming up.
> 
> > Also, the addrconf_ifdown can race with other updates to idev->addr_list
> > from addrconf timers etc.  Therefore even list_for_each_entry_safe is not safe.
> 
> No, wait... The loop is protected by idev->lock, and the code just
> before it that clears the temporary address list is essentially
> identical (except it looks over tempaddr_list instead). Wouldn't that
> blow up as well?

The old code walked the list until it was empty. New code could
get confused if list changed by other changes during the
period when idev->lock is dropped and notifier is called.



-- 

Re: [PATCH] ipv6: slightly simplify keeping IPv6 addresses on link down

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Wed, 1 Dec 2010 13:04:21 -0800

> On Wed, 1 Dec 2010 12:52:42 -0800
> Lorenzo Colitti <lorenzo@google.com> wrote:
> 
>> No, wait... The loop is protected by idev->lock, and the code just
>> before it that clears the temporary address list is essentially
>> identical (except it looks over tempaddr_list instead). Wouldn't that
>> blow up as well?
> 
> The old code walked the list until it was empty. New code could
> get confused if list changed by other changes during the
> period when idev->lock is dropped and notifier is called.

I think Stephen has a point here.

You can't use a "_safe" list traversal if you are dropping the
lock in the middle of that traversal, which is what you code
will be doing.

Lorenzo you've already added serious bugs to this code with your
previous address handling changes (which had reference counting bugs),
so I'm going to heavily scrutinize any "cleanup" or other kind of
change you try to make here.

In fact I really wish you would just leave this code as-is instead
of trying to make such pointless tweaks to it.

Thanks.

Re: [PATCH] ipv6: slightly simplify keeping IPv6 addresses on link down

[Lorenzo Colitti]

On Fri, Dec 10, 2010 at 12:43 PM, David Miller <davem@davemloft.net> wrote:
> Lorenzo you've already added serious bugs to this code with your
> previous address handling changes (which had reference counting bugs),
> so I'm going to heavily scrutinize any "cleanup" or other kind of
> change you try to make here.

By all means. Apologies for introducing the bug. Needless to say, I'll
let this one lie.

> In fact I really wish you would just leave this code as-is instead
> of trying to make such pointless tweaks to it.

My intention was to make the code a little more readable, and I did
test this by running it, but yes, a bug is a bug. As you can probably
tell, my expertise is in networking, not in writing kernel code. But I
do think that this code does the wrong thing in some areas, so I
wanted to try to fix it. In particular, keeping invalid autoconfig
addresses and routes configured on interfaces is guaranteed to break
connectivity.

I have two other patches that I think will improve the behaviour of
the stack from a networking perspective. One of them makes the kernel
properly update the lifetime of temporary addresses (this changed
between RFC 3041 and RFC 4941). The other deprecates addresses when
link goes down, which will do pretty much the same as my original
patch to delete the addresses, but will not cause the loss of any
privacy addresses configured.

My plan was to let these soak on my laptop (which I have been doing
for a few weeks), show them to colleagues who write kernel code (which
I have done), and submit them to this list. Is there anything else I
should be doing?

Thanks,
Lorenzo

Re: [PATCH] ipv6: slightly simplify keeping IPv6 addresses on link down

[Lorenzo Colitti]

ipv6: slightly simplify keeping IPv6 addresses on link down

When link goes down, all statically-configured (i.e.,
permanent and not link-local) IPv6 addresses are kept on
the interface. Instead of moving addresses to a temporary
keep list and then splicing that back on to the interface
address list, use list_for_each_entry_safe and delete the
ones we don't want.

Tested by configuring two static addresses on an interface
and verifying that pings from the addresses keep working
when bringing link down and up again and when disabling and
re-enabling IPv6 on the interface.

Signed-off-by: Lorenzo Colitti <lorenzo@google.com>

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 23cc8e1..6dfd5c5 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -2663,8 +2663,7 @@ static int addrconf_ifdown(struct net_device *dev, int how)
 {
 	struct net *net = dev_net(dev);
 	struct inet6_dev *idev;
-	struct inet6_ifaddr *ifa;
-	LIST_HEAD(keep_list);
+	struct inet6_ifaddr *ifa, *ifn;
 	int state;
 
 	ASSERT_RTNL();
@@ -2719,9 +2718,7 @@ static int addrconf_ifdown(struct net_device *dev, int how)
 	}
 #endif
 
-	while (!list_empty(&idev->addr_list)) {
-		ifa = list_first_entry(&idev->addr_list,
-				       struct inet6_ifaddr, if_list);
+	list_for_each_entry_safe(ifa, ifn, &idev->addr_list, if_list) {
 		addrconf_del_timer(ifa);
 
 		/* If just doing link down, and address is permanent
@@ -2729,15 +2726,13 @@ static int addrconf_ifdown(struct net_device *dev, int how)
 		if (!how &&
 		    (ifa->flags&IFA_F_PERMANENT) &&
 		    !(ipv6_addr_type(&ifa->addr) & IPV6_ADDR_LINKLOCAL)) {
-			list_move_tail(&ifa->if_list, &keep_list);
-
 			/* If not doing DAD on this address, just keep it. */
 			if ((dev->flags&(IFF_NOARP|IFF_LOOPBACK)) ||
 			    idev->cnf.accept_dad <= 0 ||
 			    (ifa->flags & IFA_F_NODAD))
 				continue;
 
-			/* If it was tentative already, no need to notify */
+			/* If it was tentative already, dont do anything */
 			if (ifa->flags & IFA_F_TENTATIVE)
 				continue;
 
@@ -2769,8 +2764,6 @@ static int addrconf_ifdown(struct net_device *dev, int how)
 		}
 	}
 
-	list_splice(&keep_list, &idev->addr_list);
-
 	write_unlock_bh(&idev->lock);
 
 	/* Step 5: Discard multicast list */