Module Name: src
Committed By: riastradh
Date: Thu Feb 23 02:58:41 UTC 2023
Modified Files:
src/sys/kern: kern_descrip.c
Log Message:
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.
XXX pullup-9
XXX pullup-10
To generate a diff of this commit:
cvs rdiff -u -r1.252 -r1.253 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.
Modified files:
Index: src/sys/kern/kern_descrip.c
diff -u src/sys/kern/kern_descrip.c:1.252 src/sys/kern/kern_descrip.c:1.253
--- src/sys/kern/kern_descrip.c:1.252 Thu Feb 23 02:58:28 2023
+++ src/sys/kern/kern_descrip.c Thu Feb 23 02:58:40 2023
@@ -1,4 +1,4 @@
-/* $NetBSD: kern_descrip.c,v 1.252 2023/02/23 02:58:28 riastradh Exp $ */
+/* $NetBSD: kern_descrip.c,v 1.253 2023/02/23 02:58:40 riastradh Exp $ */
/*-
* Copyright (c) 2008, 2009 The NetBSD Foundation, Inc.
@@ -70,7 +70,7 @@
*/
#include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: kern_descrip.c,v 1.252 2023/02/23 02:58:28 riastradh Exp $");
+__KERNEL_RCSID(0, "$NetBSD: kern_descrip.c,v 1.253 2023/02/23 02:58:40 riastradh Exp $");
#include <sys/param.h>
#include <sys/systm.h>
@@ -624,7 +624,7 @@ fd_close(unsigned fd)
* will prevent them from adding additional uses to this file
* while we are closing it.
*/
- ff->ff_file = NULL;
+ atomic_store_relaxed(&ff->ff_file, NULL);
ff->ff_exclose = false;
/*
@@ -1152,12 +1152,6 @@ fd_affix(proc_t *p, file_t *fp, unsigned
/*
* Insert the new file into the descriptor slot.
- *
- * The memory barriers provided by lock activity in this routine
- * ensure that any updates to the file structure become globally
- * visible before the file becomes visible to other LWPs in the
- * current process; otherwise we would set ff->ff_file with
- * atomic_store_release(&ff->ff_file, fp) at the bottom.
*/
fdp = p->p_fd;
dt = atomic_load_consume(&fdp->fd_dt);
@@ -1170,7 +1164,7 @@ fd_affix(proc_t *p, file_t *fp, unsigned
KASSERT(fd >= NDFDFILE || ff == (fdfile_t *)fdp->fd_dfdfile[fd]);
/* No need to lock in order to make file initially visible. */
- ff->ff_file = fp;
+ atomic_store_release(&ff->ff_file, fp);
}
/*
