Content-Disposition: inline
Content-Transfer-Encoding: 8bit
Content-Type: text/plain; charset="US-ASCII"
MIME-Version: 1.0
Date: Sun, 30 Jul 2023 12:09:51 +0000
From: "Martin Husemann" <martin@netbsd.org>
Subject: CVS commit: [netbsd-10] src/sys/kern
To: source-changes@NetBSD.org
Approved: for-source-only
Reply-To: martin@netbsd.org
Message-Id: <20230730120951.596AEFBDB@cvs.NetBSD.org>

Module Name:	src
Committed By:	martin
Date:		Sun Jul 30 12:09:51 UTC 2023

Modified Files:
	src/sys/kern [netbsd-10]: kern_descrip.c

Log Message:
Pull up following revision(s) (requested by riastradh in ticket #262):

	sys/kern/kern_descrip.c: revision 1.252
	sys/kern/kern_descrip.c: revision 1.253
	sys/kern/kern_descrip.c: revision 1.254

kern_descrip.c: Fix membars around reference count decrement.

In general, the `last one out hit the lights' style of reference
counting (as opposed to the `whoever's destroying must wait for
pending users to finish' style) requires memory barriers like so:
        ... usage of resources associated with object ...
        membar_release();
        if (atomic_dec_uint_nv(&obj->refcnt) != 0)
                return;
        membar_acquire();
        ... freeing of resources associated with object ...

This way, all usage happens-before all freeing.  This fixes several
errors:
- fd_close failed to ensure whatever its caller did would
  happen-before the freeing, in the case where another thread is
  concurrently trying to close the fd (ff->ff_file == NULL).
  Fix: Add membar_release before atomic_dec_uint(&ff->ff_refcnt) in
  that branch.
- fd_close failed to ensure all loads its caller had issued will have
  happened-before the freeing, in the case where the fd is still in
  use by another thread (fdp->fd_refcnt > 1 and ff->ff_refcnt-- > 0).
  Fix: Change membar_producer to membar_release before
  atomic_dec_uint(&ff->ff_refcnt).
- fd_close failed to ensure that any usage of fp by other callers
  would happen-before any freeing it does.
  Fix: Add membar_acquire after atomic_dec_uint_nv(&ff->ff_refcnt).
- fd_free failed to ensure that any usage of fdp by other callers
  would happen-before any freeing it does.
  Fix: Add membar_acquire after atomic_dec_uint_nv(&fdp->fd_refcnt).

While here, change membar_exit -> membar_release.  No semantic
change, just updating away from the legacy API.

kern_descrip.c: Use atomic_store_relaxed/release for ff->ff_file.
1. atomic_store_relaxed in fd_close avoids the appearance of race in
   sanitizers (minor bug).
2. atomic_store_release in fd_affix is necessary because the lock
   activity was not, in fact, enough to guarantee ordering (real bug
   some architectures like aarch64).
   The premise appears to have been that the mutex_enter/exit earlier
   in fd_affix is enough to guarantee that initialization of fp (A)
   happens before use of fp by a user once fp is published (B):
        fp->f_... = ...;                // A
        /* fd_affix */
        mutex_enter(&fp->f_lock);
        fp->f_count++;
        mutex_exit(&fp->f_lock);
        ...
        ff->ff_file = fp;               // B
   But actually mutex_enter/exit allow the following reordering by
   the CPU:
        mutex_enter(&fp->f_lock);
        ff->ff_file = fp;               // B
        fp->f_count++;
        fp->f_... = ...;                // A
        mutex_exit(&fp->f_lock);
   The only constraints they imply are:
        1. fp->f_count++ and B cannot precede mutex_enter
        2. mutex_exit cannot precede A and fp->f_count++
   They imply no constraint on the relative ordering of A, B, and
   fp->f_count++ amongst each other, however.
   This affects any architecture that has a native load-acquire or
   store-release operation in mutex_enter/exit, like aarch64, instead
   of explicit load-before-load/store and load/store-before-store
   barrier.

No need for atomic_store_* in fd_copy or fd_free because we have
exclusive access to ff as is.

kern_descrip.c: Change membar_enter to membar_acquire in fd_getfile.
membar_acquire is cheaper on many CPUs, and unlikely to be costlier
on any CPUs, than the legacy membar_enter.
Add a long comment explaining the interaction between fd_getfile and
fd_close and why membar_acquire is safe.


To generate a diff of this commit:
cvs rdiff -u -r1.251 -r1.251.10.1 src/sys/kern/kern_descrip.c

Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.