From: Christian Ludwig <cludwig@mailbox.org>
Subject: Re: mbuf cluster m_extref_mtx contention
To: Alexander Bluhm <bluhm@openbsd.org>, David Gwynne <david@gwynne.id.au>
Cc: tech@openbsd.org
Date: Tue, 16 Jun 2026 00:17:56 +0200

Hi,

On Thu, 2026-06-11 at 13:44 +0200, Alexander Bluhm wrote:
> On Fri, Apr 24, 2026 at 04:43:03PM +0200, Alexander Bluhm wrote:
> > On Wed, Apr 22, 2026 at 04:21:42PM +1000, David Gwynne wrote:
> > > from another perspective, if the use after free is the result
> > > mishandling mbufs, then the other diff is more forgiving of a
> > > buggy
> > > behaviour.
> > 
> > I still see stability problems with the extfree proxy diff.  It
> > only happens when doing creating kstack flamegraphs with btrace.
> 
> cludwig@ gave me a hint where r_traceidx in refcnt_rele() is
> problematic.
> 
> thread A calls atomic_dec_int_nv(&r->r_refs), count from 2 to 1
> thread B calls atomic_dec_int_nv(&r->r_refs), set refs to 0
> thread B reads r->r_traceidx
> thread B frees and poisons struct refcnt
> thread A reads r->r_traceidx from invalid memory
> 
> This scenario is unlikely as thread B has to do a lot of work between
> consecutive instructions on thread A.  But it is wrong anyway.
> 
> With the diff below, I tested dlg@'s mbuf-ext-proxy diff multiple
> times without crash.
> 
> ok for the refcnt_rele() trace index fix?

ok cludwig@

> bluhm
> 
> Index: kern/kern_synch.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/kern/kern_synch.c,v
> diff -u -p -r1.233 kern_synch.c
> --- kern/kern_synch.c	24 Nov 2025 12:54:53 -0000	1.233
> +++ kern/kern_synch.c	11 Jun 2026 11:35:50 -0000
> @@ -926,11 +926,13 @@ int
>  refcnt_rele(struct refcnt *r)
>  {
>  	u_int refs;
> +	int trace;
>  
> +	trace = r->r_traceidx;
>  	membar_exit_before_atomic();
>  	refs = atomic_dec_int_nv(&r->r_refs);
>  	KASSERT(refs != ~0);
> -	TRACEINDEX(refcnt, r->r_traceidx, r, refs + 1, -1);
> +	TRACEINDEX(refcnt, trace, r, refs + 1, -1);
>  	if (refs == 0) {
>  		membar_enter_after_atomic();
>  		return (1);
> @@ -949,17 +951,19 @@ void
>  refcnt_finalize(struct refcnt *r, const char *wmesg)
>  {
>  	u_int refs;
> +	int trace;
>  
> +	trace = r->r_traceidx;
>  	membar_exit_before_atomic();
>  	refs = atomic_dec_int_nv(&r->r_refs);
>  	KASSERT(refs != ~0);
> -	TRACEINDEX(refcnt, r->r_traceidx, r, refs + 1, -1);
> +	TRACEINDEX(refcnt, trace, r, refs + 1, -1);
>  	while (refs) {
>  		sleep_setup(r, PWAIT, wmesg);
>  		refs = atomic_load_int(&r->r_refs);
>  		sleep_finish(INFSLP, refs);
>  	}
> -	TRACEINDEX(refcnt, r->r_traceidx, r, refs, 0);
> +	TRACEINDEX(refcnt, trace, r, refs, 0);
>  	/* Order subsequent loads and stores after refs == 0 load.
> */
>  	membar_sync();
>  }
> 
>