From: Vitaliy Makkoveev <mvs@openbsd.org>
Subject: sysctl(2): push locking down to icmp_sysctl()
To: Alexander Bluhm <bluhm@openbsd.org>, tech@openbsd.org
Date: Wed, 4 Dec 2024 22:04:33 +0300

Keep locking only for ICMPCTL_REDIRTIMEOUT case. It is complicated, so
left it as is.

ICMPCTL_STATS loads per-CPU counters into local data, so no locking
required.

`icmpctl_vars' are atomically accessed integers. Except `icmperrppslim'
they are simply booleans, so nothing special required. I used the local
`icmperrppslim_local' variable to load `icmperrppslim' value because it
it could have negative values. claudio@ proposed to always load such
values to local variables, so I want to try this notation with this
diff.

Index: sys/netinet/in_proto.c
===================================================================
RCS file: /cvs/src/sys/netinet/in_proto.c,v
diff -u -p -r1.113 in_proto.c
--- sys/netinet/in_proto.c	22 Aug 2024 10:58:31 -0000	1.113
+++ sys/netinet/in_proto.c	4 Dec 2024 18:55:34 -0000
@@ -219,7 +219,7 @@ const struct protosw inetsw[] = {
   .pr_type	= SOCK_RAW,
   .pr_domain	= &inetdomain,
   .pr_protocol	= IPPROTO_ICMP,
-  .pr_flags	= PR_ATOMIC|PR_ADDR|PR_MPSOCKET,
+  .pr_flags	= PR_ATOMIC|PR_ADDR|PR_MPSOCKET|PR_MPSYSCTL,
   .pr_input	= icmp_input,
   .pr_ctloutput	= rip_ctloutput,
   .pr_usrreqs	= &rip_usrreqs,
Index: sys/netinet/ip_icmp.c
===================================================================
RCS file: /cvs/src/sys/netinet/ip_icmp.c,v
diff -u -p -r1.196 ip_icmp.c
--- sys/netinet/ip_icmp.c	14 Jul 2024 18:53:39 -0000	1.196
+++ sys/netinet/ip_icmp.c	4 Dec 2024 18:55:34 -0000
@@ -105,16 +105,21 @@
  * host table maintenance routines.
  */
 
+/*
+ * Locks used to protect data:
+ *	a	atomic
+ */
+
 #ifdef ICMPPRINTFS
 int	icmpprintfs = 0;	/* Settable from ddb */
 #endif
 
 /* values controllable via sysctl */
-int	icmpmaskrepl = 0;
-int	icmpbmcastecho = 0;
-int	icmptstamprepl = 1;
-int	icmperrppslim = 100;
-int	icmp_rediraccept = 0;
+int	icmpmaskrepl = 0;		/* [a] */
+int	icmpbmcastecho = 0;		/* [a] */
+int	icmptstamprepl = 1;		/* [a] */
+int	icmperrppslim = 100;		/* [a] */
+int	icmp_rediraccept = 0;		/* [a] */
 int	icmp_redirtimeout = 10 * 60;
 
 static int icmperrpps_count = 0;
@@ -506,7 +511,7 @@ icmp_input_if(struct ifnet *ifp, struct 
 		break;
 
 	case ICMP_ECHO:
-		if (!icmpbmcastecho &&
+		if (atomic_load_int(&icmpbmcastecho) == 0 &&
 		    (m->m_flags & (M_MCAST | M_BCAST)) != 0) {
 			icmpstat_inc(icps_bmcastecho);
 			break;
@@ -515,10 +520,10 @@ icmp_input_if(struct ifnet *ifp, struct 
 		goto reflect;
 
 	case ICMP_TSTAMP:
-		if (icmptstamprepl == 0)
+		if (atomic_load_int(&icmptstamprepl) == 0)
 			break;
 
-		if (!icmpbmcastecho &&
+		if (atomic_load_int(&icmpbmcastecho) == 0 &&
 		    (m->m_flags & (M_MCAST | M_BCAST)) != 0) {
 			icmpstat_inc(icps_bmcastecho);
 			break;
@@ -533,7 +538,7 @@ icmp_input_if(struct ifnet *ifp, struct 
 		goto reflect;
 
 	case ICMP_MASKREQ:
-		if (icmpmaskrepl == 0)
+		if (atomic_load_int(&icmpmaskrepl) == 0)
 			break;
 		if (icmplen < ICMP_MASKLEN) {
 			icmpstat_inc(icps_badlen);
@@ -590,7 +595,7 @@ reflect:
 		struct rtentry *newrt = NULL;
 		int i_am_router = (atomic_load_int(&ip_forwarding) != 0);
 
-		if (icmp_rediraccept == 0 || i_am_router)
+		if (atomic_load_int(&icmp_rediraccept) == 0 || i_am_router)
 			goto freeit;
 		if (code > 3)
 			goto badcode;
@@ -884,24 +889,27 @@ icmp_sysctl(int *name, u_int namelen, vo
 		return (ENOTDIR);
 
 	switch (name[0]) {
-	case ICMPCTL_REDIRTIMEOUT:
+	case ICMPCTL_REDIRTIMEOUT: {
+		size_t savelen = *oldlenp;
+
+		if ((error = sysctl_vslock(oldp, savelen)))
+			break;
 		NET_LOCK();
 		error = sysctl_int_bounded(oldp, oldlenp, newp, newlen,
 		    &icmp_redirtimeout, 0, INT_MAX);
 		rt_timer_queue_change(&icmp_redirect_timeout_q,
 		    icmp_redirtimeout);
 		NET_UNLOCK();
+		sysctl_vsunlock(oldp, savelen);
 		break;
-
+	}
 	case ICMPCTL_STATS:
 		error = icmp_sysctl_icmpstat(oldp, oldlenp, newp);
 		break;
 
 	default:
-		NET_LOCK();
 		error = sysctl_bounded_arr(icmpctl_vars, nitems(icmpctl_vars),
 		    name, namelen, oldp, oldlenp, newp, newlen);
-		NET_UNLOCK();
 		break;
 	}
 
@@ -1098,9 +1106,10 @@ icmp_mtudisc_timeout(struct rtentry *rt,
 int
 icmp_ratelimit(const struct in_addr *dst, const int type, const int code)
 {
+	int icmperrppslim_local = atomic_load_int(&icmperrppslim);
 	/* PPS limit */
 	if (!ppsratecheck(&icmperrppslim_last, &icmperrpps_count,
-	    icmperrppslim))
+	    icmperrppslim_local))
 		return 1;	/* The packet is subject to rate limit */
 	return 0;	/* okay to send */
 }