From fee204544ed62ffa7042759a933d534d36364b97 Mon Sep 17 00:00:00 2001 From: Mateusz Guzik Date: Mon, 3 Feb 2020 14:28:31 +0000 Subject: [PATCH] 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. --- sys/kern/kern_descrip.c | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/sys/kern/kern_descrip.c b/sys/kern/kern_descrip.c index b81f7b0eef5d..d928ce763bf0 100644 --- a/sys/kern/kern_descrip.c +++ b/sys/kern/kern_descrip.c @@ -2693,7 +2693,6 @@ fget_unlocked(struct filedesc *fdp, int fd, cap_rights_t *needrightsp, #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_t *needrightsp, 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))