From owner-freebsd-hackers@FreeBSD.ORG Fri Mar 20 05:48:28 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 8E28639A for ; Fri, 20 Mar 2015 05:48:28 +0000 (UTC) Received: from mail-ig0-x22b.google.com (mail-ig0-x22b.google.com [IPv6:2607:f8b0:4001:c05::22b]) (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 516E4372 for ; Fri, 20 Mar 2015 05:48:28 +0000 (UTC) Received: by igcau2 with SMTP id au2so9656684igc.0 for ; Thu, 19 Mar 2015 22:48:27 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type; bh=WkXxYufDemNl6Q1onwNf40yMvLpUKV+vu6Z0rb4JPqM=; b=iBy44zMbMafqyspDu5hQRQP/cRNWSmeBAC247sItZIGXWXFDLdcmDa1LveYnJgsSYy flYNYjjDcfyMhXPnnG6PyhCeTc8BR19UNkygRmVmdzxdwiNjzoEhIIN/aJz8Z80momSh nEE5e7tvQrLjx+gm57ylUwVOuw7jzfYM+s6iKKBALrYLJ8yunmZGtF4SONf6qFoc6QrT lDo6dN/Rghned7Yw7539wkHRIC8lmsfkrkFlo2OmsWpIr4LRLP3xlifSaYUaVZ64NIJ4 ORA+jLX6PDyO62S+/m16WSfawBs3zwmDIgkFWk+4ZdQIBGuIfn4CMnLsCG+3NcJmXrna z4+A== MIME-Version: 1.0 X-Received: by 10.51.17.7 with SMTP id ga7mr22164750igd.42.1426830507696; Thu, 19 Mar 2015 22:48:27 -0700 (PDT) Received: by 10.36.30.75 with HTTP; Thu, 19 Mar 2015 22:48:27 -0700 (PDT) In-Reply-To: <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> <20150320000418.GA78913@freebsd> Date: Fri, 20 Mar 2015 11:18:27 +0530 Message-ID: Subject: Re: [PATCH] Finish the task 'Fix corefilename race' From: Prasad Joshi To: Tiwei Bie Content-Type: text/plain; charset=UTF-8 Cc: Konstantin Belousov , 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 05:48:28 -0000 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. Thanks and Regards, Prasad >> > > > + 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 > > _______________________________________________ > freebsd-hackers@freebsd.org mailing list > http://lists.freebsd.org/mailman/listinfo/freebsd-hackers > To unsubscribe, send any mail to "freebsd-hackers-unsubscribe@freebsd.org"