svn commit: r357447 - head/sys/kern

[Mateusz Guzik]

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))