Date: Fri, 20 Mar 2015 14:36:04 +0800 From: Tiwei Bie <btw@mail.ustc.edu.cn> To: Prasad Joshi <prasadjoshi.linux@gmail.com> Cc: Konstantin Belousov <kostikbel@gmail.com>, freebsd-hackers@freebsd.org, mjguzik@gmail.com Subject: Re: [PATCH] Finish the task 'Fix corefilename race' Message-ID: <20150320063604.GA69307@freebsd> In-Reply-To: <CALJbWeotyhiGVkruF-mpPNM0DBSZt9bjD4RdhZZrzv-3jwP0ag@mail.gmail.com> 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> <CALJbWeotyhiGVkruF-mpPNM0DBSZt9bjD4RdhZZrzv-3jwP0ag@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, Mar 20, 2015 at 11:18:27AM +0530, Prasad Joshi wrote:
> On Fri, Mar 20, 2015 at 5:34 AM, Tiwei Bie <btw@mail.ustc.edu.cn> 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);
--
2.1.2
Best regards,
Tiwei Bie
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20150320063604.GA69307>