[PATCH] ipv6: fix inconsistent prefix route handling

[PATCH] ipv6: fix inconsistent prefix route handling

[Steven Barth]

When temporary addresses are created in userspace an accompanying
on-link prefix-route is created alongside. However when said address is
afterwards removed in userspace the respective prefix route remains
until it expires. Similarly when userspace turns a permanent address
into a temporary one the prefix route remains and is not removed when
the address expires and is thus dangling until removed manually.

This behavior is inconsistent with the addition and removal of permanent
addresses and can potentially cause lots of dangling routes on
downstream interfaces on an IPv6-router with changing prefixes.

This patch runs the on-link prefix removal check also when a temporary
address is removed and thus unifies the prefix route handling.

Signed-off-by: Steven Barth <cyrus@openwrt.org>
---
 net/ipv6/addrconf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index ad23569..55c35d5 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -1013,7 +1013,7 @@ static void ipv6_del_addr(struct inet6_ifaddr *ifp)
 		__in6_ifa_put(ifp);
 	}
 -	if (ifp->flags & IFA_F_PERMANENT && !(ifp->flags & IFA_F_NOPREFIXROUTE))
+	if (!(ifp->flags & IFA_F_NOPREFIXROUTE))
 		action = check_cleanup_prefix_route(ifp, &expires);
  	list_del_init(&ifp->if_list);
-- 
1.8.3.2

[PATCH v2] ipv6: fix inconsistent prefix route handling

[Steven Barth]

When temporary addresses are created in userspace an accompanying
on-link prefix-route is created alongside. However when said address is
afterwards removed in userspace the respective prefix route remains
until it expires. Similarly when userspace turns a permanent address
into a temporary one the prefix route remains and is not removed when
the address expires and is thus dangling until removed manually.

This behavior is inconsistent with the addition and removal of permanent
addresses and can potentially cause lots of dangling routes on
downstream interfaces on an IPv6-router with changing prefixes.

This patch runs the on-link prefix removal check also when a temporary
address is removed and thus unifies the prefix route handling.

Signed-off-by: Steven Barth <cyrus@openwrt.org>
---
 net/ipv6/addrconf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index ad23569..55c35d5 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -1013,7 +1013,7 @@ static void ipv6_del_addr(struct inet6_ifaddr *ifp)
 		__in6_ifa_put(ifp);
 	}
 
-	if (ifp->flags & IFA_F_PERMANENT && !(ifp->flags & IFA_F_NOPREFIXROUTE))
+	if (!(ifp->flags & IFA_F_NOPREFIXROUTE))
 		action = check_cleanup_prefix_route(ifp, &expires);
 
 	list_del_init(&ifp->if_list);
-- 
1.8.3.2

Re: [PATCH v2] ipv6: fix inconsistent prefix route handling

[Hannes Frederic Sowa]

On Sun, Feb 16, 2014 at 05:25:54PM +0100, Steven Barth wrote:
> -	if (ifp->flags & IFA_F_PERMANENT && !(ifp->flags & IFA_F_NOPREFIXROUTE))
> +	if (!(ifp->flags & IFA_F_NOPREFIXROUTE))
>  		action = check_cleanup_prefix_route(ifp, &expires);

This could too easily clean up valid on-link information if the address just
happens to be in the same subnet. Would (ifp-flags &
(IFA_F_PERMANENET|IFA_F_TEMPORARY) && !(...)) solve the problem, too?

Greetings,

  Hannes

Re: [PATCH v2] ipv6: fix inconsistent prefix route handling

[Steven Barth]

Hannes Frederic Sowa wrote:
> This could too easily clean up valid on-link information if the address
> just happens to be in the same subnet. Would (ifp-flags &
> (IFA_F_PERMANENET|IFA_F_TEMPORARY) && !(...)) solve the problem, too?

No, that doesn't do the trick unfortunately. Seems that the 
(non-permanent) address created by "ip" don't have IFA_F_TEMPORARY set.
Any other good ideas?


Cheers,

Steven

Re: [PATCH v2] ipv6: fix inconsistent prefix route handling

[Hannes Frederic Sowa]

On Mon, Feb 17, 2014 at 07:41:42AM +0100, Steven Barth wrote:
> Hannes Frederic Sowa wrote:
> >This could too easily clean up valid on-link information if the address
> >just happens to be in the same subnet. Would (ifp-flags &
> >(IFA_F_PERMANENET|IFA_F_TEMPORARY) && !(...)) solve the problem, too?
> 
> No, that doesn't do the trick unfortunately. Seems that the 
> (non-permanent) address created by "ip" don't have IFA_F_TEMPORARY set.
> Any other good ideas?

