Date: Mon, 3 Feb 2020 14:28:31 +0000 (UTC) From: Mateusz Guzik <mjg@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: svn commit: r357447 - head/sys/kern Message-ID: <202002031428.013ESV5R080882@repo.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: mjg Date: Mon Feb 3 14:28:31 2020 New Revision: 357447 URL: https://svnweb.freebsd.org/changeset/base/357447 Log: fd: fix f_count acquire in fget_unlocked The code was using a hand-rolled fcmpset loop, while in other places the same count is manipulated with the refcount API. This transferred from a stylistic issue into a bug after the API got extended to support flags. As a result the hand-rolled loop could bump the count high enough to set the bit flag. Another bump + refcount_release would then free the file prematurely. The bug is only present in -CURRENT. Modified: head/sys/kern/kern_descrip.c Modified: head/sys/kern/kern_descrip.c ============================================================================== --- head/sys/kern/kern_descrip.c Mon Feb 3 14:25:32 2020 (r357446) +++ head/sys/kern/kern_descrip.c Mon Feb 3 14:28:31 2020 (r357447) @@ -2693,7 +2693,6 @@ fget_unlocked(struct filedesc *fdp, int fd, cap_rights #endif const struct fdescenttbl *fdt; struct file *fp; - u_int count; #ifdef CAPABILITIES seqc_t seq; cap_rights_t haverights; @@ -2729,27 +2728,27 @@ fget_unlocked(struct filedesc *fdp, int fd, cap_rights if (error != 0) return (error); #endif - count = fp->f_count; - retry: - if (count == 0) { + if (__predict_false(!refcount_acquire_if_not_zero(&fp->f_count))) { /* + * The count was found either saturated or zero. + * This re-read is not any more racy than using the + * return value from fcmpset. + */ + if (fp->f_count != 0) + return (EBADF); + /* * Force a reload. Other thread could reallocate the - * table before this fd was closed, so it possible that - * there is a stale fp pointer in cached version. + * table before this fd was closed, so it is possible + * that there is a stale fp pointer in cached version. */ fdt = (struct fdescenttbl *)atomic_load_ptr(&fdp->fd_files); continue; } - if (__predict_false(count + 1 < count)) - return (EBADF); - /* * Use an acquire barrier to force re-reading of fdt so it is * refreshed for verification. */ - if (__predict_false(atomic_fcmpset_acq_int(&fp->f_count, - &count, count + 1) == 0)) - goto retry; + atomic_thread_fence_acq(); fdt = fdp->fd_files; #ifdef CAPABILITIES if (seqc_consistent_nomb(fd_seqc(fdt, fd), seq))
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202002031428.013ESV5R080882>