Date: Fri, 15 Aug 2014 10:31:45 -0400 From: John Baldwin <jhb@freebsd.org> To: freebsd-arch@freebsd.org Cc: Konstantin Belousov <kib@freebsd.org>, Mateusz Guzik <mjguzik@gmail.com>, Robert Watson <rwatson@freebsd.org>, Johan Schuijt <johan@transip.nl> Subject: Re: [PATCH 0/2] plug capability races Message-ID: <201408151031.45967.jhb@freebsd.org> 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
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201408151031.45967.jhb>