We are talking about managed temp addresses, no? What ip command do you use?

Bye,

  Hannes

Re: [PATCH v2] ipv6: fix inconsistent prefix route handling

[Steven Barth]

Ah sorry, maybe I was a bit unclear about this. With temporary addresses 
I meant addresses created like this:

ip address add 2001:db8::1/64 dev eth0 valid_lft 1234 preferred_lft 1001

or the equivalent netlink-calls and with permanent addresses i was 
referring to addresses created similarly but with an infinite lifetime 
(e.g. through ip without giving valid_lft or preferred_lft arguments).

Hope that makes it clear.


Cheers,

Steven


On 17.02.2014 14:05, Hannes Frederic Sowa wrote:
> On Mon, Feb 17, 2014 at 07:41:42AM +0100, Steven Barth wrote:
>> Hannes Frederic Sowa wrote:
>>> This could too easily clean up valid on-link information if the address
>>> just happens to be in the same subnet. Would (ifp-flags &
>>> (IFA_F_PERMANENET|IFA_F_TEMPORARY) && !(...)) solve the problem, too?
>> No, that doesn't do the trick unfortunately. Seems that the
>> (non-permanent) address created by "ip" don't have IFA_F_TEMPORARY set.
>> Any other good ideas?
> We are talking about managed temp addresses, no? What ip command do you use?
>
> Bye,
>
>    Hannes
>
> --
> 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 v2] ipv6: fix inconsistent prefix route handling

[Hannes Frederic Sowa]

On Mon, Feb 17, 2014 at 02:13:27PM +0100, Steven Barth wrote:
> Ah sorry, maybe I was a bit unclear about this. With temporary addresses 
> I meant addresses created like this:
> 
> ip address add 2001:db8::1/64 dev eth0 valid_lft 1234 preferred_lft 1001
> 
> or the equivalent netlink-calls and with permanent addresses i was 
> referring to addresses created similarly but with an infinite lifetime 
> (e.g. through ip without giving valid_lft or preferred_lft arguments).
> 
> Hope that makes it clear.

Yes, makes sense, thanks!

I fear there is currently no combination of ifp->flags which can test for
that. Your first patch seems to be too dangerous in removing neighbour
discovery brought in on-link state, I fear. It may even be a problem for
permanent ones, but that's how it is done, currently.

Best option would be to fully decouple prefix routes from address handling,
but that would break current userspace.

Current iproute has support for no-prefix-route address addition (see
iproute2 commit 58c69b226fb3a ("add support for IFA_F_NOPREFIXROUTE")), so you
could manage those address and route combination directly from user space.

So if we want to clean up those routes we either must set IFA_F_PERMANENT to
the ifp flags of the address in inet6_addr_add and really make sure nothing
breaks because of this. Or introduce a new flag, like IFA_F_USERINSTALLED e.g.

Current ifp flags handling is really complex and we had some bad bugs
in recent changes, so I guess, it is not a trivial task (I reviewed the
last patches changing code in that for hours and they still had bugs :/), so I
am unsure what is the right approach. Both ways (adding PERMANENT) and adding
a new IFP_P_USERINSTALLED would need to be considered and reviewed.

Thanks,

  Hannes

Re: [PATCH v2] ipv6: fix inconsistent prefix route handling

[Steven Barth]

On 17.02.2014 15:08, Hannes Frederic Sowa wrote:
>
> I fear there is currently no combination of ifp->flags which can test for
> that. Your first patch seems to be too dangerous in removing neighbour
> discovery brought in on-link state, I fear. It may even be a problem for
> permanent ones, but that's how it is done, currently.
Yeah the kernel neighbor discovery support is a bit tricky in itself and 
I found myself working around it more often then actually using it (at 
least when doing router-stuff).

>
> Best option would be to fully decouple prefix routes from address handling,
> but that would break current userspace.
>
> Current iproute has support for no-prefix-route address addition (see
> iproute2 commit 58c69b226fb3a ("add support for IFA_F_NOPREFIXROUTE")), so you
> could manage those address and route combination directly from user space.
OK, I will use a workaround for now that removes and readds addresses 
when they should be deprecated instead of just converting permanents to 
non-permanents and maybe later use that new NOPREFIXROUTE feature which 
should come in handy.

>
> So if we want to clean up those routes we either must set IFA_F_PERMANENT to
> the ifp flags of the address in inet6_addr_add and really make sure nothing
> breaks because of this. Or introduce a new flag, like IFA_F_USERINSTALLED e.g.
OK, I will try to think about this when I have some more time to spare 
on this.
Thanks for your help so far.

Steven