From: Christian Ludwig <cludwig@genua.de>
Subject: Re: pool: Check that we can sleep early
To: Jonathan Gray <jsg@jsg.id.au>
Cc: <tech@openbsd.org>
Date: Thu, 31 Jul 2025 18:40:00 +0200

On Fri, Aug 01, 2025 at 12:20:33AM +1000 Jonathan Gray wrote:
> 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?

Ah, good catch!

> 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);
> 

That is better for sure.

> > 
> > 
> >  - 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 };
> 
>