On 19/10/18(Fri) 12:15, Scott Cheloha wrote:
> 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.
Does the mutex only protect the next/previous pointer of "struct
router_info" or does it also serialize access to the other rti_* fields?
> 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?
Some comments inline :)
> 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?
Why don't we need it? Can you move it down?
> 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);
It the KERNEL_LOCK() still needed? When adding a new assertion, make
sure the older ones are still valid.
> +
> 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();
Is the NET_LOCK() still needed here? What for?
> + mtx_enter(&igmp_mutex);
Do you need to keep the mutex held before calling igmp_checktimer()?
Why? This can end up in ip_output()...
> /*
> * 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
