Date: Sat, 16 Aug 2014 03:29:46 +0200 From: Mateusz Guzik <mjguzik@gmail.com> To: John Baldwin <jhb@freebsd.org> 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: <20140816012946.GA19135@dft-labs.eu> In-Reply-To: <201408151031.45967.jhb@freebsd.org> References: <1408064112-573-1-git-send-email-mjguzik@gmail.com> <201408151031.45967.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, Aug 15, 2014 at 10:31:45AM -0400, John Baldwin wrote: > 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. > Sure, I'm happy to write one later. Just in case I would like to note the following: There are some similarities to linux version of the same idea: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/include/linux/seqlock.h On the other hand I don't see how one could implement this in a vastly different matter. -- Mateusz Guzik <mjguzik gmail.com>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20140816012946.GA19135>