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