From owner-freebsd-hackers@FreeBSD.ORG Fri Mar 20 00:05:08 2015 Return-Path: Delivered-To: freebsd-hackers@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 46D8F9B8 for ; Fri, 20 Mar 2015 00:05:08 +0000 (UTC) Received: from ustc.edu.cn (email6.ustc.edu.cn [IPv6:2001:da8:d800::8]) by mx1.freebsd.org (Postfix) with ESMTP id AC040E0E for ; Fri, 20 Mar 2015 00:05:07 +0000 (UTC) Received: from freebsd (unknown [58.211.218.74]) by newmailweb.ustc.edu.cn (Coremail) with SMTP id LkAmygBXCD0oZAtVhyK2Aw--.22783S2; Fri, 20 Mar 2015 08:05:04 +0800 (CST) Date: Fri, 20 Mar 2015 08:04:41 +0800 From: Tiwei Bie To: Konstantin Belousov Subject: Re: [PATCH] Finish the task 'Fix corefilename race' Message-ID: <20150320000418.GA78913@freebsd> References: <1426749223-18118-1-git-send-email-btw@mail.ustc.edu.cn> <20150319101019.GZ2379@kib.kiev.ua> <20150319113530.GA33176@freebsd> <20150319144004.GD2379@kib.kiev.ua> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20150319144004.GD2379@kib.kiev.ua> User-Agent: Mutt/1.5.23 (2014-03-12) X-CM-TRANSID: LkAmygBXCD0oZAtVhyK2Aw--.22783S2 X-Coremail-Antispam: 1UD129KBjvJXoW3GrW8Jw1UGw4DGrWDtFykuFg_yoW7XF18pF ZYkFyrArs5Cr45Cr1Sva1FkFn0y348tr4DW347Xa1YyrnYqrykXF1rtr1F9F1kWrn2934q qa15XrZxKryjvaDanT9S1TB71UUUUUUqnTZGkaVYY2UrUUUUjbIjqfuFe4nvWSU5nxnvy2 9KBjDU0xBIdaVrnRJUUUkjb7Iv0xC_Kw4lb4IE77IF4wAFF20E14v26r1j6r4UM7CY07I2 0VC2zVCF04k26cxKx2IYs7xG6rWj6s0DM7CIcVAFz4kK6r1j6r18M28lY4IEw2IIxxk0rw A2F7IY1VAKz4vEj48ve4kI8wA2z4x0Y4vE2Ix0cI8IcVAFwI0_Gr0_Xr1l84ACjcxK6xII jxv20xvEc7CjxVAFwI0_Gr0_Cr1l84ACjcxK6I8E87Iv67AKxVW8Jr0_Cr1UM28EF7xvwV C2z280aVCY1x0267AKxVW0oVCq3wAS0I0E0xvYzxvE52x082IY62kv0487Mc02F40EFcxC 0VAKzVAqx4xG6I80ewAv7VC0I7IYx2IY67AKxVWUGVWUXwAv7VC2z280aVAFwI0_Jr0_Gr 1lOx8S6xCaFVCjc4AY6r1j6r4UM4x0Y48IcVAKI48JMxkIecxEwVAFwVWDMxAIw28IcxkI 7VAKI48JMxC20s026xCaFVCjc4AY6r1j6r4UMI8I3I0E5I8CrVAFwI0_Jr0_Jr4lx2IqxV Cjr7xvwVAFwI0_JrI_JrWlx4CE17CEb7AF67AKxVWUXVWUAwCIc40Y0x0EwIxGrwCI42IY 6xIIjxv20xvE14v26r1j6r1xMIIF0xvE2Ix0cI8IcVCY1x0267AKxVWUJVW8JwCI42IY6x AIw20EY4v20xvaj40_WFyUJVCq3wCI42IY6I8E87Iv67AKxVWUJVW8JwCI42IY6I8E87Iv 6xkF7I0E14v26r1j6r4UYxBIdaVFxhVjvjDU0xZFpf9x07brXd8UUUUU= X-CM-SenderInfo: xewzqzxdloh3xvwfhvlgxou0/1tbiAQUNAVQhl-muRQAHs- Cc: freebsd-hackers@freebsd.org, mjguzik@gmail.com X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 20 Mar 2015 00:05:08 -0000 On Thu, Mar 19, 2015 at 04:40:04PM +0200, Konstantin Belousov wrote: > On Thu, Mar 19, 2015 at 07:35:30PM +0800, Tiwei Bie wrote: > > On Thu, Mar 19, 2015 at 12:10:19PM +0200, Konstantin Belousov wrote: > > > On Thu, Mar 19, 2015 at 03:13:43PM +0800, Tiwei Bie wrote: > > > > Hi, Mateusz! > > > > > > > > I have finished the task: Fix corefilename race [1]. > > > > > > > > Following is my patch: > > > > > > > > --- > > > > sys/kern/kern_sig.c | 22 ++++++++++++++++++++-- > > > > 1 file changed, 20 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/sys/kern/kern_sig.c b/sys/kern/kern_sig.c > > > > index 58d9707..a1421cb 100644 > > > > --- a/sys/kern/kern_sig.c > > > > +++ b/sys/kern/kern_sig.c > > > > @@ -3090,8 +3090,24 @@ static int compress_user_cores = 0; > > > > #endif > > > > > > > > static char corefilename[MAXPATHLEN] = {"%N.core"}; > > > > -SYSCTL_STRING(_kern, OID_AUTO, corefile, CTLFLAG_RWTUN, corefilename, > > > > - sizeof(corefilename), "Process corefile name format string"); > > > > + > > > > +static struct sx corefilename_lock; > > > > +SX_SYSINIT(corefilename_init, &corefilename_lock, "corefilename lock"); > > > > + > > > > +static int > > > > +sysctl_kern_corefile(SYSCTL_HANDLER_ARGS) > > > > +{ > > > > + int error; > > > > + > > > > + sx_xlock(&corefilename_lock); > > > > + error = sysctl_handle_string(oidp, corefilename, MAXPATHLEN, req); > > > > + sx_xunlock(&corefilename_lock); > > > > + > > > > + return (error); > > > > +} > > > > +SYSCTL_PROC(_kern, OID_AUTO, corefile, CTLTYPE_STRING | CTLFLAG_RWTUN | > > > > + CTLFLAG_MPSAFE, 0, 0, sysctl_kern_corefile, "A", > > > > + "Process corefile name format string"); > > > > > > > > /* > > > > * corefile_open(comm, uid, pid, td, compress, vpp, namep) > > > > @@ -3120,6 +3136,7 @@ corefile_open(const char *comm, uid_t uid, pid_t pid, struct thread *td, > > > > name = malloc(MAXPATHLEN, M_TEMP, M_WAITOK | M_ZERO); > > > > indexpos = -1; > > > > (void)sbuf_new(&sb, name, MAXPATHLEN, SBUF_FIXEDLEN); > > > > + sx_slock(&corefilename_lock); > > > > for (i = 0; format[i] != '\0'; i++) { > > > > switch (format[i]) { > > > > case '%': /* Format character */ > > > > @@ -3162,6 +3179,7 @@ corefile_open(const char *comm, uid_t uid, pid_t pid, struct thread *td, > > > > break; > > > > } > > > > } > > > > + sx_sunlock(&corefilename_lock); > > > > free(hostname, M_TEMP); > > > > if (compress) > > > > sbuf_printf(&sb, GZ_SUFFIX); > > > > > > So the race is between somebody setting the core path string and another > > > process coredumping, am I right ? > > > > > > > Yeah, you are right! > > > > > If you, could you try to reuse some existing lock for the task ? It is a > > > waste to have sx dedicated to the task, which is probably never used by > > > 99% of the machines in the world. > > > > Yeah, of course. After a quick `grep'ing, I found the sx lock: allprison_lock, > > which is defined in kern/kern_jail.c. It is rarely acquired as an exclusive > > lock, and is not defined as a static variable. So I think it will be a good > > candidate for this task. > > I think it is too quick grepping. allprison_lock is completely unrelated > to the process management. Please consider to utilize some lock used > for the process management. Well, this time I found allproc_lock (defined in kern/kern_proc.c), which is used to protect the process list. I think it may be a better choice. Here is the new patch: --- sys/kern/kern_sig.c | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/sys/kern/kern_sig.c b/sys/kern/kern_sig.c index 58d9707..cac2ea0 100644 --- a/sys/kern/kern_sig.c +++ b/sys/kern/kern_sig.c @@ -3089,9 +3089,27 @@ SYSCTL_INT(_kern, OID_AUTO, compress_user_cores_gzlevel, CTLFLAG_RWTUN, static int compress_user_cores = 0; #endif +/* + * Protect the access to corefilename[] by allproc_lock. + */ +#define corefilename_lock allproc_lock + static char corefilename[MAXPATHLEN] = {"%N.core"}; -SYSCTL_STRING(_kern, OID_AUTO, corefile, CTLFLAG_RWTUN, corefilename, - sizeof(corefilename), "Process corefile name format string"); + +static int +sysctl_kern_corefile(SYSCTL_HANDLER_ARGS) +{ + int error; + + sx_xlock(&corefilename_lock); + error = sysctl_handle_string(oidp, corefilename, MAXPATHLEN, req); + sx_xunlock(&corefilename_lock); + + return (error); +} +SYSCTL_PROC(_kern, OID_AUTO, corefile, CTLTYPE_STRING | CTLFLAG_RWTUN | + CTLFLAG_MPSAFE, 0, 0, sysctl_kern_corefile, "A", + "Process corefile name format string"); /* * corefile_open(comm, uid, pid, td, compress, vpp, namep) @@ -3120,6 +3138,7 @@ corefile_open(const char *comm, uid_t uid, pid_t pid, struct thread *td, name = malloc(MAXPATHLEN, M_TEMP, M_WAITOK | M_ZERO); indexpos = -1; (void)sbuf_new(&sb, name, MAXPATHLEN, SBUF_FIXEDLEN); + sx_slock(&corefilename_lock); for (i = 0; format[i] != '\0'; i++) { switch (format[i]) { case '%': /* Format character */ @@ -3162,6 +3181,7 @@ corefile_open(const char *comm, uid_t uid, pid_t pid, struct thread *td, break; } } + sx_sunlock(&corefilename_lock); free(hostname, M_TEMP); if (compress) sbuf_printf(&sb, GZ_SUFFIX); -- 2.1.2 Best regards, Tiwei Bie