From owner-freebsd-hackers@FreeBSD.ORG Sat Mar 21 04:43:49 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 78478A04 for ; Sat, 21 Mar 2015 04:43:49 +0000 (UTC) Received: from mail-wg0-x22c.google.com (mail-wg0-x22c.google.com [IPv6:2a00:1450:400c:c00::22c]) (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 F111B14B for ; Sat, 21 Mar 2015 04:43:48 +0000 (UTC) Received: by wgbcc7 with SMTP id cc7so104556279wgb.0 for ; Fri, 20 Mar 2015 21:43:47 -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:mail-followup-to:references :mime-version:content-type:content-disposition:in-reply-to :user-agent; bh=oKUDzhL7ZIrN6I/GnMoSmM0DIqynNOjJtqdA05z6Y8s=; b=mexGQvML6Xkv33YN7p+YEUA1TeXjnXfvANPbE6bbxV3Ty3lr1KQZQvPLk+bdv3WnCa RmftRpip/Soe+DtL9fVGbB7nDxMNipRvQn7Iev88kVb8lAezkBvrPDQKRY9hlJJGGgyg 0bkRTam0HpxNrJtS/vWg4CYcj5KySKvNg6NCDJKEJjFB40/m+RRLLMzVXW/3GyLn6L30 L85SFRhDcamAwIZo2im301q8Fk/VcARezhnC6kgaCuck0JupQSaA0hS8uW//cL0DqETu dPYxtuUgS0lPtuXtt+WvaT4gLUYSapQwxQukGP1ZZy9TdQ/O8Vi9/g6ctBb7oLx80Z5v D5Yg== X-Received: by 10.180.79.65 with SMTP id h1mr1621596wix.59.1426913027297; Fri, 20 Mar 2015 21:43:47 -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 ps4sm9046943wjc.31.2015.03.20.21.43.45 (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Fri, 20 Mar 2015 21:43:45 -0700 (PDT) Date: Sat, 21 Mar 2015 05:43:43 +0100 From: Mateusz Guzik To: Tiwei Bie Subject: Re: [PATCH] Finish the task 'Fix corefilename race' Message-ID: <20150321044343.GD27736@dft-labs.eu> Mail-Followup-To: Mateusz Guzik , Tiwei Bie , Prasad Joshi , Konstantin Belousov , freebsd-hackers@freebsd.org 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> <20150320000418.GA78913@freebsd> <20150320063604.GA69307@freebsd> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20150320063604.GA69307@freebsd> User-Agent: Mutt/1.5.21 (2010-09-15) Cc: Konstantin Belousov , freebsd-hackers@freebsd.org, Prasad Joshi 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: Sat, 21 Mar 2015 04:43:49 -0000 On Fri, Mar 20, 2015 at 02:36:04PM +0800, Tiwei Bie wrote: > On Fri, Mar 20, 2015 at 11:18:27AM +0530, Prasad Joshi wrote: > > On Fri, Mar 20, 2015 at 5:34 AM, Tiwei Bie wrote: > > > 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); > > > > Hello, > > > > Though I am not an expert FreeBSD developer. In my humble opinion, > > MAXPATHLEN must be replaced with sizeof(corefilename). For example, > > the SYSCTL_STRING line (above) removed in this patch, preferred to use > > sizeof(corefilename) instead of MAXPATHLEN. > > > > Because corefilename is defined as `char corefilename[MAXPATHLEN]', > sizeof(corefilename) equals MAXPATHLEN. So, technically speaking, > both of them are right. But one of them may be preferred to keep the > coding style consistent. I also think sizeof(corefilename) is a > better choice. Thanks for pointing it out, I updated my patch: > > --- > sys/kern/kern_sig.c | 25 +++++++++++++++++++++++-- > 1 file changed, 23 insertions(+), 2 deletions(-) > > diff --git a/sys/kern/kern_sig.c b/sys/kern/kern_sig.c > index 58d9707..8410d9d 100644 > --- a/sys/kern/kern_sig.c > +++ b/sys/kern/kern_sig.c > @@ -3089,9 +3089,28 @@ 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, sizeof(corefilename), > + 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 +3139,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 +3182,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); > Thanks, committed as r280312. -- Mateusz Guzik