From: Alexander Bluhm <bluhm@openbsd.org>
Subject: igmp fix race and cleanup
To: tech@openbsd.org
Date: Wed, 12 Nov 2025 17:02:57 +0100

Hi,

Before making IGMP MP-safe I should fix existing races.  Note that
syzkaller finds random crashes I could not track down yet.

There is a race when rti_fill() it calls malloc() with M_WAITOK.
Before adding new entries, rti_fill() scans the rti list.  But while
malloc sleeps, a new entry could enter the list.  With duplicate
entries, rti_delete() does not behave properly.  That might explain
the crashes syzkaller has found in the past.

- Rename rti_find() to rti_reset() which changes the rtt as needed
  in the one place where it is called.
- Add new rti_find() that finds the rti based on the interface index.
- Use new rti_find() in rti_fill(), rti_reset(), rti_delete().
- Fix sleep race in rti_fill() by re-scannig the list.

Maybe the net lock already prevented the sleeping race, but it is
quite unclear whether kernel or net lock is responsible for rti_head.
This will be addressed in my next diff.

ok?

bluhm

Index: netinet/igmp.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/igmp.c,v
diff -u -p -r1.90 igmp.c
--- netinet/igmp.c	12 Nov 2025 11:37:08 -0000	1.90
+++ netinet/igmp.c	12 Nov 2025 15:57:21 -0000
@@ -112,7 +112,7 @@ struct cpumem *igmpcounters;
 int igmp_checktimer(struct ifnet *);
 void igmp_sendpkt(struct ifnet *, struct in_multi *, int, in_addr_t);
 int rti_fill(struct in_multi *);
-struct router_info * rti_find(struct ifnet *);
+int rti_reset(struct ifnet *);
 int igmp_input_if(struct ifnet *, struct mbuf **, int *, int, int,
     struct netstack *);
 int igmp_sysctl_igmpstat(void *, size_t *, void *);
@@ -147,60 +147,76 @@ igmp_init(void)
 	router_alert->m_len = sizeof(ra->ipopt_dst) + ra->ipopt_list[1];
 }
 
-int
-rti_fill(struct in_multi *inm)
+static struct router_info *
+rti_find(unsigned int ifidx)
 {
 	struct router_info *rti;
 
 	LIST_FOREACH(rti, &rti_head, rti_list) {
-		if (rti->rti_ifidx == inm->inm_ifidx) {
-			inm->inm_rti = rti;
-			if (rti->rti_type == IGMP_v1_ROUTER)
-				return (IGMP_v1_HOST_MEMBERSHIP_REPORT);
-			else
-				return (IGMP_v2_HOST_MEMBERSHIP_REPORT);
-		}
+		if (rti->rti_ifidx == ifidx)
+			return (rti);
 	}
+	return (NULL);
+}
+
+int
+rti_fill(struct in_multi *inm)
+{
+	struct router_info *rti, *new_rti;
 
-	rti = malloc(sizeof(*rti), M_MRTABLE, M_WAITOK);
+	rti = rti_find(inm->inm_ifidx);
+	if (rti != NULL)
+		goto found;
+
+	new_rti = malloc(sizeof(*rti), M_MRTABLE, M_WAITOK);
+	/* check again after potential sleep */
+	rti = rti_find(inm->inm_ifidx);
+	if (rti != NULL) {
+		free(new_rti, M_MRTABLE, sizeof(*rti));
+		goto found;
+	}
+	rti = new_rti;
 	rti->rti_ifidx = inm->inm_ifidx;
 	rti->rti_type = IGMP_v2_ROUTER;
 	LIST_INSERT_HEAD(&rti_head, rti, rti_list);
 	inm->inm_rti = rti;
 	return (IGMP_v2_HOST_MEMBERSHIP_REPORT);
+
+ found:
+	inm->inm_rti = rti;
+	return (rti->rti_type == IGMP_v1_ROUTER ?
+	    IGMP_v1_HOST_MEMBERSHIP_REPORT : IGMP_v2_HOST_MEMBERSHIP_REPORT);
 }
 
-struct router_info *
-rti_find(struct ifnet *ifp)
+int
+rti_reset(struct ifnet *ifp)
 {
 	struct router_info *rti;
 
-	KERNEL_ASSERT_LOCKED();
-	LIST_FOREACH(rti, &rti_head, rti_list) {
-		if (rti->rti_ifidx == ifp->if_index)
-			return (rti);
-	}
+	rti = rti_find(ifp->if_index);
+	if (rti != NULL)
+		goto found;
 
 	rti = malloc(sizeof(*rti), M_MRTABLE, M_NOWAIT);
 	if (rti == NULL)
-		return (NULL);
+		return (ENOBUFS);
 	rti->rti_ifidx = ifp->if_index;
-	rti->rti_type = IGMP_v2_ROUTER;
 	LIST_INSERT_HEAD(&rti_head, rti, rti_list);
-	return (rti);
+ found:
+	rti->rti_type = IGMP_v1_ROUTER;
+	rti->rti_age = 0;
+	return (0);
 }
 
 void
 rti_delete(struct ifnet *ifp)
 {
-	struct router_info *rti, *trti;
+	struct router_info *rti;
 
-	LIST_FOREACH_SAFE(rti, &rti_head, rti_list, trti) {
-		if (rti->rti_ifidx == ifp->if_index) {
-			LIST_REMOVE(rti, rti_list);
-			free(rti, M_MRTABLE, sizeof(*rti));
-			break;
-		}
+	rti = rti_find(ifp->if_index);
+	if (rti != NULL) {
+		LIST_REMOVE(rti, rti_list);
+		free(rti, M_MRTABLE, sizeof(*rti));
 	}
 }
 
@@ -236,9 +252,8 @@ igmp_input_if(struct ifnet *ifp, struct 
 	int minlen;
 	struct ifmaddr *ifma;
 	struct in_multi *inm;
-	struct router_info *rti;
 	struct in_ifaddr *ia;
-	int timer, running = 0;
+	int error, timer, running = 0;
 
 	igmplen = ntohs(ip->ip_len) - iphlen;
 
@@ -281,13 +296,11 @@ igmp_input_if(struct ifnet *ifp, struct 
 			break;
 
 		if (igmp->igmp_code == 0) {
-			rti = rti_find(ifp);
-			if (rti == NULL) {
+			error = rti_reset(ifp);
+			if (error) {
 				m_freem(m);
 				return IPPROTO_DONE;
 			}
-			rti->rti_type = IGMP_v1_ROUTER;
-			rti->rti_age = 0;
 
 			if (ip->ip_dst.s_addr != INADDR_ALLHOSTS_GROUP) {
 				igmpstat_inc(igps_rcv_badqueries);