I found that zebra was crashing with a segfault and had a trashed
stack. I traced the problem to zebra/kernel_socket:ifm_read() which
assumed that a struct if_msghdr was always followed by a sockaddr_dl.
NetBSD sends if_msghdr with status on interface change, and there were
two problems:
1) ifm_read was copying junk from after the ifm_read using a length
read from the junk.
2) ifm_read was failing to match the if_msghdr with a struct
interface, and thus failing to cause the interface to go up and
down.
With the following patch zebra does not crash, and doing 'ifconfig
foo0 down' causes the routes to foo0 (which are sent via 'redistribute
connected') to be withdrawn over OSPF and RIPng. ifconfig up causes
the routes to be restored.
On NetBSD, ifm_read is apparently not used for getting the initial
interface list.
I plan to commit this soon if it runs stably for a while.
Comments?
Index: kernel_socket.c
===================================================================
RCS file: /var/cvsroot/quagga/zebra/kernel_socket.c,v
retrieving revision 1.10
diff -u -r1.10 kernel_socket.c
--- kernel_socket.c 22 Oct 2003 02:51:38 -0000 1.10
+++ kernel_socket.c 4 Jan 2004 01:04:22 -0000
@@ -211,7 +211,11 @@
}
#endif /* RTM_IFANNOUNCE */
-/* Interface adding function called from interface_list. */
+/*
+ * Handle struct if_msghdr obtained from reading routing socket or
+ * sysctl (from interface_list).
+ * There may or may not be sockaddrs present after the header.
+ */
int
ifm_read (struct if_msghdr *ifm)
{
@@ -219,6 +223,17 @@
struct sockaddr_dl *sdl = NULL;
char ifname[IFNAMSIZ];
+ /* paranoia - should not happen */
+ if (ifm->ifm_msglen < sizeof(struct if_msghdr))
+ {
+ zlog_err ("ifm_read: ifm->ifm_msglen %d too short\n",
+ ifm->ifm_msglen);
+ return -1;
+ }
+
+ /*
+ * Check for a sockaddr_dl following the message.
+ */
#ifdef SUNOS_5
int i;
struct sockaddr *sa;
@@ -247,32 +262,64 @@
}
}
#else
- sdl = (struct sockaddr_dl *)(ifm + 1);
+ /* sockaddrs_present? */
+ if (ifm->ifm_addrs)
+ {
+ if (ifm->ifm_addrs == RTA_IFP)
+ {
+ /* just the one we want */
+ sdl = (struct sockaddr_dl *)(ifm + 1);
+ }
+ else
+ {
+ /* Perhaps not an error, but more complicated parsing is needed. */
+ zlog_err ("ifm_read: ifm_addrs != 0 != RTA_IFP %x\n",
+ ifm->ifm_addrs);
+ return -1;
+ }
+ }
#endif
/*
+ * Look up on ifindex. This is useful if this is an up/down
+ * notification for an interface of which we are already aware.
+ * (This happens on NetBSD 1.6.2, for example.)
+ */
+ if (ifp == NULL)
+ ifp = if_lookup_by_index (ifm->ifm_index);
+
+ /*
* Check if ifp already exists. If the interface has already been specified
* in the conf file, but is just getting created, we would have an
* entry in the iflist with incomplete data (e.g., ifindex == -1),
* so we lookup on name.
*/
- if (sdl != NULL)
+ if (ifp != NULL && sdl != NULL)
{
- memcpy (ifname, sdl->sdl_data, sdl->sdl_nlen);
- ifname[sdl->sdl_nlen] = '\0';
+ /* Avoid relying on sdl->sdl_nlen. */
+ bzero(ifname, sizeof(ifname));
+ memcpy (ifname, sdl->sdl_data, sizeof(ifname)-1);
ifp = if_lookup_by_name (ifname);
}
+ /* New interface? */
if ((ifp == NULL) || (ifp->ifindex == -1))
{
/* Check interface's address.*/
if (! (ifm->ifm_addrs & RTA_IFP))
{
- zlog_warn ("There must be RTA_IFP address for ifindex %d\n",
+ zlog_warn ("ifindex %d: RTA_IFP sockaddr required for new interface\n",
ifm->ifm_index);
return -1;
}
+ if (sdl == NULL)
+ {
+ /* Should not happen with RTA_IFP check. */
+ zlog_warn ("ifm_read: no sockaddr_dl present");
+ return -1;
+ }
+
if (ifp == NULL)
ifp = if_create (sdl->sdl_data, sdl->sdl_nlen);
@@ -296,6 +343,7 @@
if_add_update (ifp);
}
else
+ /* Flag change notification on existing interface. */
{
/* There is a case of promisc, allmulti flag modification. */
if (if_is_up (ifp))
@@ -823,6 +871,12 @@
rtmsg_debug (&buf.r.rtm);
rtm = &buf.r.rtm;
+
+ if (rtm->rtm_msglen != nbytes)
+ {
+ zlog_warn ("kernel_read: rtm->rtm_msglen %d, nbytes %d, type %d\n",
+ rtm->rtm_msglen, nbytes, rtm->rtm_type);
+ }
switch (rtm->rtm_type)
{
stack. I traced the problem to zebra/kernel_socket:ifm_read() which
assumed that a struct if_msghdr was always followed by a sockaddr_dl.
NetBSD sends if_msghdr with status on interface change, and there were
two problems:
1) ifm_read was copying junk from after the ifm_read using a length
read from the junk.
2) ifm_read was failing to match the if_msghdr with a struct
interface, and thus failing to cause the interface to go up and
down.
With the following patch zebra does not crash, and doing 'ifconfig
foo0 down' causes the routes to foo0 (which are sent via 'redistribute
connected') to be withdrawn over OSPF and RIPng. ifconfig up causes
the routes to be restored.
On NetBSD, ifm_read is apparently not used for getting the initial
interface list.
I plan to commit this soon if it runs stably for a while.
Comments?
Index: kernel_socket.c
===================================================================
RCS file: /var/cvsroot/quagga/zebra/kernel_socket.c,v
retrieving revision 1.10
diff -u -r1.10 kernel_socket.c
--- kernel_socket.c 22 Oct 2003 02:51:38 -0000 1.10
+++ kernel_socket.c 4 Jan 2004 01:04:22 -0000
@@ -211,7 +211,11 @@
}
#endif /* RTM_IFANNOUNCE */
-/* Interface adding function called from interface_list. */
+/*
+ * Handle struct if_msghdr obtained from reading routing socket or
+ * sysctl (from interface_list).
+ * There may or may not be sockaddrs present after the header.
+ */
int
ifm_read (struct if_msghdr *ifm)
{
@@ -219,6 +223,17 @@
struct sockaddr_dl *sdl = NULL;
char ifname[IFNAMSIZ];
+ /* paranoia - should not happen */
+ if (ifm->ifm_msglen < sizeof(struct if_msghdr))
+ {
+ zlog_err ("ifm_read: ifm->ifm_msglen %d too short\n",
+ ifm->ifm_msglen);
+ return -1;
+ }
+
+ /*
+ * Check for a sockaddr_dl following the message.
+ */
#ifdef SUNOS_5
int i;
struct sockaddr *sa;
@@ -247,32 +262,64 @@
}
}
#else
- sdl = (struct sockaddr_dl *)(ifm + 1);
+ /* sockaddrs_present? */
+ if (ifm->ifm_addrs)
+ {
+ if (ifm->ifm_addrs == RTA_IFP)
+ {
+ /* just the one we want */
+ sdl = (struct sockaddr_dl *)(ifm + 1);
+ }
+ else
+ {
+ /* Perhaps not an error, but more complicated parsing is needed. */
+ zlog_err ("ifm_read: ifm_addrs != 0 != RTA_IFP %x\n",
+ ifm->ifm_addrs);
+ return -1;
+ }
+ }
#endif
/*
+ * Look up on ifindex. This is useful if this is an up/down
+ * notification for an interface of which we are already aware.
+ * (This happens on NetBSD 1.6.2, for example.)
+ */
+ if (ifp == NULL)
+ ifp = if_lookup_by_index (ifm->ifm_index);
+
+ /*
* Check if ifp already exists. If the interface has already been specified
* in the conf file, but is just getting created, we would have an
* entry in the iflist with incomplete data (e.g., ifindex == -1),
* so we lookup on name.
*/
- if (sdl != NULL)
+ if (ifp != NULL && sdl != NULL)
{
- memcpy (ifname, sdl->sdl_data, sdl->sdl_nlen);
- ifname[sdl->sdl_nlen] = '\0';
+ /* Avoid relying on sdl->sdl_nlen. */
+ bzero(ifname, sizeof(ifname));
+ memcpy (ifname, sdl->sdl_data, sizeof(ifname)-1);
ifp = if_lookup_by_name (ifname);
}
+ /* New interface? */
if ((ifp == NULL) || (ifp->ifindex == -1))
{
/* Check interface's address.*/
if (! (ifm->ifm_addrs & RTA_IFP))
{
- zlog_warn ("There must be RTA_IFP address for ifindex %d\n",
+ zlog_warn ("ifindex %d: RTA_IFP sockaddr required for new interface\n",
ifm->ifm_index);
return -1;
}
+ if (sdl == NULL)
+ {
+ /* Should not happen with RTA_IFP check. */
+ zlog_warn ("ifm_read: no sockaddr_dl present");
+ return -1;
+ }
+
if (ifp == NULL)
ifp = if_create (sdl->sdl_data, sdl->sdl_nlen);
@@ -296,6 +343,7 @@
if_add_update (ifp);
}
else
+ /* Flag change notification on existing interface. */
{
/* There is a case of promisc, allmulti flag modification. */
if (if_is_up (ifp))
@@ -823,6 +871,12 @@
rtmsg_debug (&buf.r.rtm);
rtm = &buf.r.rtm;
+
+ if (rtm->rtm_msglen != nbytes)
+ {
+ zlog_warn ("kernel_read: rtm->rtm_msglen %d, nbytes %d, type %d\n",
+ rtm->rtm_msglen, nbytes, rtm->rtm_type);
+ }
switch (rtm->rtm_type)
{