From: Christian Ludwig <cludwig@genua.de>
Subject: Re: Improve assertion in refcnt_take()
To: Mark Kettenis <mark.kettenis@xs4all.nl>
Cc: <tech@openbsd.org>
Date: Tue, 25 Feb 2025 10:37:59 +0100

On Mon, Feb 24, 2025 at 11:24:52PM +0100, Mark Kettenis wrote:
> > Date: Mon, 24 Feb 2025 23:12:54 +0100
> > From: Christian Ludwig <cludwig@genua.de>
> > 
> > Hi,
> > 
> > refcnt objects are always initialized to 1. When the counter drops to 0
> > the referenced object can be destroyed at any time. In refcnt_take()
> > atomic_inc_int_nv() returns the incremented counter. That means if
> > refs == 1, it has been 0 before. That would be a bug that should be
> > cought.
> 
> Correct.  You can only take a reference if you currently hold one.
> 
> Did you actually do some testing with this?  We don't want to have
> folks hit bugs the first miniute this lands ;)

It has passed regress on amd64. And it should get more testing, of
course. In particular for drivers that use refcnt:

 bpf(4), iwm(4), iwx(4), qwx(4), qwz(4), vmm(4), xen(4)

Tests and feedback welcome.

> > ---
> >  sys/kern/kern_synch.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/sys/kern/kern_synch.c b/sys/kern/kern_synch.c
> > index 8548aea1c87b..75f7615fdf6a 100644
> > --- a/sys/kern/kern_synch.c
> > +++ b/sys/kern/kern_synch.c
> > @@ -906,7 +906,7 @@ refcnt_take(struct refcnt *r)
> >  	u_int refs;
> >  
> >  	refs = atomic_inc_int_nv(&r->r_refs);
> > -	KASSERT(refs != 0);
> > +	KASSERT(refs > 1);
> >  	TRACEINDEX(refcnt, r->r_traceidx, r, refs - 1, +1);
> >  	(void)refs;
> >  }
> > -- 
> > 2.34.1
> > 
> > 
> > [2:application/pkcs7-signature Show Save:smime.p7s (5kB)]
> >