From owner-freebsd-hackers@freebsd.org Sun Jan 17 08:24:44 2016 Return-Path: Delivered-To: freebsd-hackers@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 507D0A6A8E6 for ; Sun, 17 Jan 2016 08:24:44 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: from mail-wm0-x230.google.com (mail-wm0-x230.google.com [IPv6:2a00:1450:400c:c09::230]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id DA808115D; Sun, 17 Jan 2016 08:24:43 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: by mail-wm0-x230.google.com with SMTP id b14so80897728wmb.1; Sun, 17 Jan 2016 00:24:43 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=date:from:to:cc:subject:message-id:mail-followup-to:references :mime-version:content-type:content-disposition:in-reply-to :user-agent; bh=invnWqP1yO6u9TkkIYlFNyOMtwl2G4YoQlmKeuxEP5U=; b=JFJUsI80KeaU+knfTIHcHX95ZXxowtCx1lQztQaFh/R5I6TTIQOxDJEkb2dsdlr/1C dihgFDdK+eq/u2mrrfx6KCc8D8pGjCMUi8z/u07KnxmspfA4Gm5DLqlfbO4H+oGERLlZ 0kvN9XQiqdcRc8Plb7UawaxOF7+vTfgWI6H44BEHearJ6/LjpuWayL4h6/KArOPWTZq0 AVvncbNBUo+z0JueR4svqWJpL1Nxvm4Qjc9dQm+YaTQYuvrsVwegZc0LeC4jiPOpx8yB NEtEUqwkK8FChffxkIQnCOTfm1vHXVEH2bNUT9/2ZklY3nXuKwKg6iPn3dYMzvviRIbW Iztg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:cc:subject:message-id :mail-followup-to:references:mime-version:content-type :content-disposition:in-reply-to:user-agent; bh=invnWqP1yO6u9TkkIYlFNyOMtwl2G4YoQlmKeuxEP5U=; b=YL9vQqCf25eKZAyrsOHBfJ1XOsjQI3EDUEE64JWtXUa8sh6MNKrbSdlskNbFKYXaX2 6eYZaePUQ4iI6RiOsVqqyh31IqfDZTQLy8/9wwc1XEgMKYLMUDXWgb53zMw0UEa4PqyF WZRasN+67xtvzpOgbZ0NLLtAEJeSAX7VDnBKAt3PJrsxHDgv3pgfyUssbenUWN8C7nCb URaLbQVZ4ygzYDvfg+f8IKWmAnnAkd51vHEYqfL4JDN5E4jjs22xee4vGlGReNYbyJRd 5I/FwMbRsF32RbO2+lyZeWFCucqOgkGBUhH9QsuPzkUsbibj/BttCsjobzVDmsCEUdkl Wylg== X-Gm-Message-State: AG10YOQLTqJlNUrv1TThZpb6qd3g1IJkKMq/q/FHas5veziR7tRH/uSSanOUyMojsSAQ0A== X-Received: by 10.28.35.6 with SMTP id j6mr7456273wmj.80.1453019082446; Sun, 17 Jan 2016 00:24:42 -0800 (PST) Received: from dft-labs.eu (n1x0n-1-pt.tunnel.tserv5.lon1.ipv6.he.net. [2001:470:1f08:1f7::2]) by smtp.gmail.com with ESMTPSA id lx4sm18271645wjb.5.2016.01.17.00.24.41 (version=TLS1_2 cipher=AES128-SHA bits=128/128); Sun, 17 Jan 2016 00:24:41 -0800 (PST) Date: Sun, 17 Jan 2016 09:24:40 +0100 From: Mateusz Guzik To: Konstantin Belousov Cc: Vijay Singh , freebsd-hackers@freebsd.org, Chagin Dmitry Subject: Re: irrelevant locking Message-ID: <20160117082439.GB1963@dft-labs.eu> Mail-Followup-To: Mateusz Guzik , Konstantin Belousov , Vijay Singh , freebsd-hackers@freebsd.org, Chagin Dmitry References: <20160116195819.GA41610@chd.heemeyer.club> <20160116202643.GL3942@kib.kiev.ua> <20160116224312.GA1963@dft-labs.eu> <20160117034315.GN3942@kib.kiev.ua> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20160117034315.GN3942@kib.kiev.ua> User-Agent: Mutt/1.5.21 (2010-09-15) X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 17 Jan 2016 08:24:44 -0000 On Sun, Jan 17, 2016 at 05:43:15AM +0200, Konstantin Belousov wrote: > On Sat, Jan 16, 2016 at 11:43:13PM +0100, Mateusz Guzik wrote: > > On Sat, Jan 16, 2016 at 02:08:58PM -0800, Vijay Singh wrote: > > > Couldn't the get & set race otherwise? > > > > Current locking plays no role in correctness here. > > > > Say that locking is left in place. A concurrent setgroups (or whatever) > > call resulting in setting P_SUGID is also being executed. Regardless of > > whether PROC_LOCK/PROC_UNLOCK pair is in place, it can set the bit > > before or after it is being tested by sys_issetugid. > > > > In principle, the very moment you drop a lock, your informatoin is > > stale. > Right, this is the reason why the locking is useless. > > > > > This does not matter here. It's only the process itself which can set > > the bit, so it would have to race with itself. > One thread in the process executing issetugid() can race with another > executing setsugid(). It is legitimate. > I meant threads within the process would have to race. But if there are several threads doing what they want, one needs to synchronize them in userspace as there is nothing the kernel can do in this regard. But who and why would encounter such an issue with issetugid. > > > > Finally, the bit can be only unset during execve, which cannot be > > executed here - if it is being executed, there is only one thread doing > > work and, well, it is doing execve. > > > > The real question is if it would make sense to add the bit to elf aux > > vector to save the call as done by the loader. > I once did a pass to remove (most of) sysctls executed during process > startup. issetugid indeed may be treated same. Hm, looks like the code is more hairy. I'm not touching this at least for now. By the time the vector is prepared it is not known if P_SUGID is going to be set. do_execve locks and unlocks the vnode 3 times, with the first lock being exclusive. That said, when at one point only shared locking is required the code could be reorganized to save at least one lock/unlock pair and with that reorganized it would clearer what to do with it. As a side note I think current code is buggy. VOP_CLOSE is being called with only shared lock held even for filesystems without MNTK_EXTENDED_SHARED, I don't know how harmful this really is. -- Mateusz Guzik