Index | Thread | Search

From:
Alexander Bluhm <bluhm@openbsd.org>
Subject:
Re: mbuf cluster m_extref_mtx contention
To:
David Gwynne <david@gwynne.id.au>, Christian Ludwig <cludwig@genua.de>
Cc:
tech@openbsd.org
Date:
Thu, 11 Jun 2026 13:44:26 +0200

Download raw body.

Thread
  • Alexander Bluhm:

    mbuf cluster m_extref_mtx contention

  • 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?
    
    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();
     }
    
    
    
  • Alexander Bluhm:

    mbuf cluster m_extref_mtx contention