[radvd-devel-l] Linux infinite loop if interface goes down

[Pekka Savola]

On Thu, 27 Aug 2009, Teemu Torma wrote:
> The issue is that when interface comes back up, send_ra will call
> reload_config which will reset timers for all interfaces.  However, the
> current timer event will continue processing as usual and at the end will
> reset the current timer.  Since reload_config already did that, timer list
> pointers will be screwed and at least in my case cause an infinite loop.
>
> I haven't tried this, but it might suffice to ignore already existing timer
> entry in set_timer (i.e, tm->next && tm->prev).
>
> I don't know the code well enough to say if this is a good way to deal with
> the issue.

I like this approach.  I tried to figure out a way to avoid timer list 
corruption otherwise, but couldn't come up with anything that'd work 
well.

I added a 'return 0' also to the branch where interface first failed 
and added some comments.  Reuben, could you test whether this works 
for you too?

-- 
Pekka Savola                 "You each name yourselves king, yet the
Netcore Oy                    kingdom bleeds."
Systems. Networks. Security. -- George R.R. Martin: A Clash of Kings
-------------- next part --------------
diff -ur radvd-1.4/process.c radvd-1.4.reload/process.c
--- radvd-1.4/process.c	2009-08-03 16:00:51.000000000 +0300
+++ radvd-1.4.reload/process.c	2009-08-28 12:19:53.000000000 +0300
@@ -205,10 +205,10 @@
 	else {
 		/* no RA sent in a while, send an immediate multicast reply */
 		clear_timer(&iface->tm);
-		send_ra_forall(sock, iface, NULL);
-		
-		next = rand_between(iface->MinRtrAdvInterval, iface->MaxRtrAdvInterval); 
-		set_timer(&iface->tm, next);
+		if (send_ra_forall(sock, iface, NULL) == 0) {
+			next = rand_between(iface->MinRtrAdvInterval, iface->MaxRtrAdvInterval); 
+			set_timer(&iface->tm, next);
+		}
 	}
 }
 
diff -ur radvd-1.4/radvd.c radvd-1.4.reload/radvd.c
--- radvd-1.4/radvd.c	2009-06-19 10:37:11.000000000 +0300
+++ radvd-1.4.reload/radvd.c	2009-08-28 12:19:53.000000000 +0300
@@ -339,7 +339,8 @@
 
 	dlog(LOG_DEBUG, 4, "timer_handler called for %s", iface->Name);
 
-	send_ra_forall(sock, iface, NULL);
+	if (send_ra_forall(sock, iface, NULL) != 0)
+		return;
 
 	next = rand_between(iface->MinRtrAdvInterval, iface->MaxRtrAdvInterval); 
 
@@ -389,13 +390,14 @@
 			break;
 
 		/* send an initial advertisement */
-		send_ra_forall(sock, iface, NULL);
+		if (send_ra_forall(sock, iface, NULL) == 0) {
 
-		iface->init_racount++;
+			iface->init_racount++;
 
-		set_timer(&iface->tm,
-			  min(MAX_INITIAL_RTR_ADVERT_INTERVAL,
-			      iface->MaxRtrAdvInterval));
+			set_timer(&iface->tm,
+				  min(MAX_INITIAL_RTR_ADVERT_INTERVAL,
+				      iface->MaxRtrAdvInterval));
+		}
 	}
 }
 
diff -ur radvd-1.4/radvd.h radvd-1.4.reload/radvd.h
--- radvd-1.4/radvd.h	2009-06-19 10:34:07.000000000 +0300
+++ radvd-1.4.reload/radvd.h	2009-08-28 12:19:53.000000000 +0300
@@ -217,8 +217,8 @@
 int open_icmpv6_socket(void);
 
 /* send.c */
-void send_ra(int, struct Interface *iface, struct in6_addr *dest);
-void send_ra_forall(int, struct Interface *iface, struct in6_addr *dest);
+int send_ra(int, struct Interface *iface, struct in6_addr *dest);
+int send_ra_forall(int, struct Interface *iface, struct in6_addr *dest);
 
 /* process.c */
 void process(int sock, struct Interface *, unsigned char *, int,
diff -ur radvd-1.4/send.c radvd-1.4.reload/send.c
--- radvd-1.4/send.c	2009-06-24 12:27:49.000000000 +0300
+++ radvd-1.4.reload/send.c	2009-08-28 12:35:07.000000000 +0300
@@ -26,16 +26,14 @@
  * address only, but only if it was configured.
  *
  */
-void
+int
 send_ra_forall(int sock, struct Interface *iface, struct in6_addr *dest)
 {
 	struct Clients *current;
 
 	/* If no list of clients was specified for this interface, we broadcast */
-	if (iface->ClientList == NULL) {
-		send_ra(sock, iface, dest);
-		return;
-	}
+	if (iface->ClientList == NULL)
+		return send_ra(sock, iface, dest);
 
 	/* If clients are configured, send the advertisement to all of them via unicast */
 	for (current = iface->ClientList; current; current = current->next)
@@ -53,10 +51,10 @@
 
 		/* If we should only send the RA to a specific address, we are done */
 		if (dest != NULL)
-			return;
+			return 0;
 	}
 	if (dest == NULL)
-		return;
+		return 0;
 
         /* If we refused a client's solicitation, log it if debugging is high enough */
 	char address_text[INET6_ADDRSTRLEN];
@@ -65,9 +63,10 @@
 		inet_ntop(AF_INET6, dest, address_text, INET6_ADDRSTRLEN);
 
 	dlog(LOG_DEBUG, 5, "Not answering request from %s, not configured", address_text);
+	return 0;
 }
 
-void
+int
 send_ra(int sock, struct Interface *iface, struct in6_addr *dest)
 {
 	uint8_t all_hosts_addr[] = {0xff,0x02,0,0,0,0,0,0,0,0,0,0,0,0,0,1};
@@ -96,13 +95,20 @@
 			flog(LOG_WARNING, "interface %s does not exist, ignoring the interface", iface->Name);
 		}
 		iface->HasFailed = 1;
+		/* not really a 'success', but we need to schedule new timers.. */
+		return 0;
 	} else {
 		/* check_device was successful, act if it has failed previously */
 		if (iface->HasFailed == 1) {
 			flog(LOG_WARNING, "interface %s seems to have come back up, trying to reinitialize", iface->Name);
 			iface->HasFailed = 0;
-			/* XXX: reinitializes 'iface', so this probably isn't going to work until next send_ra().. */
-			reload_config();	
+			/*
+			 * return -1 so timer_handler() doesn't schedule new timers,
+			 * reload_config() will kick off new timers anyway.  This avoids
+			 * timer list corruption.
+			 */
+			reload_config();
+			return -1;
 		}
 	}
 
@@ -375,4 +381,6 @@
 		else
 			dlog(LOG_DEBUG, 3, "sendmsg: %s", strerror(errno));
 	}
+
+	return 0;
 }