From: Alexander Bluhm <bluhm@openbsd.org>
Subject: Re: sysctl(2): push locking down to icmp_sysctl()
To: Vitaliy Makkoveev <mvs@openbsd.org>
Cc: tech@openbsd.org
Date: Wed, 4 Dec 2024 23:02:47 +0100

On Wed, Dec 04, 2024 at 10:04:33PM +0300, Vitaliy Makkoveev wrote:
> 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.

OK bluhm@

> 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 */
>  }