Hi, syzkaller has found this race in arp.
https://syzkaller.appspot.com/bug?id=e3dc94533ddee95b6d69c2e7049360022f4190d3 The assumption of the code is that either the arp entry or the interface is removed. But in if_detach() if_remove() is called without net lock and all arp entries are removed later in in_ifdetach() -> in_purgeaddr() -> rt_ifa_purge() -> rtdeletemsg(). When the arp timeout fires while if_detach() is between if_remove() and NET_LOCK() then arptfree() has do deal with partially destroyed interfaces. We can skip rtdeletemsg() as if_detach() will take care of it. While syzkaller has not found it, nd6 has to deal with the same problem. Make nd6_free() simmilar to arptfree(). This crash may have the same source of problem. https://syzkaller.appspot.com/bug?id=9649f7319437a49298a38572b83f38f0b7d37fbe if_detach() does if_remove(ifp); NET_LOCK(); rti_delete(). So new igmp groups may appear during interface destruction. igmp_joingroup() does not call rti_fill() as if_get() fails. Then inm->inm_rti may be NULL. This is the condition when syzkaller crashes in igmp_leavegroup(). When we pass the ifp this CPU is already holding, we avoid half constructed igmp groups. Calling if_get() multiple times in caller and callee makes no sense anyway. ok? partial ok for one of the fixes also welcome. bluhm Index: netinet/if_ether.c =================================================================== RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/if_ether.c,v retrieving revision 1.248 diff -u -p -r1.248 if_ether.c --- netinet/if_ether.c 28 Apr 2021 21:21:44 -0000 1.248 +++ netinet/if_ether.c 3 Mar 2022 23:31:55 -0000 @@ -722,7 +722,9 @@ arptfree(struct rtentry *rt) arpinvalidate(rt); ifp = if_get(rt->rt_ifidx); - KASSERT(ifp != NULL); + if (ifp == NULL) + return; + if (!ISSET(rt->rt_flags, RTF_STATIC|RTF_CACHED)) rtdeletemsg(rt, ifp, ifp->if_rdomain); if_put(ifp); Index: netinet/igmp.c =================================================================== RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/igmp.c,v retrieving revision 1.77 diff -u -p -r1.77 igmp.c --- netinet/igmp.c 15 Dec 2021 15:58:01 -0000 1.77 +++ netinet/igmp.c 3 Mar 2022 23:58:32 -0000 @@ -483,17 +483,14 @@ igmp_input_if(struct ifnet *ifp, struct } void -igmp_joingroup(struct in_multi *inm) +igmp_joingroup(struct in_multi *inm, struct ifnet *ifp) { - struct ifnet* ifp; int i; - ifp = if_get(inm->inm_ifidx); - inm->inm_state = IGMP_IDLE_MEMBER; if (!IN_LOCAL_GROUP(inm->inm_addr.s_addr) && - ifp && (ifp->if_flags & IFF_LOOPBACK) == 0) { + (ifp->if_flags & IFF_LOOPBACK) == 0) { i = rti_fill(inm); igmp_sendpkt(ifp, inm, i, 0); inm->inm_state = IGMP_DELAYING_MEMBER; @@ -502,22 +499,16 @@ igmp_joingroup(struct in_multi *inm) igmp_timers_are_running = 1; } else inm->inm_timer = 0; - - if_put(ifp); } void -igmp_leavegroup(struct in_multi *inm) +igmp_leavegroup(struct in_multi *inm, struct ifnet *ifp) { - struct ifnet* ifp; - - ifp = if_get(inm->inm_ifidx); - switch (inm->inm_state) { case IGMP_DELAYING_MEMBER: case IGMP_IDLE_MEMBER: if (!IN_LOCAL_GROUP(inm->inm_addr.s_addr) && - ifp && (ifp->if_flags & IFF_LOOPBACK) == 0) + (ifp->if_flags & IFF_LOOPBACK) == 0) if (inm->inm_rti->rti_type != IGMP_v1_ROUTER) igmp_sendpkt(ifp, inm, IGMP_HOST_LEAVE_MESSAGE, @@ -528,7 +519,6 @@ igmp_leavegroup(struct in_multi *inm) case IGMP_SLEEPING_MEMBER: break; } - if_put(ifp); } void Index: netinet/igmp_var.h =================================================================== RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/igmp_var.h,v retrieving revision 1.14 diff -u -p -r1.14 igmp_var.h --- netinet/igmp_var.h 17 Aug 2020 16:25:34 -0000 1.14 +++ netinet/igmp_var.h 4 Mar 2022 00:02:45 -0000 @@ -107,8 +107,8 @@ igmpstat_inc(enum igmpstat_counters c) void igmp_init(void); int igmp_input(struct mbuf **, int *, int, int); -void igmp_joingroup(struct in_multi *); -void igmp_leavegroup(struct in_multi *); +void igmp_joingroup(struct in_multi *, struct ifnet *); +void igmp_leavegroup(struct in_multi *, struct ifnet *); void igmp_fasttimo(void); void igmp_slowtimo(void); int igmp_sysctl(int *, u_int, void *, size_t *, void *, size_t); Index: netinet/in.c =================================================================== RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/in.c,v retrieving revision 1.171 diff -u -p -r1.171 in.c --- netinet/in.c 10 Mar 2021 10:21:48 -0000 1.171 +++ netinet/in.c 3 Mar 2022 23:57:49 -0000 @@ -894,7 +894,7 @@ in_addmulti(struct in_addr *ap, struct i /* * Let IGMP know that we have joined a new IP multicast group. */ - igmp_joingroup(inm); + igmp_joingroup(inm, ifp); } return (inm); @@ -911,35 +911,34 @@ in_delmulti(struct in_multi *inm) NET_ASSERT_LOCKED(); - if (--inm->inm_refcnt == 0) { + if (--inm->inm_refcnt != 0) + return; + + ifp = if_get(inm->inm_ifidx); + if (ifp != NULL) { /* * No remaining claims to this record; let IGMP know that * we are leaving the multicast group. */ - igmp_leavegroup(inm); - ifp = if_get(inm->inm_ifidx); + igmp_leavegroup(inm, ifp); /* * Notify the network driver to update its multicast * reception filter. */ - if (ifp != NULL) { - memset(&ifr, 0, sizeof(ifr)); - satosin(&ifr.ifr_addr)->sin_len = - sizeof(struct sockaddr_in); - satosin(&ifr.ifr_addr)->sin_family = AF_INET; - satosin(&ifr.ifr_addr)->sin_addr = inm->inm_addr; - KERNEL_LOCK(); - (*ifp->if_ioctl)(ifp, SIOCDELMULTI, (caddr_t)&ifr); - KERNEL_UNLOCK(); - - TAILQ_REMOVE(&ifp->if_maddrlist, &inm->inm_ifma, - ifma_list); - } - if_put(ifp); + memset(&ifr, 0, sizeof(ifr)); + satosin(&ifr.ifr_addr)->sin_len = sizeof(struct sockaddr_in); + satosin(&ifr.ifr_addr)->sin_family = AF_INET; + satosin(&ifr.ifr_addr)->sin_addr = inm->inm_addr; + KERNEL_LOCK(); + (*ifp->if_ioctl)(ifp, SIOCDELMULTI, (caddr_t)&ifr); + KERNEL_UNLOCK(); - free(inm, M_IPMADDR, sizeof(*inm)); + TAILQ_REMOVE(&ifp->if_maddrlist, &inm->inm_ifma, ifma_list); } + if_put(ifp); + + free(inm, M_IPMADDR, sizeof(*inm)); } /* Index: netinet6/nd6.c =================================================================== RCS file: /data/mirror/openbsd/cvs/src/sys/netinet6/nd6.c,v retrieving revision 1.238 diff -u -p -r1.238 nd6.c --- netinet6/nd6.c 22 Feb 2022 01:15:02 -0000 1.238 +++ netinet6/nd6.c 3 Mar 2022 23:38:56 -0000 @@ -716,7 +716,12 @@ nd6_free(struct rtentry *rt) NET_ASSERT_LOCKED(); + KASSERT(!ISSET(rt->rt_flags, RTF_LOCAL)); + nd6_invalidate(rt); + ifp = if_get(rt->rt_ifidx); + if (ifp == NULL) + return; if (!ip6_forwarding) { if (ln->ln_router) { @@ -729,9 +734,6 @@ nd6_free(struct rtentry *rt) } } - KASSERT(!ISSET(rt->rt_flags, RTF_LOCAL)); - nd6_invalidate(rt); - /* * Detach the route from the routing tree and the list of neighbor * caches, and disable the route entry not to be used in already @@ -739,7 +741,6 @@ nd6_free(struct rtentry *rt) */ if (!ISSET(rt->rt_flags, RTF_STATIC|RTF_CACHED)) rtdeletemsg(rt, ifp, ifp->if_rdomain); - if_put(ifp); }
