[PATCH] [IPV4] Fix secondary IP addresses after promotion

[PATCH] [IPV4] Fix secondary IP addresses after promotion

[Brian Pomerantz]

When 3 or more IP addresses in the same subnet exist on a device and the
first one is removed, only the promoted IP address can be reached.  Just
after promotion of the next IP address, this fix spins through any more
IP addresses on the interface and sends a NETDEV_UP notification for
that address.  This repopulates the FIB with the proper route
information.

Signed-off-by: Brian Pomerantz <bapper@piratehaven.org>

---

 net/ipv4/devinet.c |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

applies-to: 9bbb209cab841f700162a96e158dfa3ecd361f46
489d4e25469c8329451aca3e91c8e1929e6ecf63
diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
index 4ec4b2c..72d6c93 100644
--- a/net/ipv4/devinet.c
+++ b/net/ipv4/devinet.c
@@ -235,6 +235,7 @@ static void inet_del_ifa(struct in_devic
 {
 	struct in_ifaddr *promote = NULL;
 	struct in_ifaddr *ifa1 = *ifap;
+	struct in_ifaddr *ifa;
 
 	ASSERT_RTNL();
 
@@ -243,7 +244,6 @@ static void inet_del_ifa(struct in_devic
 	 **/
 
 	if (!(ifa1->ifa_flags & IFA_F_SECONDARY)) {
-		struct in_ifaddr *ifa;
 		struct in_ifaddr **ifap1 = &ifa1->ifa_next;
 
 		while ((ifa = *ifap1) != NULL) {
@@ -294,7 +294,13 @@ static void inet_del_ifa(struct in_devic
 		/* not sure if we should send a delete notify first? */
 		promote->ifa_flags &= ~IFA_F_SECONDARY;
 		rtmsg_ifa(RTM_NEWADDR, promote);
-		notifier_call_chain(&inetaddr_chain, NETDEV_UP, promote);
+
+		/* update fib in the rest of this address list */
+		ifa = promote;
+		while (ifa != NULL) {
+			notifier_call_chain(&inetaddr_chain, NETDEV_UP, ifa);
+			ifa = ifa->ifa_next;
+		}
 	}
 }
 
---
0.99.9.GIT

Re: [PATCH] [IPV4] Fix secondary IP addresses after promotion

[Patrick McHardy]

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

Brian Pomerantz wrote:
> When 3 or more IP addresses in the same subnet exist on a device and the
> first one is removed, only the promoted IP address can be reached.  Just
> after promotion of the next IP address, this fix spins through any more
> IP addresses on the interface and sends a NETDEV_UP notification for
> that address.  This repopulates the FIB with the proper route
> information.
> 
> @@ -294,7 +294,13 @@ static void inet_del_ifa(struct in_devic
>  		/* not sure if we should send a delete notify first? */
>  		promote->ifa_flags &= ~IFA_F_SECONDARY;
>  		rtmsg_ifa(RTM_NEWADDR, promote);
> -		notifier_call_chain(&inetaddr_chain, NETDEV_UP, promote);
> +
> +		/* update fib in the rest of this address list */
> +		ifa = promote;
> +		while (ifa != NULL) {
> +			notifier_call_chain(&inetaddr_chain, NETDEV_UP, ifa);
> +			ifa = ifa->ifa_next;
> +		}
>  	}
>  }

You assume all addresses following the primary addresses are secondary
addresses of the primary, which is not true with multiple primaries.
This patch (untested) makes sure only to send notification for real
secondaries of the deleted address. It also removes a racy double-
check for IN_DEV_PROMOTE_SECONDARIES - once we've decided to promote
an address checking again opens a window in which address promotion
could be disabled and we end up with only secondaries without a
primary address.

Signed-off-by: Patrick McHardy <kaber@trash.net>


[-- Attachment #2: x --]
[-- Type: text/plain, Size: 1535 bytes --]

diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
index 4ec4b2c..beb02cc 100644
--- a/net/ipv4/devinet.c
+++ b/net/ipv4/devinet.c
@@ -234,7 +234,7 @@ static void inet_del_ifa(struct in_devic
 			 int destroy)
 {
 	struct in_ifaddr *promote = NULL;
-	struct in_ifaddr *ifa1 = *ifap;
+	struct in_ifaddr *ifa, *ifa1 = *ifap;
 
 	ASSERT_RTNL();
 
@@ -243,7 +243,6 @@ static void inet_del_ifa(struct in_devic
 	 **/
 
 	if (!(ifa1->ifa_flags & IFA_F_SECONDARY)) {
-		struct in_ifaddr *ifa;
 		struct in_ifaddr **ifap1 = &ifa1->ifa_next;
 
 		while ((ifa = *ifap1) != NULL) {
@@ -283,19 +282,25 @@ static void inet_del_ifa(struct in_devic
 	 */
 	rtmsg_ifa(RTM_DELADDR, ifa1);
 	notifier_call_chain(&inetaddr_chain, NETDEV_DOWN, ifa1);
+
+	if (promote) {
+		/* not sure if we should send a delete notify first? */
+		promote->ifa_flags &= ~IFA_F_SECONDARY;
+		rtmsg_ifa(RTM_NEWADDR, promote);
+		for (ifa = promote; ifa; ifa = ifa->ifa_next) {
+			if (ifa1->ifa_mask != ifa->ifa_mask ||
+			    !inet_ifa_match(ifa1->ifa_address, ifa))
+				continue;
+			notifier_call_chain(&inetaddr_chain, NETDEV_UP, ifa);
+		}
+	}
+
 	if (destroy) {
 		inet_free_ifa(ifa1);
 
 		if (!in_dev->ifa_list)
 			inetdev_destroy(in_dev);
 	}
-
-	if (promote && IN_DEV_PROMOTE_SECONDARIES(in_dev)) {
-		/* not sure if we should send a delete notify first? */
-		promote->ifa_flags &= ~IFA_F_SECONDARY;
-		rtmsg_ifa(RTM_NEWADDR, promote);
-		notifier_call_chain(&inetaddr_chain, NETDEV_UP, promote);
-	}
 }
 
 static int inet_insert_ifa(struct in_ifaddr *ifa)

Re: [PATCH] [IPV4] Fix secondary IP addresses after promotion

[Brian Pomerantz]

On Sat, Nov 05, 2005 at 01:34:16AM +0100, Patrick McHardy wrote:
> 
> You assume all addresses following the primary addresses are secondary
> addresses of the primary, which is not true with multiple primaries.
> This patch (untested) makes sure only to send notification for real
> secondaries of the deleted address. It also removes a racy double-
> check for IN_DEV_PROMOTE_SECONDARIES - once we've decided to promote
> an address checking again opens a window in which address promotion
> could be disabled and we end up with only secondaries without a
> primary address.
> 

Yeah, I was wondering if there could be primaries after the
secondaries.  I'm pretty unfamiliar with this code (first looked at it
last week) and still don't have a handle on how the primaries
interact with the secondaries in the route lookup.  Which means it's
not clear to me why this was failing to begin with. :)

Your patch works for all of the cases I've been testing with so it
looks good to go from here.


BAPper

Re: [PATCH] [IPV4] Fix secondary IP addresses after promotion

[Thomas Graf]

* Patrick McHardy <kaber@trash.net> 2005-11-05 01:34
> Brian Pomerantz wrote:
> >When 3 or more IP addresses in the same subnet exist on a device and the
> >first one is removed, only the promoted IP address can be reached.  Just
> >after promotion of the next IP address, this fix spins through any more
> >IP addresses on the interface and sends a NETDEV_UP notification for
> >that address.  This repopulates the FIB with the proper route
> >information.
> >
> >@@ -294,7 +294,13 @@ static void inet_del_ifa(struct in_devic
> > 		/* not sure if we should send a delete notify first? */
> > 		promote->ifa_flags &= ~IFA_F_SECONDARY;
> > 		rtmsg_ifa(RTM_NEWADDR, promote);
> >-		notifier_call_chain(&inetaddr_chain, NETDEV_UP, promote);
> >+
> >+		/* update fib in the rest of this address list */
> >+		ifa = promote;
> >+		while (ifa != NULL) {
> >+			notifier_call_chain(&inetaddr_chain, NETDEV_UP, ifa);
> >+			ifa = ifa->ifa_next;
> >+		}
> > 	}
> > }
> 
> You assume all addresses following the primary addresses are secondary
> addresses of the primary, which is not true with multiple primaries.
> This patch (untested) makes sure only to send notification for real
> secondaries of the deleted address.

Even this corrected version is only a workaround, the real bug is that 
or whatever reason all local routes of seconaries get deleted upon an
address promotion. I started debugging it a bit by looking at the
requests generated by fib_magic() and the resulting notifications, the
local routes just disappear when they shouldn't.

Situation is: 10.0.0.[1-4]/24 on dev0, 10.0.0.1 is the primary address
and gets deleted while address promotion is enabled. The following
happens:

	[Format:]
		Request generated by fib_magic()
			Notification event received

RTM_DELROUTE 10.0.0.0/24 dev eth0 scope link
  unicast table main protocol 2 preferred-src 10.0.0.1
        RTM_DELROUTE 10.0.0.0/24 dev eth0 scope link
          unicast table main protocol 2 preferred-src 10.0.0.1

RTM_DELROUTE 10.0.0.255 dev eth0 scope link
  broadcast table local protocol 2 preferred-src 10.0.0.1
        RTM_DELROUTE 10.0.0.255 dev eth0 scope link
          broadcast table local protocol 2 preferred-src 10.0.0.1

RTM_DELROUTE 10.0.0.0 dev eth0 scope link
  broadcast table local protocol 2 preferred-src 10.0.0.1
        RTM_DELROUTE 10.0.0.0 dev eth0 scope link
          broadcast table local protocol 2 preferred-src 10.0.0.1

RTM_DELROUTE 10.0.0.1 dev eth0 scope host
  local table local protocol 2 preferred-src 10.0.0.1
        RTM_DELROUTE 10.0.0.1 dev eth0 scope host
          local table local protocol 2 preferred-src 10.0.0.1

RTM_NEWROUTE 10.0.0.2 dev eth0 scope host
  local table local protocol 2 preferred-src 10.0.0.2
        RTM_NEWROUTE 10.0.0.2 dev eth0 scope host
          local table local protocol 2 preferred-src 10.0.0.2

RTM_NEWROUTE 10.0.0.0/24 dev eth0 scope link
  unicast table main protocol 2 preferred-src 10.0.0.2
        RTM_NEWROUTE 10.0.0.0/24 dev eth0 scope link
          unicast table main protocol 2 preferred-src 10.0.0.2

RTM_NEWROUTE 10.0.0.0 dev eth0 scope link
  broadcast table local protocol 2 preferred-src 10.0.0.2
        RTM_NEWROUTE 10.0.0.0 dev eth0 scope link
          broadcast table local protocol 2 preferred-src 10.0.0.2

RTM_NEWROUTE 10.0.0.255 dev eth0 scope link
  broadcast table local protocol 2 preferred-src 10.0.0.2
        RTM_NEWROUTE 10.0.0.255 dev eth0 scope link
          broadcast table local protocol 2 preferred-src 10.0.0.2

State afterwards:
4: eth0: <BROADCAST,MULTICAST,UP> mtu 1500 qdisc pfifo_fast qlen 1000
    inet 10.0.0.2/24 scope global eth0
    inet 10.0.0.3/24 scope global secondary eth0
    inet 10.0.0.4/24 scope global secondary eth0

broadcast 10.0.0.0  proto kernel  scope link  src 10.0.0.2 
local 10.0.0.2  proto kernel  scope host  src 10.0.0.2 
broadcast 10.0.0.255  proto kernel  scope link  src 10.0.0.2 

Local routes for 10.0.0.3 and 10.0.0.4 have disappeared _without_
any notification.

I think the correct way to fix this is to prevent the deletion of
the local routes, not just readding them. _If_ the deletion of them
is intended, which I doubt, then at least notifications must be
sent out.

Code to get fib_magic() requests to userspace:

Index: linux-2.6/include/linux/rtnetlink.h
===================================================================
--- linux-2.6.orig/include/linux/rtnetlink.h
+++ linux-2.6/include/linux/rtnetlink.h
@@ -880,6 +880,8 @@ enum rtnetlink_groups {
 #define RTNLGRP_DECnet_ROUTE	RTNLGRP_DECnet_ROUTE
 	RTNLGRP_IPV6_PREFIX,
 #define RTNLGRP_IPV6_PREFIX	RTNLGRP_IPV6_PREFIX
+	RTNLGRP_FIB_MAGIC,
+#define RTNLGRP_FIB_MAGIC	RTNLGRP_FIB_MAGIC
 	__RTNLGRP_MAX
 };
 #define RTNLGRP_MAX	(__RTNLGRP_MAX - 1)
Index: linux-2.6/net/ipv4/fib_frontend.c
===================================================================
--- linux-2.6.orig/net/ipv4/fib_frontend.c
+++ linux-2.6/net/ipv4/fib_frontend.c
@@ -359,6 +359,48 @@ int inet_dump_fib(struct sk_buff *skb, s
 	return skb->len;
 }
 
+static int fib_magic_build(struct sk_buff *skb, int type, struct nlmsghdr *nlh,
+			   struct rtmsg *rtm, struct kern_rta *rta)
+{
+	struct nlmsghdr *dst = NULL;
+	struct rtmsg *rtm_dst;
+
+	dst = NLMSG_NEW(skb, current->pid, 0, type, sizeof(*rtm), 0);
+	memcpy(dst, nlh, sizeof(*nlh));
+
+	rtm_dst = NLMSG_DATA(dst);
+	memcpy(rtm_dst, rtm, sizeof(*rtm));
+	rtm_dst->rtm_family = AF_INET;
+
+	RTA_PUT(skb, RTA_DST, 4, rta->rta_dst);
+	RTA_PUT(skb, RTA_PREFSRC, 4, rta->rta_prefsrc);
+	RTA_PUT(skb, RTA_OIF, 4, rta->rta_oif);
+
+	return NLMSG_END(skb, dst);
+rtattr_failure:
+nlmsg_failure:
+	return NLMSG_CANCEL(skb, dst);
+}
+
+static void fib_magic_event(int type, struct nlmsghdr *nlh, struct rtmsg *rtm,
+			    struct kern_rta *rta)
+{
+	struct sk_buff *skb;
+
+	skb = alloc_skb(NLMSG_SPACE(sizeof(struct rtmsg) + 256), GFP_KERNEL);
+	if (!skb)
+		return;
+
+	if (fib_magic_build(skb, type, nlh, rtm, rta) < 0) {
+		kfree_skb(skb);
+		return;
+	}
+
+	NETLINK_CB(skb).dst_group = RTNLGRP_FIB_MAGIC;
+	netlink_broadcast(rtnl, skb, 0, RTNLGRP_FIB_MAGIC, GFP_KERNEL);
+}
+
+
 /* Prepare and feed intra-kernel routing request.
    Really, it should be netlink message, but :-( netlink
    can be not configured, so that we feed it directly
@@ -402,6 +444,8 @@ static void fib_magic(int cmd, int type,
 	rta.rta_prefsrc = &ifa->ifa_local;
 	rta.rta_oif = &ifa->ifa_dev->dev->ifindex;
 
+	fib_magic_event(cmd, &req.nlh, &req.rtm, &rta);
+
 	if (cmd == RTM_NEWROUTE)
 		tb->tb_insert(tb, &req.rtm, &rta, &req.nlh, NULL);
 	else

Re: [PATCH] [IPV4] Fix secondary IP addresses after promotion

[Patrick McHardy]

Thomas Graf wrote:
> * Patrick McHardy <kaber@trash.net> 2005-11-05 01:34
> 
>>You assume all addresses following the primary addresses are secondary
>>addresses of the primary, which is not true with multiple primaries.
>>This patch (untested) makes sure only to send notification for real
>>secondaries of the deleted address.
> 
> 
> Even this corrected version is only a workaround, the real bug is that 
> or whatever reason all local routes of seconaries get deleted upon an
> address promotion. I started debugging it a bit by looking at the
> requests generated by fib_magic() and the resulting notifications, the
> local routes just disappear when they shouldn't.
> 
> Situation is: 10.0.0.[1-4]/24 on dev0, 10.0.0.1 is the primary address
> and gets deleted while address promotion is enabled. The following
> happens:
> 
> 	[Format:]
> 		Request generated by fib_magic()
> 			Notification event received
> 
> RTM_DELROUTE 10.0.0.0/24 dev eth0 scope link
>   unicast table main protocol 2 preferred-src 10.0.0.1
>         RTM_DELROUTE 10.0.0.0/24 dev eth0 scope link
>           unicast table main protocol 2 preferred-src 10.0.0.1
> 
> RTM_DELROUTE 10.0.0.255 dev eth0 scope link
>   broadcast table local protocol 2 preferred-src 10.0.0.1
>         RTM_DELROUTE 10.0.0.255 dev eth0 scope link
>           broadcast table local protocol 2 preferred-src 10.0.0.1
> 
> RTM_DELROUTE 10.0.0.0 dev eth0 scope link
>   broadcast table local protocol 2 preferred-src 10.0.0.1
>         RTM_DELROUTE 10.0.0.0 dev eth0 scope link
>           broadcast table local protocol 2 preferred-src 10.0.0.1
> 
> RTM_DELROUTE 10.0.0.1 dev eth0 scope host
>   local table local protocol 2 preferred-src 10.0.0.1
>         RTM_DELROUTE 10.0.0.1 dev eth0 scope host
>           local table local protocol 2 preferred-src 10.0.0.1
> 
> RTM_NEWROUTE 10.0.0.2 dev eth0 scope host
>   local table local protocol 2 preferred-src 10.0.0.2
>         RTM_NEWROUTE 10.0.0.2 dev eth0 scope host
>           local table local protocol 2 preferred-src 10.0.0.2
> 
> RTM_NEWROUTE 10.0.0.0/24 dev eth0 scope link
>   unicast table main protocol 2 preferred-src 10.0.0.2
>         RTM_NEWROUTE 10.0.0.0/24 dev eth0 scope link
>           unicast table main protocol 2 preferred-src 10.0.0.2
> 
> RTM_NEWROUTE 10.0.0.0 dev eth0 scope link
>   broadcast table local protocol 2 preferred-src 10.0.0.2
>         RTM_NEWROUTE 10.0.0.0 dev eth0 scope link
>           broadcast table local protocol 2 preferred-src 10.0.0.2
> 
> RTM_NEWROUTE 10.0.0.255 dev eth0 scope link
>   broadcast table local protocol 2 preferred-src 10.0.0.2
>         RTM_NEWROUTE 10.0.0.255 dev eth0 scope link
>           broadcast table local protocol 2 preferred-src 10.0.0.2
> 
> State afterwards:
> 4: eth0: <BROADCAST,MULTICAST,UP> mtu 1500 qdisc pfifo_fast qlen 1000
>     inet 10.0.0.2/24 scope global eth0
>     inet 10.0.0.3/24 scope global secondary eth0
>     inet 10.0.0.4/24 scope global secondary eth0
> 
> broadcast 10.0.0.0  proto kernel  scope link  src 10.0.0.2 
> local 10.0.0.2  proto kernel  scope host  src 10.0.0.2 
> broadcast 10.0.0.255  proto kernel  scope link  src 10.0.0.2 
> 
> Local routes for 10.0.0.3 and 10.0.0.4 have disappeared _without_
> any notification.
> 
> I think the correct way to fix this is to prevent the deletion of
> the local routes, not just readding them. _If_ the deletion of them
> is intended, which I doubt, then at least notifications must be
> sent out.

I agree, the routes should ideally not be deleted at all. The missing
notifications appear to be a different bug. Let me have another look ..

Re: [PATCH] [IPV4] Fix secondary IP addresses after promotion

[Patrick McHardy]

Patrick McHardy wrote:
> Thomas Graf wrote:
> 
>> broadcast 10.0.0.0  proto kernel  scope link  src 10.0.0.2 local 
>> 10.0.0.2  proto kernel  scope host  src 10.0.0.2 broadcast 10.0.0.255  
>> proto kernel  scope link  src 10.0.0.2
>> Local routes for 10.0.0.3 and 10.0.0.4 have disappeared _without_
>> any notification.
>>
>> I think the correct way to fix this is to prevent the deletion of
>> the local routes, not just readding them. _If_ the deletion of them
>> is intended, which I doubt, then at least notifications must be
>> sent out.
> 
> I agree, the routes should ideally not be deleted at all. The missing
> notifications appear to be a different bug. Let me have another look ..

The reason why all routes are deleted is because their prefered
source addresses is the primary address. fn_flush_list should
probably send the missing notifications for the deleted routes.
Changing address promotion to not delete the other routes at all
looks extremly complicated, I think just fixing it to behave
correctly is good enough (which my patch didn't do entirely,
I'll send a new one this weekend).

Re: [PATCH] [IPV4] Fix secondary IP addresses after promotion

[Thomas Graf]

* Patrick McHardy <kaber@trash.net> 2005-11-05 05:28
> The reason why all routes are deleted is because their prefered
> source addresses is the primary address. fn_flush_list should
> probably send the missing notifications for the deleted routes.
> Changing address promotion to not delete the other routes at all
> looks extremly complicated, I think just fixing it to behave
> correctly is good enough (which my patch didn't do entirely,
> I'll send a new one this weekend).

Yes, fib_sync_down(), but even when I remove the code setting
RTNH_F_DEAD I still see _some_ local routes disappearing which
I cannot explain right now. I can only reproduce this with
!CONFIG_IP_MULTIPLE_TABLES though.

Assuming this is a separate bug, I'm not sure if this is the right
way to fix it. I think it would be better to rewrite the preferred
source address of all related local routes and only perform a
remove-and-add on the secondary address being promoted.

_If_ we let them die, we should announce it in fib_sync_down()
rather then in the algorithm specific flush routines.

Re: [PATCH] [IPV4] Fix secondary IP addresses after promotion

[Thomas Graf]

* Thomas Graf <tgraf@suug.ch> 2005-11-05 14:46
> Assuming this is a separate bug, I'm not sure if this is the right
> way to fix it. I think it would be better to rewrite the preferred
> source address of all related local routes and only perform a
> remove-and-add on the secondary address being promoted.

I tried it out and although it works it's not clean yet because
changing fib_info also changes pref_src for the routes to be
deleted which will lead to slightly incorrect notifications later
on. I now think that explicitely deleting and re-adding them
later on is better as well ;-)

Index: linux-2.6/net/ipv4/devinet.c
===================================================================
--- linux-2.6.orig/net/ipv4/devinet.c
+++ linux-2.6/net/ipv4/devinet.c
@@ -230,31 +230,38 @@ int inet_addr_onlink(struct in_device *i
 	return 0;
 }
 
+static inline int ifa_is_secondary(struct in_ifaddr *primary,
+				   struct in_ifaddr *candiate)
+{
+	return ((candiate->ifa_flags & IFA_F_SECONDARY) &&
+		primary->ifa_mask == candiate->ifa_mask &&
+		inet_ifa_match(primary->ifa_address, candiate));
+}
+
 static void inet_del_ifa(struct in_device *in_dev, struct in_ifaddr **ifap,
 			 int destroy)
 {
+	int do_promote;
 	struct in_ifaddr *promote = NULL;
-	struct in_ifaddr *ifa1 = *ifap;
+	struct in_ifaddr *ifa, *ifa1 = *ifap;
 
 	ASSERT_RTNL();
 
 	/* 1. Deleting primary ifaddr forces deletion all secondaries 
 	 * unless alias promotion is set
 	 **/
+	do_promote = IN_DEV_PROMOTE_SECONDARIES(in_dev);
 
 	if (!(ifa1->ifa_flags & IFA_F_SECONDARY)) {
-		struct in_ifaddr *ifa;
 		struct in_ifaddr **ifap1 = &ifa1->ifa_next;
 
 		while ((ifa = *ifap1) != NULL) {
-			if (!(ifa->ifa_flags & IFA_F_SECONDARY) ||
-			    ifa1->ifa_mask != ifa->ifa_mask ||
-			    !inet_ifa_match(ifa1->ifa_address, ifa)) {
+			if (!ifa_is_secondary(ifa1, ifa)) {
 				ifap1 = &ifa->ifa_next;
 				continue;
 			}
 
-			if (!IN_DEV_PROMOTE_SECONDARIES(in_dev)) {
+			if (!do_promote) {
 				*ifap1 = ifa->ifa_next;
 
 				rtmsg_ifa(RTM_DELADDR, ifa);
@@ -271,6 +278,10 @@ static void inet_del_ifa(struct in_devic
 
 	*ifap = ifa1->ifa_next;
 
+	for (ifa = promote; ifa != NULL; ifa = ifa->ifa_next)
+		if (ifa_is_secondary(ifa1, ifa))
+			fib_promote(ifa1->ifa_local, ifa->ifa_local);
+
 	/* 3. Announce address deletion */
 
 	/* Send message first, then call notifier.
@@ -290,7 +301,7 @@ static void inet_del_ifa(struct in_devic
 			inetdev_destroy(in_dev);
 	}
 
-	if (promote && IN_DEV_PROMOTE_SECONDARIES(in_dev)) {
+	if (promote) {
 		/* not sure if we should send a delete notify first? */
 		promote->ifa_flags &= ~IFA_F_SECONDARY;
 		rtmsg_ifa(RTM_NEWADDR, promote);
Index: linux-2.6/include/net/ip_fib.h
===================================================================
--- linux-2.6.orig/include/net/ip_fib.h
+++ linux-2.6/include/net/ip_fib.h
@@ -242,6 +242,7 @@ extern void fib_select_multipath(const s
 extern int ip_fib_check_default(u32 gw, struct net_device *dev);
 extern int fib_sync_down(u32 local, struct net_device *dev, int force);
 extern int fib_sync_up(struct net_device *dev);
+extern int fib_promote(u32 old, u32 new);
 extern int fib_convert_rtentry(int cmd, struct nlmsghdr *nl, struct rtmsg *rtm,
 			       struct kern_rta *rta, struct rtentry *r);
 extern u32  __fib_res_prefsrc(struct fib_result *res);
Index: linux-2.6/net/ipv4/fib_semantics.c
===================================================================
--- linux-2.6.orig/net/ipv4/fib_semantics.c
+++ linux-2.6/net/ipv4/fib_semantics.c
@@ -1338,3 +1338,24 @@ void fib_select_multipath(const struct f
 	spin_unlock_bh(&fib_multipath_lock);
 }
 #endif
+
+int fib_promote(u32 old, u32 new)
+{
+	int ret = 0;
+
+	if (old && fib_info_laddrhash) {
+		unsigned int hash = fib_laddr_hashfn(old);
+		struct hlist_head *head = &fib_info_laddrhash[hash];
+		struct hlist_node *node;
+		struct fib_info *fi;
+
+		hlist_for_each_entry(fi, node, head, fib_lhash) {
+			if (fi->fib_prefsrc == old) {
+				fi->fib_prefsrc = new;
+				ret++;
+			}
+		}
+	}
+
+	return ret;
+}

Re: [PATCH] [IPV4] Fix secondary IP addresses after promotion

[Patrick McHardy]

Thomas Graf wrote:
> * Thomas Graf <tgraf@suug.ch> 2005-11-05 14:46
> 
>>Assuming this is a separate bug, I'm not sure if this is the right
>>way to fix it. I think it would be better to rewrite the preferred
>>source address of all related local routes and only perform a
>>remove-and-add on the secondary address being promoted.
> 
> 
> I tried it out and although it works it's not clean yet because
> changing fib_info also changes pref_src for the routes to be
> deleted which will lead to slightly incorrect notifications later
> on. I now think that explicitely deleting and re-adding them
> later on is better as well ;-)

Yes, fixing it correctly looks very hard. Just changing the routes
doesn't seem right to me, someone might have added it with exactly
this prefsrc and doesn't want it to change, its also not clear how
to notify on this. Taking care of correct ordering of the ifa_list
is also more complicated without just deleting and readding them.

I have a patch to do this, but it needs some debugging, for some
unknown reason it crashes sometimes if I remove addresses without
specifying the mask.

Re: [PATCH] [IPV4] Fix secondary IP addresses after promotion

[Thomas Graf]

* Patrick McHardy <kaber@trash.net> 2005-11-08 15:11
> Yes, fixing it correctly looks very hard. Just changing the routes
> doesn't seem right to me, someone might have added it with exactly
> this prefsrc and doesn't want it to change, its also not clear how
> to notify on this.

Right, OTOH this someone might also just use the primary address
as prefsrc and would welcome a translation to the new address
instead of a silent deletion. Nevertheless, I agree with deleting
and readding the local routes (for now ;-) while I keep this in
mind for later improvement.

> I have a patch to do this, but it needs some debugging, for some
> unknown reason it crashes sometimes if I remove addresses without
> specifying the mask.

Interesting, do you use an iproute2 version with the wildcard
address deletion fix attached below?

diff -Nru a/ChangeLog b/ChangeLog
--- a/ChangeLog	2005-03-19 00:49:52 +01:00
+++ b/ChangeLog	2005-03-19 00:49:52 +01:00
@@ -1,3 +1,8 @@
+2005-03-19  Thomas Graf <tgraf@suug.ch>
+
+	* Warn about wildcard deletions and provide IFA_ADDRESS upon
+	  deletions to enforce prefix length validation for IPv4.
+
 2005-03-18  Stephen Hemminger  <shemminger@osdl.org>
 
 	* add -force option to batch mode
diff -Nru a/include/utils.h b/include/utils.h
--- a/include/utils.h	2005-03-19 00:49:52 +01:00
+++ b/include/utils.h	2005-03-19 00:49:52 +01:00
@@ -43,8 +43,11 @@
 	__u8 family;
 	__u8 bytelen;
 	__s16 bitlen;
+	__u32 flags;
 	__u32 data[4];
 } inet_prefix;
+
+#define PREFIXLEN_SPECIFIED 1
 
 #define DN_MAXADDL 20
 #ifndef AF_DECnet
diff -Nru a/ip/ipaddress.c b/ip/ipaddress.c
--- a/ip/ipaddress.c	2005-03-19 00:49:52 +01:00
+++ b/ip/ipaddress.c	2005-03-19 00:49:52 +01:00
@@ -744,6 +744,7 @@
 	} req;
 	char  *d = NULL;
 	char  *l = NULL;
+	char  *lcl_arg = NULL;
 	inet_prefix lcl;
 	inet_prefix peer;
 	int local_len = 0;
@@ -821,6 +822,7 @@
 				usage();
 			if (local_len)
 				duparg2("local", *argv);
+			lcl_arg = *argv;
 			get_prefix(&lcl, *argv, req.ifa.ifa_family);
 			if (req.ifa.ifa_family == AF_UNSPEC)
 				req.ifa.ifa_family = lcl.family;
@@ -838,9 +840,17 @@
 		exit(1);
 	}
 
-	if (peer_len == 0 && local_len && cmd != RTM_DELADDR) {
-		peer = lcl;
-		addattr_l(&req.n, sizeof(req), IFA_ADDRESS, &lcl.data, lcl.bytelen);
+	if (peer_len == 0 && local_len) {
+		if (cmd == RTM_DELADDR && lcl.family == AF_INET && !(lcl.flags & PREFIXLEN_SPECIFIED)) {
+			fprintf(stderr,
+			    "Warning: Executing wildcard deletion to stay compatible with old scripts.\n" \
+			    "         Explicitly specify the prefix length (%s/%d) to avoid this warning.\n" \
+			    "         This special behaviour is likely to disappear in further releases,\n" \
+			    "         fix your scripts!\n", lcl_arg, local_len*8);
+		} else {
+			peer = lcl;
+			addattr_l(&req.n, sizeof(req), IFA_ADDRESS, &lcl.data, lcl.bytelen);
+		}
 	}
 	if (req.ifa.ifa_prefixlen == 0)
 		req.ifa.ifa_prefixlen = lcl.bitlen;
diff -Nru a/lib/utils.c b/lib/utils.c
--- a/lib/utils.c	2005-03-19 00:49:52 +01:00
+++ b/lib/utils.c	2005-03-19 00:49:52 +01:00
@@ -241,6 +241,7 @@
 				err = -1;
 				goto done;
 			}
+			dst->flags |= PREFIXLEN_SPECIFIED;
 			dst->bitlen = plen;
 		}
 	}

Re: [PATCH] [IPV4] Fix secondary IP addresses after promotion

[Patrick McHardy]

Thomas Graf wrote:
> * Patrick McHardy <kaber@trash.net> 2005-11-08 15:11
>
>>I have a patch to do this, but it needs some debugging, for some
>>unknown reason it crashes sometimes if I remove addresses without
>>specifying the mask.
>  
> Interesting, do you use an iproute2 version with the wildcard
> address deletion fix attached below?

No, its some old debian version. I'm going to try if it makes a
difference, but it needs to be fixed to work (or at least not crash)
with old versions anyway.

Re: [PATCH] [IPV4] Fix secondary IP addresses after promotion

[Brian Pomerantz]

On Tue, Nov 08, 2005 at 03:11:15PM +0100, Patrick McHardy wrote:
> 
> Yes, fixing it correctly looks very hard. Just changing the routes
> doesn't seem right to me, someone might have added it with exactly
> this prefsrc and doesn't want it to change, its also not clear how
> to notify on this. Taking care of correct ordering of the ifa_list
> is also more complicated without just deleting and readding them.
> 
> I have a patch to do this, but it needs some debugging, for some
> unknown reason it crashes sometimes if I remove addresses without
> specifying the mask.

Looks like I'm back on this one because just sending the NETDEV_UP for
the secondaries didn't work if a primary other than the first one is
removed.  If you have anything that you need help testing/debugging,
I'm stuck with this until it is fixed.  I'd prefer not to duplicate
effort on this if you're close to a fix.  If not, then I'll try to
come up with something and toss it out for comment.


BAPper

Re: [PATCH] [IPV4] Fix secondary IP addresses after promotion

[Alexey Kuznetsov]

Hello!

> Local routes for 10.0.0.3 and 10.0.0.4 have disappeared _without_
> any notification.

Flushes do not generate notifications. The reason is technical: they
are usually massive, do overflow buffer, get lost and listeners have
to do painful resynchronization. The justification: they are useless
because these events are derived.

Alexey

Re: [PATCH] [IPV4] Fix secondary IP addresses after promotion

[Thomas Graf]

> > Local routes for 10.0.0.3 and 10.0.0.4 have disappeared _without_
> > any notification.
> 
> Flushes do not generate notifications. The reason is technical: they
> are usually massive, do overflow buffer, get lost and listeners have
> to do painful resynchronization. The justification: they are useless
> because these events are derived.

I perfectly agree, still I'm not happy with deleting the local routes
for the temporarly orphaned secondaries without notifications and
just re-add them again later.

I think we should either prevent the deletion of the local routes by
rewriting their preferred source address during promotion or explicitely
announce the temprary orphaned secondaries as down and up again in order
to have the local routes deleted/re-added in a clean way.