From: Alexander Bluhm Subject: Re: unlock igmp slow timeout To: tech@openbsd.org Date: Wed, 12 Nov 2025 18:22:26 +0100 On Tue, Nov 11, 2025 at 10:49:59AM +0100, Alexander Bluhm wrote: > I would like to remove net lock from igmp_slowtimo(). Replace it > with a mutex that protects the router_info list. And here rebase, cleanup, fix race, add mutex. If you prefer to review the whole thing. ok? 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 16:29:23 -0000 @@ -77,6 +77,7 @@ #include #include +#include #include #include #include @@ -95,24 +96,32 @@ #define IP_MULTICASTOPTS 0 /* + * Locks used to protect global data and struct members: + * I immutable after creation + * a atomic + * G global igmp mutex igmp_mtx + */ + +/* * Per-interface router version information. */ struct router_info { - LIST_ENTRY(router_info) rti_list; - unsigned int rti_ifidx; - int rti_type; /* type of router on this interface */ - int rti_age; /* time since last v1 query */ + LIST_ENTRY(router_info) rti_list; /* [G] */ + unsigned int rti_ifidx; /* [I] */ + int rti_type; /* [G] type of router on interface */ + int rti_age; /* [G] time since last v1 query */ }; int igmp_timers_are_running; /* [a] shortcut for fast timer */ -static LIST_HEAD(, router_info) rti_head; +struct mutex igmp_mtx = MUTEX_INITIALIZER(IPL_SOFTNET); +static LIST_HEAD(, router_info) rti_head; /* [G] */ static struct mbuf *router_alert; 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,61 +156,95 @@ 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; + MUTEX_ASSERT_LOCKED(&igmp_mtx); + 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); +} - rti = malloc(sizeof(*rti), M_MRTABLE, M_WAITOK); +int +rti_fill(struct in_multi *inm) +{ + struct router_info *rti, *new_rti = NULL; + int type; + + mtx_enter(&igmp_mtx); + rti = rti_find(inm->inm_ifidx); + if (rti != NULL) + goto found; + mtx_leave(&igmp_mtx); + + new_rti = malloc(sizeof(*rti), M_MRTABLE, M_WAITOK); + + mtx_enter(&igmp_mtx); + /* check again after unlock and lock */ + rti = rti_find(inm->inm_ifidx); + if (rti != NULL) + 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; + mtx_leave(&igmp_mtx); + return (IGMP_v2_HOST_MEMBERSHIP_REPORT); + + found: + inm->inm_rti = rti; + type = rti->rti_type; + mtx_leave(&igmp_mtx); + + free(new_rti, M_MRTABLE, sizeof(*rti)); + return (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); - } + mtx_enter(&igmp_mtx); + rti = rti_find(ifp->if_index); + if (rti != NULL) + goto found; rti = malloc(sizeof(*rti), M_MRTABLE, M_NOWAIT); - if (rti == NULL) - return (NULL); + if (rti == NULL) { + mtx_leave(&igmp_mtx); + 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; + mtx_leave(&igmp_mtx); + + 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; - } - } + mtx_enter(&igmp_mtx); + rti = rti_find(ifp->if_index); + if (rti != NULL) + LIST_REMOVE(rti, rti_list); + mtx_leave(&igmp_mtx); + + free(rti, M_MRTABLE, sizeof(*rti)); } int @@ -236,9 +279,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 +323,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); @@ -609,16 +649,14 @@ igmp_slowtimo(void) { struct router_info *rti; - NET_LOCK(); - + mtx_enter(&igmp_mtx); LIST_FOREACH(rti, &rti_head, rti_list) { if (rti->rti_type == IGMP_v1_ROUTER && ++rti->rti_age >= IGMP_AGE_THRESHOLD) { rti->rti_type = IGMP_v2_ROUTER; } } - - NET_UNLOCK(); + mtx_leave(&igmp_mtx); } void