Date: Tue, 16 Jul 2002 14:15:39 -0400 (EDT) From: John Baldwin <jhb@FreeBSD.org> To: Alfred Perlstein <bright@mu.org> Cc: "freebsd-current@freebsd.org" <freebsd-current@freebsd.org>, David Xu <davidx@viasoft.com.cn> Subject: Re: race condition in kern_descrip.c and fix Message-ID: <XFMail.20020716141539.jhb@FreeBSD.org> In-Reply-To: <20020716062416.GT77219@elvis.mu.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On 16-Jul-2002 Alfred Perlstein wrote:
> * David Xu <davidx@viasoft.com.cn> [020715 22:31] wrote:
>> I found a race condition in kern_descrip.c, the race is in function falloc(),
>> it opens a race window at line 1147:
>
> You're right, however I'd appreciate it if you'd look deeper into the
> possiblity of races in this code before committing this patch to make
> sure we don't want to do this another way.
Here's a way w/o goto loops (caution, untested):
Index: kern_descrip.c
===================================================================
RCS file: /usr/cvs/src/sys/kern/kern_descrip.c,v
retrieving revision 1.149
diff -u -r1.149 kern_descrip.c
--- kern_descrip.c 29 Jun 2002 01:50:24 -0000 1.149
+++ kern_descrip.c 16 Jul 2002 17:27:00 -0000
@@ -1107,32 +1107,27 @@
register struct file *fp, *fq;
int error, i;
+ /*
+ * Allocate a new file descriptor.
+ */
+ fp = uma_zalloc(file_zone, M_WAITOK | M_ZERO);
sx_xlock(&filelist_lock);
if (nfiles >= maxfiles) {
sx_xunlock(&filelist_lock);
+ uma_zfree(file_zone, fp);
tablefull("file");
return (ENFILE);
}
nfiles++;
- sx_xunlock(&filelist_lock);
+
/*
- * Allocate a new file descriptor.
* If the process has file descriptor zero open, add to the list
* of open files at that point, otherwise put it at the front of
* the list of open files.
*/
- fp = uma_zalloc(file_zone, M_WAITOK);
- bzero(fp, sizeof(*fp));
-
- /*
- * wait until after malloc (which may have blocked) returns before
- * allocating the slot, else a race might have shrunk it if we had
- * allocated it before the malloc.
- */
FILEDESC_LOCK(p->p_fd);
if ((error = fdalloc(td, 0, &i))) {
FILEDESC_UNLOCK(p->p_fd);
- sx_xlock(&filelist_lock);
nfiles--;
sx_xunlock(&filelist_lock);
uma_zfree(file_zone, fp);
@@ -1144,9 +1139,6 @@
fp->f_cred = crhold(td->td_ucred);
fp->f_ops = &badfileops;
fp->f_seqcount = 1;
- FILEDESC_UNLOCK(p->p_fd);
- sx_xlock(&filelist_lock);
- FILEDESC_LOCK(p->p_fd);
if ((fq = p->p_fd->fd_ofiles[0])) {
LIST_INSERT_AFTER(fq, fp, f_list);
} else {
This is more how fork1() works. If you want to optimize the maxfiles
check, then you could grab the lock and do that check before the malloc()
and then continue on with the rest of the function like it is after this
patch. I.e., you would do the maxfiles check twice.
--
John Baldwin <jhb@FreeBSD.org> <>< http://www.FreeBSD.org/~jhb/
"Power Users Use the Power to Serve!" - http://www.FreeBSD.org/
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-current" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?XFMail.20020716141539.jhb>