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); } /*