From: Alexander Bluhm <bluhm@openbsd.org>
Subject: Re: sysctl(2): unlock carp_sysctl()
To: Vitaliy Makkoveev <mvs@openbsd.org>
Cc: Alexandr Nedvedicky <sashan@openbsd.org>, tech@openbsd.org
Date: Thu, 19 Dec 2024 22:06:40 +0100

On Thu, Dec 12, 2024 at 09:39:03PM +0300, Vitaliy Makkoveev wrote:
> This is the statistics implemented with per-CPU counters and atomically
> accessed integers.
> 
> I replaced the `carp_opts' array with individual `carpctl_*' variables
> and used sysctl_bounded_arr() instead former sysctl_int(). So, now we
> have no used elements of `carp_opts'. Also we could define the bounds
> for carp(4) controls. I intentionally left previous behaviour fro now,
> but in the future I like to fix them too.

OK bluhm@

carpctl_allow and carpctl_preempt should be boolean 0 or 1.

carpctl_log is beween LOG_EMERG and LOG_DEBUG, so 0 ... 7
Then it is actually unsigned and you don't need the (int) cast.

I think CARPCTL_MAXID can be removed, but we should make a full
build before.

> Index: sys/netinet/in_proto.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet/in_proto.c,v
> diff -u -p -r1.116 in_proto.c
> --- sys/netinet/in_proto.c	4 Dec 2024 22:48:41 -0000	1.116
> +++ sys/netinet/in_proto.c	12 Dec 2024 18:28:46 -0000
> @@ -331,7 +331,7 @@ const struct protosw inetsw[] = {
>    .pr_type	= SOCK_RAW,
>    .pr_domain	= &inetdomain,
>    .pr_protocol	= IPPROTO_CARP,
> -  .pr_flags	= PR_ATOMIC|PR_ADDR|PR_MPSOCKET,
> +  .pr_flags	= PR_ATOMIC|PR_ADDR|PR_MPSOCKET|PR_MPSYSCTL,
>    .pr_input	= carp_proto_input,
>    .pr_ctloutput	= rip_ctloutput,
>    .pr_usrreqs	= &rip_usrreqs,
> Index: sys/netinet/ip_carp.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet/ip_carp.c,v
> diff -u -p -r1.364 ip_carp.c
> --- sys/netinet/ip_carp.c	7 Dec 2024 01:14:45 -0000	1.364
> +++ sys/netinet/ip_carp.c	12 Dec 2024 18:28:46 -0000
> @@ -88,6 +88,11 @@
>  
>  #include <netinet/ip_carp.h>
>  
> +/*
> + * Locks used to protect data:
> + *	a	atomic
> + */
> +
>  struct carp_mc_entry {
>  	LIST_ENTRY(carp_mc_entry)	mc_entries;
>  	union {
> @@ -189,14 +194,23 @@ void	carp_sc_unref(void *, void *);
>  struct srpl_rc carp_sc_rc =
>      SRPL_RC_INITIALIZER(carp_sc_ref, carp_sc_unref, NULL);
>  
> -int carp_opts[CARPCTL_MAXID] = { 0, 1, 0, LOG_CRIT };	/* XXX for now */
> +int carpctl_allow = 1;		/* [a] */
> +int carpctl_preempt = 0;	/* [a] */
> +int carpctl_log = LOG_CRIT;	/* [a] */
> +
> +const struct sysctl_bounded_args carpctl_vars[] = {
> +	{CARPCTL_ALLOW, &carpctl_allow, INT_MIN, INT_MAX},
> +	{CARPCTL_PREEMPT, &carpctl_preempt, INT_MIN, INT_MAX},
> +	{CARPCTL_LOG, &carpctl_log, INT_MIN, INT_MAX},
> +};
> +
>  struct cpumem *carpcounters;
>  
>  int	carp_send_all_recur = 0;
>  
>  #define	CARP_LOG(l, sc, s)						\
>  	do {								\
> -		if (carp_opts[CARPCTL_LOG] >= l) {			\
> +		if ((int)atomic_load_int(&carpctl_log) >= l) {		\
>  			if (sc)						\
>  				log(l, "%s: ",				\
>  				    (sc)->sc_if.if_xname);		\
> @@ -451,7 +465,7 @@ carp_proto_input_if(struct ifnet *ifp, s
>  
>  	carpstat_inc(carps_ipackets);
>  
> -	if (!carp_opts[CARPCTL_ALLOW]) {
> +	if (!atomic_load_int(&carpctl_allow)) {
>  		m_freem(m);
>  		return IPPROTO_DONE;
>  	}
> @@ -550,7 +564,7 @@ carp6_proto_input_if(struct ifnet *ifp, 
>  
>  	carpstat_inc(carps_ipackets6);
>  
> -	if (!carp_opts[CARPCTL_ALLOW]) {
> +	if (!atomic_load_int(&carpctl_allow)) {
>  		m_freem(m);
>  		return IPPROTO_DONE;
>  	}
> @@ -707,7 +721,7 @@ carp_proto_input_c(struct ifnet *ifp, st
>  		 * and do not have a better demote count, treat them as down.
>  		 *
>  		 */
> -		if (carp_opts[CARPCTL_PREEMPT] &&
> +		if (atomic_load_int(&carpctl_preempt) &&
>  		    timercmp(&sc_tv, &ch_tv, <) &&
>  		    ch->carp_demote >= carp_group_demote_count(sc)) {
>  			carp_master_down(vhe);
> @@ -765,8 +779,6 @@ int
>  carp_sysctl(int *name, u_int namelen, void *oldp, size_t *oldlenp, void *newp,
>      size_t newlen)
>  {
> -	int error;
> -
>  	/* All sysctl names at this level are terminal. */
>  	if (namelen != 1)
>  		return (ENOTDIR);
> @@ -775,13 +787,8 @@ carp_sysctl(int *name, u_int namelen, vo
>  	case CARPCTL_STATS:
>  		return (carp_sysctl_carpstat(oldp, oldlenp, newp));
>  	default:
> -		if (name[0] <= 0 || name[0] >= CARPCTL_MAXID)
> -			return (ENOPROTOOPT);
> -		NET_LOCK();
> -		error = sysctl_int(oldp, oldlenp, newp, newlen,
> -		    &carp_opts[name[0]]);
> -		NET_UNLOCK();
> -		return (error);
> +		return (sysctl_bounded_arr(carpctl_vars, nitems(carpctl_vars),
> +		    name, namelen, oldp, oldlenp, newp, newlen));
>  	}
>  }
>  
> Index: sys/netinet6/in6_proto.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet6/in6_proto.c,v
> diff -u -p -r1.120 in6_proto.c
> --- sys/netinet6/in6_proto.c	4 Dec 2024 22:48:41 -0000	1.120
> +++ sys/netinet6/in6_proto.c	12 Dec 2024 18:28:46 -0000
> @@ -278,7 +278,7 @@ const struct protosw inet6sw[] = {
>    .pr_type	= SOCK_RAW,
>    .pr_domain	= &inet6domain,
>    .pr_protocol	= IPPROTO_CARP,
> -  .pr_flags	= PR_ATOMIC|PR_ADDR|PR_MPSOCKET,
> +  .pr_flags	= PR_ATOMIC|PR_ADDR|PR_MPSOCKET|PR_MPSYSCTL,
>    .pr_input	= carp6_proto_input,
>    .pr_ctloutput = rip6_ctloutput,
>    .pr_usrreqs	= &rip6_usrreqs,