git: dfd7d1610aeb - main - filedesc: Close race between fcntl(F_SETFL) and ioctl(FIONBIO/FIOASYNC)

The branch main has been updated by jhb:

URL: https://cgit.FreeBSD.org/src/commit/?id=dfd7d1610aeb654aafcaa8cf77b75fc5c7d60250

commit dfd7d1610aeb654aafcaa8cf77b75fc5c7d60250
Author:     John Baldwin <jhb@FreeBSD.org>
AuthorDate: 2025-10-03 16:43:30 +0000
Commit:     John Baldwin <jhb@FreeBSD.org>
CommitDate: 2025-10-03 16:43:30 +0000

    filedesc: Close race between fcntl(F_SETFL) and ioctl(FIONBIO/FIOASYNC)
    
    - Use the recently-added fsetfl_lock/unlock API to synchronize direct
      FIONBIO and FIOASYNC ioctls with fcntl(F_SETFL).
    
    - While here, skip calling the underlying ioctl if the flag's current
      state matches the requested state.
    
    - Also while here, only update the flag state if the underlying ioctl
      succeeds.  This fixes a bug where the flags represented the new
      state even if the underlying ioctl failed.  A test is added for this
      last case that a failing FIOASYNC on /dev/null doesn't result in
      setting O_ASYNC in the file flags.
    
    Reviewed by:    kib
    Differential Revision:  https://reviews.freebsd.org/D52721
---
 sys/kern/sys_generic.c           | 36 +++++++++++++++++++++---------------
 tests/sys/file/fcntlflags_test.c | 27 +++++++++++++++++++++++++++
 2 files changed, 48 insertions(+), 15 deletions(-)

diff --git a/sys/kern/sys_generic.c b/sys/kern/sys_generic.c
index 5606b36f772f..7d666da9f88b 100644
--- a/sys/kern/sys_generic.c
+++ b/sys/kern/sys_generic.c
@@ -729,7 +729,7 @@ kern_ioctl(struct thread *td, int fd, u_long com, caddr_t data)
 {
 	struct file *fp;
 	struct filedesc *fdp;
-	int error, tmp, locked;
+	int error, f_flag, tmp, locked;
 
 	AUDIT_ARG_FD(fd);
 	AUDIT_ARG_CMD(com);
@@ -782,30 +782,36 @@ kern_ioctl(struct thread *td, int fd, u_long com, caddr_t data)
 		goto out;
 	}
 
+	f_flag = 0;
 	switch (com) {
 	case FIONCLEX:
 		fdp->fd_ofiles[fd].fde_flags &= ~UF_EXCLOSE;
-		goto out;
+		break;
 	case FIOCLEX:
 		fdp->fd_ofiles[fd].fde_flags |= UF_EXCLOSE;
-		goto out;
-	case FIONBIO:
-		if ((tmp = *(int *)data))
-			atomic_set_int(&fp->f_flag, FNONBLOCK);
-		else
-			atomic_clear_int(&fp->f_flag, FNONBLOCK);
-		data = (void *)&tmp;
 		break;
+	case FIONBIO:
 	case FIOASYNC:
-		if ((tmp = *(int *)data))
-			atomic_set_int(&fp->f_flag, FASYNC);
-		else
-			atomic_clear_int(&fp->f_flag, FASYNC);
-		data = (void *)&tmp;
+		f_flag = com == FIONBIO ? FNONBLOCK : FASYNC;
+		tmp = *(int *)data;
+		fsetfl_lock(fp);
+		if (((fp->f_flag & f_flag) != 0) != (tmp != 0)) {
+			error = fo_ioctl(fp, com, (void *)&tmp, td->td_ucred,
+			    td);
+			if (error == 0) {
+				if (tmp != 0)
+					atomic_set_int(&fp->f_flag, f_flag);
+				else
+					atomic_clear_int(&fp->f_flag, f_flag);
+			}
+		}
+		fsetfl_unlock(fp);
+		break;
+	default:
+		error = fo_ioctl(fp, com, data, td->td_ucred, td);
 		break;
 	}
 
-	error = fo_ioctl(fp, com, data, td->td_ucred, td);
 out:
 	switch (locked) {
 	case LA_XLOCKED:
diff --git a/tests/sys/file/fcntlflags_test.c b/tests/sys/file/fcntlflags_test.c
index 009bb87371a1..15a18c113c4a 100644
--- a/tests/sys/file/fcntlflags_test.c
+++ b/tests/sys/file/fcntlflags_test.c
@@ -24,6 +24,7 @@
  * SUCH DAMAGE.
  */
 
+#include <sys/filio.h>
 #include <errno.h>
 #include <fcntl.h>
 #include <stdio.h>
@@ -95,12 +96,38 @@ ATF_TC_BODY(exec_only_sh, tc)
 	basic_tests("/bin/sh", O_EXEC, "O_EXEC");
 }
 
+ATF_TC_WITHOUT_HEAD(fioasync_dev_null);
+ATF_TC_BODY(fioasync_dev_null, tc)
+{
+	int fd, flags1, flags2, val;
+
+	fd = open("/dev/null", O_RDONLY);
+	ATF_REQUIRE_MSG(fd != -1, "open(\"/dev/null\") failed: %s",
+	    strerror(errno));
+
+	flags1 = fcntl(fd, F_GETFL);
+	ATF_REQUIRE_MSG(flags1 != -1, "fcntl(F_GETFL) (1) failed: %s",
+	    strerror(errno));
+	ATF_REQUIRE((flags1 & O_ASYNC) == 0);
+
+	val = 1;
+	ATF_REQUIRE_ERRNO(EINVAL, ioctl(fd, FIOASYNC, &val) == -1);
+
+	flags2 = fcntl(fd, F_GETFL);
+	ATF_REQUIRE_MSG(flags2 != -1, "fcntl(F_GETFL) (2) failed: %s",
+	    strerror(errno));
+	ATF_REQUIRE_INTEQ(flags1, flags2);
+
+	(void)close(fd);
+}
+
 ATF_TP_ADD_TCS(tp)
 {
 	ATF_TP_ADD_TC(tp, read_only_null);
 	ATF_TP_ADD_TC(tp, write_only_null);
 	ATF_TP_ADD_TC(tp, read_write_null);
 	ATF_TP_ADD_TC(tp, exec_only_sh);
+	ATF_TP_ADD_TC(tp, fioasync_dev_null);
 
 	return (atf_no_error());
 }