Download raw body.
igmp fix race and cleanup
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);
igmp fix race and cleanup