Date: Wed, 20 Aug 2014 21:24:19 +0200 From: Pawel Jakub Dawidek <pjd@FreeBSD.org> To: Mateusz Guzik <mjguzik@gmail.com> Cc: Konstantin Belousov <kib@FreeBSD.org>, Robert Watson <rwatson@FreeBSD.org>, Johan Schuijt <johan@transip.nl>, freebsd-arch@freebsd.org Subject: Re: [PATCH 0/2] plug capability races Message-ID: <20140820192419.GA1834@garage.freebsd.pl> In-Reply-To: <1408064112-573-1-git-send-email-mjguzik@gmail.com> References: <1408064112-573-1-git-send-email-mjguzik@gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
The patch looks good to me. Thanks for working on the fix, Mateusz! The only minor nit I found is that fde_change, fde_change_size and fde_seq should use capital letters as those are macros. On Fri, Aug 15, 2014 at 02:55:10AM +0200, 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. > > -- > 2.0.2 > -- Pawel Jakub Dawidek http://www.wheelsystems.com FreeBSD committer http://www.FreeBSD.org Am I Evil? Yes, I Am! http://mobter.com
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20140820192419.GA1834>