From hsu@FreeBSD.org Fri Nov 29 15:37:16 2002 Date: Wed, 27 Nov 2002 21:22:44 -0800 From: Jeffrey Hsu To: rwatson@freebsd.org Subject: file descriptor locking problem #1 (fwd) ------- Forwarded Message X-Mailer: exmh version 2.5 07/13/2001 with nmh-1.0.4 To: jhb@freebsd.org Subject: file descriptor locking problem #1 Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Date: Wed, 27 Nov 2002 12:40:07 -0800 From: Jeffrey Hsu In kern_descrip.c, the kern_fcntl() routine, two processes could do a fcntl(F_SETFL) and the resulting race condition could leave f_flags in an indeterminate state due to the following read-modify-write code not being properly locked: case F_SETFL: fhold(fp); FILEDESC_UNLOCK(fdp); fp->f_flag &= ~FCNTLFLAGS; fp->f_flag |= FFLAGS(arg & ~O_ACCMODE) & FCNTLFLAGS; The fhold() only increments the f_count field and does not lock the struct file: #define fhold(fp) \ do { \ FILE_LOCK(fp); \ fhold_locked(fp); \ FILE_UNLOCK(fp); \ } while (0) #define fhold_locked(fp) \ do { \ FILE_LOCK_ASSERT(fp, MA_OWNED); \ (fp)->f_count++; \ } while (0) ------- End of Forwarded Message From hsu@FreeBSD.org Fri Nov 29 15:37:23 2002 Date: Wed, 27 Nov 2002 21:22:56 -0800 From: Jeffrey Hsu To: jhb@freebsd.org Cc: rwatson@freebsd.org Subject: Re: file descriptor locking problem #1 Code speaks louder than words, so here's a patch for file descriptor locking problem #1. I've already identified 2 other problems with file descriptor locking and will mail you patches for them too. I plan to gather them all and then commit them (in separate commits for each race condition fixed). Jeffrey Index: kern_descrip.c =================================================================== RCS file: /home/ncvs/src/sys/kern/kern_descrip.c,v retrieving revision 1.168 diff -u -r1.168 kern_descrip.c --- kern_descrip.c 27 Oct 2002 18:07:41 -0000 1.168 +++ kern_descrip.c 27 Nov 2002 23:07:16 -0000 @@ -298,10 +298,12 @@ break; case F_SETFL: - fhold(fp); + FILE_LOCK(fp); FILEDESC_UNLOCK(fdp); + fhold_locked(fp); fp->f_flag &= ~FCNTLFLAGS; fp->f_flag |= FFLAGS(arg & ~O_ACCMODE) & FCNTLFLAGS; + FILE_UNLOCK(fp); tmp = fp->f_flag & FNONBLOCK; error = fo_ioctl(fp, FIONBIO, &tmp, td->td_ucred, td); if (error) { @@ -314,7 +316,9 @@ fdrop(fp, td); break; } + FILE_LOCK(fp); fp->f_flag &= ~FNONBLOCK; + FILE_UNLOCK(fp); tmp = 0; (void)fo_ioctl(fp, FIONBIO, &tmp, td->td_ucred, td); fdrop(fp, td);