From owner-freebsd-stable Sun Oct 29 14:22:34 2000 Delivered-To: freebsd-stable@freebsd.org Received: from math.psu.edu (leibniz.math.psu.edu [146.186.130.2]) by hub.freebsd.org (Postfix) with ESMTP id 1DCAA37B479 for ; Sun, 29 Oct 2000 14:22:32 -0800 (PST) Received: from weyl.math.psu.edu (weyl.math.psu.edu [146.186.130.226]) by math.psu.edu (8.9.3/8.9.3) with ESMTP id RAA23055; Sun, 29 Oct 2000 17:22:26 -0500 (EST) Received: from localhost (viro@localhost) by weyl.math.psu.edu (8.9.3/8.9.3) with ESMTP id RAA28706; Sun, 29 Oct 2000 17:22:25 -0500 (EST) X-Authentication-Warning: weyl.math.psu.edu: viro owned process doing -bs Date: Sun, 29 Oct 2000 17:22:25 -0500 (EST) From: Alexander Viro To: Matt Dillon Cc: Jordan Hubbard , freebsd-stable@freebsd.org Subject: Re: Proposed patch related to August discussion on file descriptor races and kern/11629 In-Reply-To: <200010292152.e9TLqt371320@earth.backplane.com> Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: owner-freebsd-stable@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.ORG On Sun, 29 Oct 2000, Matt Dillon wrote: > I would appreciate a review for correctness. Unfortunately I have only > one test box at the moment, and very little time available so this patch > is directly for 4.2, not -current. > > http://apollo.backplane.com/FreeBSD4/ Matt, one obvious comment (I promise to look through all that stuff, just that right now I've got my hands full with read()/mmap()/truncate() races in our -current, and it's not a pretty sight). In dup2() you have a window when new becomes unused even if it was used. I.e. you can get /* Both 0 and 1 are opened */ Thread A: Thread B: dup2(0,1); fd = open(...); with both threads succeeding and fd being 1. It _is_ a correct use (target descriptor is opened, so it's not a case of inherently racey userland) and result is clearly wrong: fd suddenly becomes an alias of stdin. Proposed fix: start with fhold(oldf = fdp->fd_ofiles[old]); then exchange oldf with fdp->fd_ofiles[new] + set flags, _then_ do if (oldf) closef(oldf); - obviously correct and has all manipulations of the table well-contained. Actually I would (OK, did) put the first part (prior to closef()) under the spinlock - the descriptor layer was the first part of SMP threading in our VFS, but that's another story. To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-stable" in the body of the message