Date: Sun, 17 Jan 2016 09:24:40 +0100 From: Mateusz Guzik <mjguzik@gmail.com> To: Konstantin Belousov <kib@kib.kiev.ua> Cc: Vijay Singh <vijju.singh@gmail.com>, freebsd-hackers@freebsd.org, Chagin Dmitry <dchagin@freebsd.org> Subject: Re: irrelevant locking Message-ID: <20160117082439.GB1963@dft-labs.eu> In-Reply-To: <20160117034315.GN3942@kib.kiev.ua> References: <20160116195819.GA41610@chd.heemeyer.club> <20160116202643.GL3942@kib.kiev.ua> <CALCNsJT_gH5gJaB%2ByVQRcON84JntSUevG8-X-0Z5_13DkPC%2BBg@mail.gmail.com> <20160116224312.GA1963@dft-labs.eu> <20160117034315.GN3942@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
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 <mjguzik gmail.com>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20160117082439.GB1963>