From: Alexander Bluhm <bluhm@openbsd.org>
Subject: Re: Improve assertion in refcnt_take()
To: Christian Ludwig <cludwig@genua.de>
Cc: Mark Kettenis <mark.kettenis@xs4all.nl>, tech@openbsd.org
Date: Wed, 26 Feb 2025 19:56:05 +0100

On Tue, Feb 25, 2025 at 10:37:59AM +0100, Christian Ludwig wrote:
> 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.

run full regress with witness on amd64
vmm(4) also works

OK bluhm@

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