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