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

[Patrick McHardy]

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@xxxxxxxxx>

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)