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