From: Vitaliy Makkoveev Subject: Re: igmp fix race and cleanup To: Alexander Bluhm Cc: tech@openbsd.org Date: Thu, 13 Nov 2025 18:19:34 +0300 On Thu, Nov 13, 2025 at 12:57:47PM +0100, Alexander Bluhm wrote: > On Thu, Nov 13, 2025 at 01:04:50AM +0300, Vitaliy Makkoveev wrote: > > On Wed, Nov 12, 2025 at 05:02:57PM +0100, Alexander Bluhm wrote: > > > 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? > > > > > > > You don't need 'goto found' in rti_reset(). The code can be pushed into > > "if (rti != NULL)" block. The rest of the diff looks fine to me. > > In the next diff I will also put the mutex leave there. Then it > is more common code between the rti = rti_find() and rti = malloc() > case. > > In practice it makes not much difference and I moved the code around > for a while to come to the current version. I can change if you > see a real advantage. > No problem, feel free to commit this diff as is. > 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); > > > >