svn commit: r278479 - in head: etc sys/kern

[Konstantin Belousov]

On Mon, Feb 09, 2015 at 11:13:51PM +0000, Rui Paulo wrote:
> Author: rpaulo
> Date: Mon Feb  9 23:13:50 2015
> New Revision: 278479
> URL: https://svnweb.freebsd.org/changeset/base/278479
> 
> Log:
>   Notify devd(8) when a process crashed.
>   
>   This change implements a notification (via devctl) to userland when
>   the kernel produces coredumps after a process has crashed.
>   devd can then run a specific command to produce a human readable crash
>   report.  The command is most usually a helper that runs gdb/lldb
>   commands on the file/coredump pair.  It's possible to use this
>   functionality for implementing automatic generation of crash reports.
>   
>   devd(8) will be notified of the full path of the binary that crashed and
>   the full path of the coredump file.
Arguably, there should be a knob, probably sysctl, to turn the
functionality off. I definitely do not want this on crash boxes used for
userspace debugging.  Even despite the example handler is inactive.

> 
> Modified:
>   head/etc/devd.conf
>   head/sys/kern/kern_sig.c
> 
> Modified: head/etc/devd.conf
> ==============================================================================
> --- head/etc/devd.conf	Mon Feb  9 23:04:30 2015	(r278478)
> +++ head/etc/devd.conf	Mon Feb  9 23:13:50 2015	(r278479)
> @@ -325,4 +325,16 @@ notify 100 {
>  	action "/usr/sbin/automount -c";
>  };
>  
> +# Handle userland coredumps.
> +# This commented out handler makes it possible to run an
> +# automated debugging session after the core dump is generated.
> +# Replace action with a proper coredump handler, but be aware that
> +# it will run with elevated privileges.
> +notify 10 {
> +	match "system"          "kernel";
> +	match "subsystem"       "signal";
> +	match "type"            "coredump";
> +	action "logger $comm $core";
> +};
> +
>  */
> 
> Modified: head/sys/kern/kern_sig.c
> ==============================================================================
> --- head/sys/kern/kern_sig.c	Mon Feb  9 23:04:30 2015	(r278478)
> +++ head/sys/kern/kern_sig.c	Mon Feb  9 23:13:50 2015	(r278479)
> @@ -46,6 +46,7 @@ __FBSDID("$FreeBSD$");
>  #include <sys/signalvar.h>
>  #include <sys/vnode.h>
>  #include <sys/acct.h>
> +#include <sys/bus.h>
>  #include <sys/capsicum.h>
>  #include <sys/condvar.h>
>  #include <sys/event.h>
> @@ -3237,6 +3238,9 @@ coredump(struct thread *td)
>  	void *rl_cookie;
>  	off_t limit;
>  	int compress;
> +	char *data = NULL;
> +	size_t len;
> +	char *fullpath, *freepath = NULL;
>  
>  #ifdef COMPRESS_USER_CORES
>  	compress = compress_user_cores;
> @@ -3322,9 +3326,36 @@ close:
>  	error1 = vn_close(vp, FWRITE, cred, td);
>  	if (error == 0)
>  		error = error1;
> +	else
> +		goto out;
> +	/*
> +	 * Notify the userland helper that a process triggered a core dump.
> +	 * This allows the helper to run an automated debugging session.
> +	 */
> +	len = MAXPATHLEN * 2 + 5 /* comm= */ + 5 /* core= */ + 1;
It is much cleaner to use static const char arrays for the names,
and use sizeof() - 1 instead of hard-coding commented constants.

> +	data = malloc(len, M_TEMP, M_NOWAIT);
Why is this allocation M_NOWAIT ?

> +	if (data == NULL)
> +		goto out;
> +	if (vn_fullpath_global(td, p->p_textvp, &fullpath, &freepath) != 0)
> +		goto out;
> +	snprintf(data, len, "comm=%s", fullpath);
> +	if (freepath != NULL) {
> +		free(freepath, M_TEMP);
Checks for NULL pointer before free(9) are redundant.

> +		freepath = NULL;
> +	}
> +	if (vn_fullpath_global(td, vp, &fullpath, &freepath) != 0)
> +		goto out;
> +	snprintf(data, len, "%s core=%s", data, fullpath);
This is weird, and highly depends on the implementation details, supplying
the same string as target and source.  IMO strcat(9) is enough there.

> +	devctl_notify("kernel", "signal", "coredump", data);
> +	free(name, M_TEMP);
> +out:
>  #ifdef AUDIT
>  	audit_proc_coredump(td, name, error);
>  #endif
> +	if (freepath != NULL)
> +		free(freepath, M_TEMP);
> +	if (data != NULL)
> +		free(data, M_TEMP);
>  	free(name, M_TEMP);
>  	return (error);
>  }