From owner-freebsd-arch@FreeBSD.ORG Fri Aug 15 19:13:23 2014 Return-Path: Delivered-To: freebsd-arch@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id A18CDD79; Fri, 15 Aug 2014 19:13:23 +0000 (UTC) Received: from bigwig.baldwin.cx (bigwig.baldwin.cx [IPv6:2001:470:1f11:75::1]) (using TLSv1 with cipher DHE-RSA-CAMELLIA256-SHA (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 7756F2DC5; Fri, 15 Aug 2014 19:13:23 +0000 (UTC) Received: from jhbbsd.localnet (unknown [209.249.190.124]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id DBDE7B963; Fri, 15 Aug 2014 15:13:20 -0400 (EDT) From: John Baldwin To: freebsd-arch@freebsd.org Subject: Re: [PATCH 0/2] plug capability races Date: Fri, 15 Aug 2014 10:31:45 -0400 User-Agent: KMail/1.13.5 (FreeBSD/8.4-CBSD-20140415; KDE/4.5.5; amd64; ; ) References: <1408064112-573-1-git-send-email-mjguzik@gmail.com> In-Reply-To: <1408064112-573-1-git-send-email-mjguzik@gmail.com> MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Message-Id: <201408151031.45967.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.7 (bigwig.baldwin.cx); Fri, 15 Aug 2014 15:13:21 -0400 (EDT) Cc: Konstantin Belousov , Mateusz Guzik , Robert Watson , Johan Schuijt X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 15 Aug 2014 19:13:23 -0000 On Thursday, August 14, 2014 8:55:10 pm Mateusz Guzik wrote: > fget_unlocked currently reads 'fde' which is a structure consisting of > serveral fields. In effect the read is inatomic and may result in > obtaining file pointer with stale or incorrect capabilities. > > Example race is with dup2. > > Side effect is that capability checks can be circumvented. > > Proposed way to fix it is with the help of sequence counters. > > Patchset assumes stuff from > 'Getting rid of atomic_load_acq_int(&fdp->fd_nfiles)) from fget_unlocked' > ( http://lists.freebsd.org/pipermail/freebsd-arch/2014-July/015550.html ) > is applied. There is no technical dependency between patches (apart from > READ_ONCE), but this patch amortizes performance hit introduced with seqlock. > > So this introduces a measurable hit with a microbenchmark (16 threads > reading from a pipe which fails with EAGAIN), but is still much faster than > current code with atomic_load_acq_int(&fdp->fd_nfiles). > > x propernoacq-readpipe-run-sum > + seq2-noacq-readpipe-run-sum > N Min Max Median Avg Stddev > x 20 59479718 59527286 59496714 59499504 13752.968 > + 20 54520752 54920054 54829539 54773480 136842.96 > Difference at 95.0% confidence > -4.72602e+06 +/- 62244.4 > -7.94296% +/- 0.104613% > (Student's t, pooled s = 97250) > > There is still one theoretical race unfixed, but I don't believe it matters > much. > > The race is: > fp gets reallocated before refcount check. this resuls in returning fp > regardless of new caps, but I don't see how this particular race could be > exploited. It could be fixed by re-reading entire fde and checking if it > changed. One thing I would like to see is for the timecounter code to be adapted to use the seq API instead of doing it by hand (the timecounter code is also missing barriers due to doing it by hand). Also, seq needs a seq(9) manpage. -- John Baldwin