From: Jonathan Gray <jsg@jsg.id.au>
Subject: Re: pool: Check that we can sleep early
To: Christian Ludwig <cludwig@genua.de>
Cc: tech@openbsd.org
Date: Fri, 1 Aug 2025 00:20:33 +1000

On Thu, Jul 31, 2025 at 04:01:12PM +0200, Christian Ludwig wrote:
> Hi,
> 
> bluhm@ has hit a bug earlier ([1]) that panics pretty late, after a
> context switch to a different thread. The resulting backtrace is rather
> useless. The problem was a missing mtx_leave() before calling
> pool_get(..., PR_WAITOK) in a completely different code path.
> 
> This diff adds a check in pool_get() that we actually are in a sleepable
> context when PR_WAITOK is given. There is an equivalent check present in
> malloc() already.
> 
> Tests and feedback welcome.

Why after the pool_do_get() call?

For a long time my tree has had the following without issue

Index: subr_pool.c
===================================================================
RCS file: /cvs/src/sys/kern/subr_pool.c,v
diff -u -p -r1.241 subr_pool.c
--- subr_pool.c	21 Jul 2025 20:36:41 -0000	1.241
+++ subr_pool.c	22 Jul 2025 02:52:13 -0000
@@ -562,6 +562,9 @@ pool_get(struct pool *pp, int flags)
 	void *v = NULL;
 	int slowdown = 0;
 
+	if (flags & PR_WAITOK)
+		assertwaitok();
+
 	KASSERT(flags & (PR_WAITOK | PR_NOWAIT));
 	if (pp->pr_flags & PR_RWLOCK)
 		KASSERT(flags & PR_WAITOK);

> 
> 
>  - Christian
> 
> [1] https://marc.info/?l=openbsd-bugs&m=175139531419854
> 
> 
> diff --git a/sys/kern/subr_pool.c b/sys/kern/subr_pool.c
> index d76e8b27cd57..7fb57d569d1c 100644
> --- a/sys/kern/subr_pool.c
> +++ b/sys/kern/subr_pool.c
> @@ -584,8 +584,13 @@ pool_get(struct pool *pp, int flags)
>  	}
>  	pl_leave(pp, &pp->pr_lock);
>  
> -	if ((slowdown || pool_debug == 2) && ISSET(flags, PR_WAITOK))
> -		yield();
> +	if (ISSET(flags, PR_WAITOK)) {
> +#ifdef DIAGNOSTIC
> +		assertwaitok();
> +#endif
> +		if (slowdown || pool_debug == 2)
> +			yield();
> +	}
>  
>  	if (v == NULL) {
>  		struct pool_get_memory mem = { .v = NULL };