Hi,
If we introduce a mutex for the igmp module we can drop the NETLOCK
from igmp_slowtimo(). The router_info list, rti_head, and its entries
are only ever accessed within the igmp module. The mutex also needs
to protect igmp_timers_are_running.
This is largely a monkey-see-monkey-do imitation of what visa@ did for
frag6.c. Obviously any errors are mine.
There are spots in igmp_input_if() where we could push the mutex into
smaller sections of the switch statement. I'm under the impression that
IGMP is a cooler path, though, so that additional complexity is probably
pointless, but I figure it's worth mentioning. Maybe on a later pass.
I must admit I only have a small test setup for multipath routing. We
also don't have any regress tests for IGMP (no idea how tricky those
would be to write). The introduction of the mutex was pretty
straightforward, though, given the size of igmp.c and the fact that the
mutex is module-local.
Thoughts? ok?
-Scott
P.S. It doesn't look like we need KERNEL_LOCK in igmp_input_if() until
rip_input(). Is that fair game, or is there a larger plan to coordinate
moving KERNEL_LOCK into various *_input_if() routines later?
Index: sys/netinet/igmp.c
===================================================================
RCS file: /cvs/src/sys/netinet/igmp.c,v
retrieving revision 1.74
diff -u -p -r1.74 igmp.c
--- sys/netinet/igmp.c 18 Oct 2018 15:46:28 -0000 1.74
+++ sys/netinet/igmp.c 19 Oct 2018 17:07:31 -0000
@@ -1,4 +1,4 @@
-/* $OpenBSD: igmp.c,v 1.74 2018/10/18 15:46:28 cheloha Exp $ */
+/* $OpenBSD: igmp.c,v 1.73 2018/10/18 15:23:04 cheloha Exp $ */
/* $NetBSD: igmp.c,v 1.15 1996/02/13 23:41:25 christos Exp $ */
/*
@@ -77,6 +77,8 @@
#include <sys/param.h>
#include <sys/mbuf.h>
+#include <sys/mutex.h>
+#include <sys/queue.h>
#include <sys/systm.h>
#include <sys/socket.h>
#include <sys/protosw.h>
@@ -98,7 +100,13 @@
int *igmpctl_vars[IGMPCTL_MAXID] = IGMPCTL_VARS;
-int igmp_timers_are_running;
+/*
+ * Protects igmp_timers_are_running and rti_head. Note that rti_head needs
+ * to be protected when one of its entries is accessed via in_multi->rti.
+ */
+struct mutex igmp_mutex = MUTEX_INITIALIZER(IPL_SOFTNET);
+
+int igmp_timers_are_running;
static LIST_HEAD(, router_info) rti_head;
static struct mbuf *router_alert;
struct cpumem *igmpcounters;
@@ -150,6 +158,8 @@ rti_fill(struct in_multi *inm)
{
struct router_info *rti;
+ MUTEX_ASSERT_LOCKED(&igmp_mutex);
+
LIST_FOREACH(rti, &rti_head, rti_list) {
if (rti->rti_ifidx == inm->inm_ifidx) {
inm->inm_rti = rti;
@@ -176,6 +186,8 @@ rti_find(struct ifnet *ifp)
struct router_info *rti;
KERNEL_ASSERT_LOCKED();
+ MUTEX_ASSERT_LOCKED(&igmp_mutex);
+
LIST_FOREACH(rti, &rti_head, rti_list) {
if (rti->rti_ifidx == ifp->if_index)
return (rti);
@@ -195,13 +207,18 @@ rti_delete(struct ifnet *ifp)
{
struct router_info *rti, *trti;
+ mtx_enter(&igmp_mutex);
+
LIST_FOREACH_SAFE(rti, &rti_head, rti_list, trti) {
if (rti->rti_ifidx == ifp->if_index) {
LIST_REMOVE(rti, rti_list);
+ mtx_leave(&igmp_mutex);
free(rti, M_MRTABLE, sizeof(*rti));
- break;
+ return;
}
}
+
+ mtx_leave(&igmp_mutex);
}
int
@@ -271,6 +288,8 @@ igmp_input_if(struct ifnet *ifp, struct
m->m_len += iphlen;
ip = mtod(m, struct ip *);
+ mtx_enter(&igmp_mutex);
+
switch (igmp->igmp_type) {
case IGMP_HOST_MEMBERSHIP_QUERY:
@@ -282,6 +301,7 @@ igmp_input_if(struct ifnet *ifp, struct
if (igmp->igmp_code == 0) {
rti = rti_find(ifp);
if (rti == NULL) {
+ mtx_leave(&igmp_mutex);
m_freem(m);
return IPPROTO_DONE;
}
@@ -289,6 +309,7 @@ igmp_input_if(struct ifnet *ifp, struct
rti->rti_age = 0;
if (ip->ip_dst.s_addr != INADDR_ALLHOSTS_GROUP) {
+ mtx_leave(&igmp_mutex);
igmpstat_inc(igps_rcv_badqueries);
m_freem(m);
return IPPROTO_DONE;
@@ -314,6 +335,7 @@ igmp_input_if(struct ifnet *ifp, struct
}
} else {
if (!IN_MULTICAST(ip->ip_dst.s_addr)) {
+ mtx_leave(&igmp_mutex);
igmpstat_inc(igps_rcv_badqueries);
m_freem(m);
return IPPROTO_DONE;
@@ -371,6 +393,7 @@ igmp_input_if(struct ifnet *ifp, struct
if (!IN_MULTICAST(igmp->igmp_group.s_addr) ||
igmp->igmp_group.s_addr != ip->ip_dst.s_addr) {
+ mtx_leave(&igmp_mutex);
igmpstat_inc(igps_rcv_badreports);
m_freem(m);
return IPPROTO_DONE;
@@ -437,6 +460,7 @@ igmp_input_if(struct ifnet *ifp, struct
if (!IN_MULTICAST(igmp->igmp_group.s_addr) ||
igmp->igmp_group.s_addr != ip->ip_dst.s_addr) {
+ mtx_leave(&igmp_mutex);
igmpstat_inc(igps_rcv_badreports);
m_freem(m);
return IPPROTO_DONE;
@@ -484,6 +508,8 @@ igmp_input_if(struct ifnet *ifp, struct
}
+ mtx_leave(&igmp_mutex);
+
/*
* Pass all valid IGMP packets up to any process(es) listening
* on a raw IGMP socket.
@@ -503,14 +529,18 @@ igmp_joingroup(struct in_multi *inm)
if (!IN_LOCAL_GROUP(inm->inm_addr.s_addr) &&
ifp && (ifp->if_flags & IFF_LOOPBACK) == 0) {
- if ((i = rti_fill(inm)) == -1)
+ mtx_enter(&igmp_mutex);
+ if ((i = rti_fill(inm)) == -1) {
+ mtx_leave(&igmp_mutex);
goto out;
+ }
igmp_sendpkt(ifp, inm, i, 0);
inm->inm_state = IGMP_DELAYING_MEMBER;
inm->inm_timer = IGMP_RANDOM_DELAY(
IGMP_MAX_HOST_REPORT_DELAY * PR_FASTHZ);
igmp_timers_are_running = 1;
+ mtx_leave(&igmp_mutex);
} else
inm->inm_timer = 0;
@@ -529,11 +559,15 @@ igmp_leavegroup(struct in_multi *inm)
case IGMP_DELAYING_MEMBER:
case IGMP_IDLE_MEMBER:
if (!IN_LOCAL_GROUP(inm->inm_addr.s_addr) &&
- ifp && (ifp->if_flags & IFF_LOOPBACK) == 0)
- if (inm->inm_rti->rti_type != IGMP_v1_ROUTER)
+ ifp && (ifp->if_flags & IFF_LOOPBACK) == 0) {
+ mtx_enter(&igmp_mutex);
+ if (inm->inm_rti->rti_type != IGMP_v1_ROUTER) {
igmp_sendpkt(ifp, inm,
IGMP_HOST_LEAVE_MESSAGE,
INADDR_ALLROUTERS_GROUP);
+ }
+ mtx_leave(&igmp_mutex);
+ }
break;
case IGMP_LAZY_MEMBER:
case IGMP_AWAKENING_MEMBER:
@@ -549,6 +583,7 @@ igmp_fasttimo(void)
struct ifnet *ifp;
NET_LOCK();
+ mtx_enter(&igmp_mutex);
/*
* Quick check to see if any work needs to be done, in order
@@ -562,6 +597,7 @@ igmp_fasttimo(void)
igmp_checktimer(ifp);
out:
+ mtx_leave(&igmp_mutex);
NET_UNLOCK();
}
@@ -573,6 +609,7 @@ igmp_checktimer(struct ifnet *ifp)
struct ifmaddr *ifma;
NET_ASSERT_LOCKED();
+ MUTEX_ASSERT_LOCKED(&igmp_mutex);
TAILQ_FOREACH(ifma, &ifp->if_maddrlist, ifma_list) {
if (ifma->ifma_addr->sa_family != AF_INET)
@@ -601,7 +638,7 @@ igmp_slowtimo(void)
{
struct router_info *rti;
- NET_LOCK();
+ mtx_enter(&igmp_mutex);
LIST_FOREACH(rti, &rti_head, rti_list) {
if (rti->rti_type == IGMP_v1_ROUTER &&
@@ -610,7 +647,7 @@ igmp_slowtimo(void)
}
}
- NET_UNLOCK();
+ mtx_leave(&igmp_mutex);
}
void
