[PATCH] Finish the task 'Fix corefilename race'

[Tiwei Bie]

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