From: Alexander Bluhm <bluhm@openbsd.org>
Subject: Re: sysctl(2): make sysctl_int{,_lower}() mp-safe and unlock KERN_HOSTID
To: Vitaliy Makkoveev <mvs@openbsd.org>
Cc: tech@openbsd.org
Date: Wed, 14 Aug 2024 15:13:55 +0200

On Wed, Aug 14, 2024 at 02:50:41PM +0300, Vitaliy Makkoveev wrote:
> On Tue, Aug 13, 2024 at 02:24:34PM +0300, Vitaliy Makkoveev wrote:
> > Make sysctl_int() mp-safe in the sysctl_int_bounded() way and unlock
> > KERN_HOSTID. Except bootstrap called clockattach() of sparc64, `hostid'
> > never used outside kern_sysctl().
> > 
> > mp-safe sysctl_int() is meaningless for sysctl_int_lower(), so rework it
> > too. This time all affected paths are kernel locked, but this doesn't
> > make sysctl_int_lower() worse.
> > 
> > ok?
> > 
> 
> I want to change `hostid' type to the type of int. It only stored but
> never used within kernel, userland accesses it through sysctl_int().
> Nothing changes, but variable becomes consistent with sysctl_int().
> 
> Also, the only difference between sysctl_int() and sysctl_int_bounded()
> is the range check, so sysctl_int() is just sysctl_int_bounded(..., 
> INT_MIN, INT_MAX). sysctl_int() is not the fast path, so this useless
> check should not be significant.

OK bluhm@

> Index: sys/kern/kern_sysctl.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/kern_sysctl.c,v
> diff -u -p -r1.437 kern_sysctl.c
> --- sys/kern/kern_sysctl.c	11 Aug 2024 15:10:53 -0000	1.437
> +++ sys/kern/kern_sysctl.c	14 Aug 2024 11:46:41 -0000
> @@ -306,7 +306,7 @@ char hostname[MAXHOSTNAMELEN];
>  int hostnamelen;
>  char domainname[MAXHOSTNAMELEN];
>  int domainnamelen;
> -long hostid;
> +int hostid;
>  char *disknames = NULL;
>  size_t disknameslen;
>  struct diskstats *diskstats = NULL;
> @@ -507,6 +507,8 @@ kern_sysctl(int *name, u_int namelen, vo
>  		return (sysctl_rdstring(oldp, oldlenp, newp, version));
>  	case KERN_NUMVNODES:  /* XXX numvnodes is a long */
>  		return (sysctl_rdint(oldp, oldlenp, newp, numvnodes));
> +	case KERN_HOSTID:
> +		return (sysctl_int(oldp, oldlenp, newp, newlen, &hostid));
>  	case KERN_CLOCKRATE:
>  		return (sysctl_clockrate(oldp, oldlenp, newp));
>  	case KERN_BOOTTIME: {
> @@ -585,7 +587,7 @@ int
>  kern_sysctl_locked(int *name, u_int namelen, void *oldp, size_t *oldlenp,
>      void *newp, size_t newlen, struct proc *p)
>  {
> -	int error, level, inthostid, stackgap;
> +	int error, level, stackgap;
>  	dev_t dev;
>  	extern int pool_debug;
>  
> @@ -623,11 +625,6 @@ kern_sysctl_locked(int *name, u_int name
>  		if (newp && !error)
>  			domainnamelen = newlen;
>  		return (error);
> -	case KERN_HOSTID:
> -		inthostid = hostid;  /* XXX assumes sizeof long <= sizeof int */
> -		error =  sysctl_int(oldp, oldlenp, newp, newlen, &inthostid);
> -		hostid = inthostid;
> -		return (error);
>  	case KERN_CONSBUF:
>  		if ((error = suser(p)))
>  			return (error);
> @@ -1055,17 +1052,36 @@ int
>  sysctl_int_lower(void *oldp, size_t *oldlenp, void *newp, size_t newlen,
>      int *valp)
>  {
> -	unsigned int oval = *valp, val = *valp;
> +	unsigned int oldval, newval;
>  	int error;
>  
> -	if (newp == NULL)
> -		return (sysctl_rdint(oldp, oldlenp, newp, val));
> +	if (oldp && *oldlenp < sizeof(int))
> +		return (ENOMEM);
> +	if (newp && newlen != sizeof(int))
> +		return (EINVAL);
> +	*oldlenp = sizeof(int);
> +
> +	if (newp) {
> +		if ((error = copyin(newp, &newval, sizeof(int))))
> +			return (error);
> +		do {
> +			oldval = atomic_load_int(valp);
> +			if (oldval < (unsigned int)newval)
> +				return (EPERM);	/* do not allow raising */
> +		} while (atomic_cas_uint(valp, oldval, newval) != oldval);
> +
> +		if (oldp) {
> +			/* new value has been set although user gets error */
> +			if ((error = copyout(&oldval, oldp, sizeof(int))))
> +				return (error);
> +		}
> +	} else if (oldp) {
> +		oldval = atomic_load_int(valp);
> +
> +		if ((error = copyout(&oldval, oldp, sizeof(int))))
> +			return (error);	
> +	}
>  
> -	if ((error = sysctl_int(oldp, oldlenp, newp, newlen, &val)))
> -		return (error);
> -	if (val > oval)
> -		return (EPERM);		/* do not allow raising */
> -	*(unsigned int *)valp = val;
>  	return (0);
>  }
>  
> @@ -1076,18 +1092,8 @@ sysctl_int_lower(void *oldp, size_t *old
>  int
>  sysctl_int(void *oldp, size_t *oldlenp, void *newp, size_t newlen, int *valp)
>  {
> -	int error = 0;
> -
> -	if (oldp && *oldlenp < sizeof(int))
> -		return (ENOMEM);
> -	if (newp && newlen != sizeof(int))
> -		return (EINVAL);
> -	*oldlenp = sizeof(int);
> -	if (oldp)
> -		error = copyout(valp, oldp, sizeof(int));
> -	if (error == 0 && newp)
> -		error = copyin(newp, valp, sizeof(int));
> -	return (error);
> +	return (sysctl_int_bounded(oldp, oldlenp, newp, newlen, valp,
> +	    INT_MIN, INT_MAX));
>  }
>  
>  /*
> Index: sys/sys/kernel.h
> ===================================================================
> RCS file: /cvs/src/sys/sys/kernel.h,v
> diff -u -p -r1.26 kernel.h
> --- sys/sys/kernel.h	3 Mar 2023 20:16:44 -0000	1.26
> +++ sys/sys/kernel.h	14 Aug 2024 11:46:41 -0000
> @@ -40,7 +40,7 @@
>  /* Global variables for the kernel. */
>  
>  /* 1.1 */
> -extern long hostid;
> +extern int hostid;
>  extern char hostname[MAXHOSTNAMELEN];
>  extern int hostnamelen;
>  extern char domainname[MAXHOSTNAMELEN];