From owner-freebsd-arch@FreeBSD.ORG Sat Aug 16 01:29:53 2014 Return-Path: Delivered-To: freebsd-arch@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 6E84DC61; Sat, 16 Aug 2014 01:29:53 +0000 (UTC) Received: from mail-wi0-x236.google.com (mail-wi0-x236.google.com [IPv6:2a00:1450:400c:c05::236]) (using TLSv1 with cipher ECDHE-RSA-RC4-SHA (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 8C3C1264C; Sat, 16 Aug 2014 01:29:52 +0000 (UTC) Received: by mail-wi0-f182.google.com with SMTP id d1so1465876wiv.9 for ; Fri, 15 Aug 2014 18:29:50 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=date:from:to:cc:subject:message-id:references:mime-version :content-type:content-disposition:in-reply-to:user-agent; bh=2d1GCBETo3RGJRh9TopKYGpikDARqCyPcx6tPmJ3BdA=; b=kE4w8FrjOwMG7KVI0XLQzABkiXQiuRSumj2tDsifzzrrf7HAFELSRqFGr3U10vCVKw Yv1hP/cqyddVk3vlSGV0jhjzA0lE9f4Ku+KplohBa1Dma1OfGuamGLF2D/Qbmc9NZVCT hqxGXsvoHG8OIRJCk0Do1TEhl4a/+KTRH3r5LOuQksfrjetlk4sIhkNUs3Mh5v2LxAFQ jDLI2xh9YNuNDdB1CNQtLao6euHBKgvmnP3UcdRZrB+jdEjYRuCLyujAqpaZ3vr8WzTx uqXW/RxM7WEhYrHKDylvUGLJJltMS6/B0YblBIN/A6m8vBSmDNuNNdlCKZQBEKoA2oLj 9rbg== X-Received: by 10.194.205.129 with SMTP id lg1mr23223568wjc.97.1408152590067; Fri, 15 Aug 2014 18:29:50 -0700 (PDT) Received: from dft-labs.eu (n1x0n-1-pt.tunnel.tserv5.lon1.ipv6.he.net. [2001:470:1f08:1f7::2]) by mx.google.com with ESMTPSA id e3sm22822029wjp.4.2014.08.15.18.29.48 for (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Fri, 15 Aug 2014 18:29:49 -0700 (PDT) Date: Sat, 16 Aug 2014 03:29:46 +0200 From: Mateusz Guzik To: John Baldwin Subject: Re: [PATCH 0/2] plug capability races Message-ID: <20140816012946.GA19135@dft-labs.eu> References: <1408064112-573-1-git-send-email-mjguzik@gmail.com> <201408151031.45967.jhb@freebsd.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <201408151031.45967.jhb@freebsd.org> User-Agent: Mutt/1.5.21 (2010-09-15) Cc: Konstantin Belousov , Robert Watson , Johan Schuijt , freebsd-arch@freebsd.org 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: Sat, 16 Aug 2014 01:29:53 -0000 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