From: Alexander Bluhm Subject: Re: igmp fix race and cleanup To: Vitaliy Makkoveev Cc: tech@openbsd.org Date: Thu, 13 Nov 2025 12:57:47 +0100 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. 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); > >