Skip site navigation (1)Skip section navigation (2)
Date:      Sun, 22 Mar 2015 12:24:28 +0200
From:      Konstantin Belousov <kostikbel@gmail.com>
To:        Mateusz Guzik <mjguzik@gmail.com>, Tiwei Bie <btw@mail.ustc.edu.cn>, freebsd-hackers@freebsd.org
Subject:   Re: [PATCH] Finish the task 'Validate coredump format string'
Message-ID:  <20150322102428.GZ2379@kib.kiev.ua>
In-Reply-To: <20150322101401.GH14650@dft-labs.eu>
References:  <1426946345-67889-1-git-send-email-btw@mail.ustc.edu.cn> <20150321200500.GC14650@dft-labs.eu> <20150322091853.GA89976@freebsd> <20150322101401.GH14650@dft-labs.eu>

next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, Mar 22, 2015 at 11:14:01AM +0100, Mateusz Guzik wrote:
> On Sun, Mar 22, 2015 at 05:19:40PM +0800, Tiwei Bie wrote:
> > Sorry, I introduced a bug... allproc_lock could not be used to protect
> > the access to corefilename[].
> > 
> 
> First off I committed the code, so the fault is on me.
> 
> > Because, sysctl_kern_corefile() could be called very early:
> > 
> [..]
> > That is to say, when the tunable `kern.corefile' is set in loader.conf,
> > sysctl_kern_corefile() will be called as the priority of (SI_SUB_KMEM,
> > SI_ORDER_FIRST).
> > 
> > At this time, allproc_lock is not initialized.
> > 
> > I couldn't find a proper existing lock for this task. Maybe a dedicated
> > lock needs to be created. And initialize it together with sysctlmemlock:
> > 
> [..]
> > Or maybe sysctlmemlock could be used, which is only acuqired when
> > req.oldlen > PAGE_SIZE.
> > 
> > 
> 
> I was somehow convinced that tunables are dealt with other code.
> 
> If such sysctl handler is also called for tunables, the kernel should
> pass a flag or some other indicator so that the function knows it is
> dealing with a tunable and that would avoid locking and thus solve the
> problem.
> 
> I'm wondering if we should go a little bit further and get rid of
> static char corefilename[MAXPATHLEN]
> 
> and have a static char *corefilename instead.
Accessing the array through the pointer dereference is micro-pessimization,
as well as having to maintain metadata for the malloced memory, isn't it ?

> 
> A dedicated sysinit func could fetch and validate the tunable, if any.
> If no tunable was provided it would alloc memory for the default.

Or you could move initialization of the sx in question earlier.



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20150322102428.GZ2379>