From: Alexander Bluhm <bluhm@openbsd.org>
Subject: Re: fstat(1) caused a weird hang on 7.7
To: Vitaliy Makkoveev <mvs@openbsd.org>
Cc: "H. Hartzer" <h@hartzer.sh>, tech@openbsd.org
Date: Thu, 29 May 2025 21:32:54 +0900

On Thu, May 29, 2025 at 01:28:27PM +0300, Vitaliy Makkoveev wrote:
> I suspect it stuck in malloc(M_WAITOK) allocation of `kf'. This sleeping
> malloc() happens with `sysctl_lock' held, so the whole sysctl layer
> became unusable. 

Probably, yes.  But we cannot be sure without ps axl output.

> Fortunately, we expect context switch in `allprocess'
> foreach loops, so we could keep kernel lock only around them and avoid
> sleeping memory allocation under `sysctl_lock'.

Correct, you fixed process loop sleeps by reference counting.

> Actually, we don't need to wire memory in the most sysctl() cases, so we
> could avoid `sysctl_lock' and rely on kernel lock only.

Less sysctl_lock may help for this deadlock.

OK bluhm@

> Index: sys/kern/kern_sysctl.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/kern_sysctl.c,v
> diff -u -p -r1.468 kern_sysctl.c
> --- sys/kern/kern_sysctl.c	9 May 2025 14:53:22 -0000	1.468
> +++ sys/kern/kern_sysctl.c	29 May 2025 10:25:00 -0000
> @@ -401,6 +401,8 @@ kern_sysctl_dirs(int top_name, int *name
>  	int error;
>  
>  	switch (top_name) {
> +	case KERN_FILE:
> +		return (sysctl_file(name, namelen, oldp, oldlenp, p));
>  	case KERN_MALLOCSTATS:
>  		return (sysctl_malloc(name, namelen, oldp, oldlenp,
>  		    newp, newlen, p));
> @@ -447,8 +449,6 @@ kern_sysctl_dirs_locked(int top_name, in
>  		     newp, newlen, oldp, oldlenp, p));
>  	case KERN_PROC_VMMAP:
>  		return (sysctl_proc_vmmap(name, namelen, oldp, oldlenp, p));
> -	case KERN_FILE:
> -		return (sysctl_file(name, namelen, oldp, oldlenp, p));
>  #endif
>  #if defined(GPROF) || defined(DDBPROF)
>  	case KERN_PROF:
> @@ -1776,8 +1776,11 @@ do {									\
>  					if (af == AF_INET || af == AF_INET6)
>  						skip = 1;
>  				}
> -				if (!skip)
> +				if (!skip) {
> +					KERNEL_LOCK();
>  					FILLIT(fp, NULL, 0, NULL, NULL);
> +					KERNEL_UNLOCK();
> +				}
>  			}
>  		}
>  		break;
> @@ -1788,6 +1791,7 @@ do {									\
>  			break;
>  		}
>  		matched = 0;
> +		KERNEL_LOCK();
>  		LIST_FOREACH(pr, &allprocess, ps_list) {
>  			/*
>  			 * skip system, exiting, embryonic and undead
> @@ -1825,10 +1829,12 @@ do {									\
>  			if (arg >= 0)
>  				break;
>  		}
> +		KERNEL_UNLOCK();
>  		if (!matched)
>  			error = ESRCH;
>  		break;
>  	case KERN_FILE_BYUID:
> +		KERNEL_LOCK();
>  		LIST_FOREACH(pr, &allprocess, ps_list) {
>  			/*
>  			 * skip system, exiting, embryonic and undead
> @@ -1859,6 +1865,7 @@ do {									\
>  
>  			refcnt_rele_wake(&pr->ps_refcnt);
>  		}
> +		KERNEL_UNLOCK();
>  		break;
>  	default:
>  		error = EINVAL;