tech-kern archive
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]
Re: In-kernel process exit hooks?
OK, I now have a third way of handling the problem.
To recap the three options (refer to the attachments)
1. fd_getfile
This option calls fd_getfile() each time it needs to access the
activity log, and calls fd_putfile() after each use. This way,
the additional reference on the file descriptor lasts only for a
short period of time, and does not exist at any time that it can
be passed to fd_close().
The biggest drawback here is that the user-land application can
close() and re-open() this fd, possibly referring to a different
file; this will not be visible to filemon, which will write new
event records to any file that happens to be opened on this fd.
2. exithook
This option extends the existing exithook() mechanism to have
multiple "phases", one of which can happen before the process
exit code calls fd_free() (which in turn calls fd_close() for all
open file descriptors). The exithook registered by filemon finds
any usages of filemon and resets the activity-log, which releases
the extra reference to the log fd.
This option works well for normal process exit (including signal),
but does not resolve the problem if the application itself calls
close() on the log fd. In that situation, the process will still
hang.
Additionally, setting this up correctly is awkward, due to the
order in which kernel components are initialized. (Modules of
CLASS_DRIVER get loaded and initialized before exec_init() can
set up the hook mechanisms, so we need to use a config_finalizer
to establish the exit hook.)
3. filemon-fd_close
This solution introduces a new, filemon-specific callback in
fd_close() (but only if the filemon module is loaded or built-in).
Each time a file descriptor is passed to fd_close, the callback
is invoked. The callback checks each usage of /dev/filemon and
if that usage is logging activity to the file being closed, the
activity-log is reset, releasing the extra reference. Thus,
after we return to fd_close() the reference count is normal and
the file gets properly closed.
The only drawback I see here is the additional overhead of the
callback, on every call to fd_close(). The code catches every
occurrence of the "hang" that I can find, and handles it cleanly.
I still need to think about the fd_getfile2()/fclose() approach to see
if it meets our needs, but the comments that prohibit calling fork()
would seem to preclude this mechanism.
The "detach file from userland" approach suggested by kre would also
likely work, but I'm reluctant to change the semantics of filemon.
+------------------+--------------------------+------------------------+
| Paul Goyette | PGP Key fingerprint: | E-mail addresses: |
| (Retired) | FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com |
| Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org |
+------------------+--------------------------+------------------------+
Index: filemon.c
===================================================================
RCS file: /cvsroot/src/sys/dev/filemon/filemon.c,v
retrieving revision 1.24
diff -u -p -r1.24 filemon.c
--- filemon.c 5 Jan 2016 22:08:54 -0000 1.24
+++ filemon.c 7 Jan 2016 06:28:01 -0000
@@ -94,9 +94,21 @@ filemon_output(struct filemon * filemon,
{
struct uio auio;
struct iovec aiov;
+ struct file *fp; /* Output file pointer. */
- if (filemon->fm_fp == NULL)
+
+ if (filemon->fm_fp == NULL) /* No output file specified */
+ return;
+ if ((fp = fd_getfile(filemon->fm_fd)) != filemon->fm_fp) {
+ /*
+ * The file descriptor refers to a different file
+ * than when it was passed to SET_FD. So assume we
+ * never had an output file.
+ */
+ filemon->fm_fp = NULL;
+ filemon->fm_fd = -1;
return;
+ }
aiov.iov_base = msg;
aiov.iov_len = len;
@@ -122,6 +134,7 @@ filemon_output(struct filemon * filemon,
(*filemon->fm_fp->f_ops->fo_write) (filemon->fm_fp,
&(filemon->fm_fp->f_offset),
&auio, curlwp->l_cred, FOF_UPDATE_OFFSET);
+ fd_putfile(filemon->fm_fd); /* release our reference */
}
void
@@ -264,10 +277,7 @@ filemon_close(struct file * fp)
* once we have exclusive access, it should never be used again
*/
rw_enter(&filemon->fm_mtx, RW_WRITER);
- if (filemon->fm_fp) {
- fd_putfile(filemon->fm_fd); /* release our reference */
- filemon->fm_fp = NULL;
- }
+ filemon->fm_fp = NULL;
rw_exit(&filemon->fm_mtx);
rw_destroy(&filemon->fm_mtx);
kmem_free(filemon, sizeof(struct filemon));
@@ -309,6 +319,7 @@ filemon_ioctl(struct file * fp, u_long c
error = EBADF;
break;
}
+ fd_putfile(filemon->fm_fd);
/* Write the file header. */
filemon_comment(filemon);
break;
Index: dev/filemon/filemon.c
===================================================================
RCS file: /cvsroot/src/sys/dev/filemon/filemon.c,v
retrieving revision 1.24
diff -u -p -r1.24 filemon.c
--- dev/filemon/filemon.c 5 Jan 2016 22:08:54 -0000 1.24
+++ dev/filemon/filemon.c 7 Jan 2016 23:54:37 -0000
@@ -43,6 +43,8 @@ __KERNEL_RCSID(0, "$NetBSD: filemon.c,v
#include <sys/kmem.h>
#include <sys/syslog.h>
#include <sys/kauth.h>
+#include <sys/device.h>
+#include <sys/errno.h>
#include "filemon.h"
#include "ioconf.h"
@@ -89,6 +91,72 @@ static TAILQ_HEAD(, filemon) filemons_in
static int logLevel = LOG_DEBUG;
#endif
+/*
+ * Use an exithook to make sure that the filemon device is closed and
+ * our additional reference to the output file is released before the
+ * normal closure of open files. The extra reference would otherwise
+ * cause process exit to hang indefinitely.
+ *
+ * We need to use a config_finalize() routine to register the hook
+ * due to system initialization sequence.
+ */
+void *hook = NULL; /* cookie from exithook_establish() */
+
+static int filemon_register_hook(device_t dev __unused);
+static void filemon_exithook(struct proc *p, void *arg);
+
+static int
+filemon_register_hook(device_t dev __unused)
+{
+
+ if (hook == NULL)
+ hook = exithook_establish(filemon_exithook, hook,
+ EXITHOOK_BEFORE_CLOSE);
+ if (hook != NULL)
+ return 0;
+ return ENXIO;
+}
+
+static void
+filemon_exithook(struct proc *p, void *arg)
+{
+ int fd;
+ struct filemon *filemon;
+ fdfile_t *ff;
+ filedesc_t *fdp;
+ fdtab_t *dt;
+
+ fdp = p->p_fd;
+ dt = fdp->fd_dt;
+ for (fd = 0; fd < dt->dt_nfiles; fd++) {
+ ff = dt->dt_ff[fd];
+ KASSERT(fd >= NDFDFILE ||
+ ff == (fdfile_t *)fdp->fd_dfdfile[fd]);
+ if (ff == NULL || ff->ff_file == NULL)
+ continue;
+ if (ff->ff_file->f_type != DTYPE_MISC ||
+ ff->ff_file->f_ops != &filemon_fileops)
+ continue;
+ rw_enter(&filemon_mtx, RW_WRITER);
+ filemon = ff->ff_file->f_data;
+ if (filemon != NULL) {
+
+ /*
+ * If we have an output file, release our
+ * reference to it.
+ */
+ rw_enter(&filemon->fm_mtx, RW_WRITER);
+ if (filemon->fm_fp) {
+ KASSERT(p == curproc);
+ fd_putfile(filemon->fm_fd);
+ filemon->fm_fp = NULL;
+ }
+ rw_exit(&filemon->fm_mtx);
+ }
+ rw_exit(&filemon_mtx);
+ }
+}
+
void
filemon_output(struct filemon * filemon, char *msg, size_t len)
{
@@ -216,6 +284,9 @@ filemon_open(dev_t dev, int oflags __unu
struct file *fp;
int error, fd;
+ if (hook == NULL)
+ return ENXIO;
+
/* falloc() will fill in the descriptor for us. */
if ((error = fd_allocfile(&fp, &fd)) != 0)
return error;
@@ -399,19 +471,35 @@ filemon_modcmd(modcmd_t cmd, void *data)
switch (cmd) {
case MODULE_CMD_INIT:
+ KASSERT(hook == NULL);
#ifdef DEBUG
logLevel = LOG_INFO;
#endif
error = filemon_load(data);
+ if (error != 0)
+ break;
#ifdef _MODULE
- if (!error)
- error = devsw_attach("filemon", NULL, &bmajor,
- &filemon_cdevsw, &cmajor);
+ error = devsw_attach("filemon", NULL, &bmajor,
+ &filemon_cdevsw, &cmajor);
+ if (error != 0) {
+ filemon_unload();
+ break;
+ }
#endif
+ error = config_finalize_register(NULL, filemon_register_hook);
+ if (error != 0) {
+ devsw_detach(NULL, &filemon_cdevsw);
+ filemon_unload();
+ break;
+ }
break;
case MODULE_CMD_FINI:
+ if (hook != NULL) {
+ exithook_disestablish(hook, EXITHOOK_BEFORE_CLOSE);
+ hook = NULL;
+ }
error = filemon_unload();
#ifdef _MODULE
if (!error)
Index: kern/kern_exit.c
===================================================================
RCS file: /cvsroot/src/sys/kern/kern_exit.c,v
retrieving revision 1.248
diff -u -p -r1.248 kern_exit.c
--- kern/kern_exit.c 13 Oct 2015 06:47:21 -0000 1.248
+++ kern/kern_exit.c 7 Jan 2016 23:55:02 -0000
@@ -279,10 +279,11 @@ exit1(struct lwp *l, int rv)
* Close open files, release open-file table and free signal
* actions. This may block!
*/
+ doexithooks(p, EXITHOOK_BEFORE_CLOSE);
fd_free();
cwdfree(p->p_cwdi);
p->p_cwdi = NULL;
- doexithooks(p);
+ doexithooks(p, EXITHOOK_AFTER_CLOSE);
sigactsfree(p->p_sigacts);
/*
Index: kern/kern_hook.c
===================================================================
RCS file: /cvsroot/src/sys/kern/kern_hook.c,v
retrieving revision 1.6
diff -u -p -r1.6 kern_hook.c
--- kern/kern_hook.c 22 Nov 2013 21:04:11 -0000 1.6
+++ kern/kern_hook.c 7 Jan 2016 23:55:13 -0000
@@ -219,26 +219,33 @@ doexechooks(struct proc *p)
hook_proc_run(&exechook_list, p);
}
-static hook_list_t exithook_list = LIST_HEAD_INITIALIZER(exithook_list);
+static hook_list_t exithook_list0 = LIST_HEAD_INITIALIZER(exithook_list0);
+static hook_list_t exithook_list1 = LIST_HEAD_INITIALIZER(exithook_list1);
+static hook_list_t *exithook_table[] = {
+ &exithook_list0, /* EXITHOOK_BEFORE_CLOSE */
+ &exithook_list1, /* EXITHOOK_AFTER_CLOSE */
+};
extern krwlock_t exec_lock;
void *
-exithook_establish(void (*fn)(struct proc *, void *), void *arg)
+exithook_establish(void (*fn)(struct proc *, void *), void *arg, int ph)
{
void *rv;
+ KASSERT(ph >= 0 && ph < __arraycount(exithook_table));
rw_enter(&exec_lock, RW_WRITER);
- rv = hook_establish(&exithook_list, (void (*)(void *))fn, arg);
+ rv = hook_establish(exithook_table[ph], (void (*)(void *))fn, arg);
rw_exit(&exec_lock);
return rv;
}
void
-exithook_disestablish(void *vhook)
+exithook_disestablish(void *vhook, int ph)
{
+ KASSERT(ph >= 0 && ph < __arraycount(exithook_table));
rw_enter(&exec_lock, RW_WRITER);
- hook_disestablish(&exithook_list, vhook);
+ hook_disestablish(exithook_table[ph], vhook);
rw_exit(&exec_lock);
}
@@ -246,9 +253,11 @@ exithook_disestablish(void *vhook)
* Run exit hooks.
*/
void
-doexithooks(struct proc *p)
+doexithooks(struct proc *p, int ph)
{
- hook_proc_run(&exithook_list, p);
+
+ KASSERT(ph >= 0 && ph < __arraycount(exithook_table));
+ hook_proc_run(exithook_table[ph], p);
}
static hook_list_t forkhook_list = LIST_HEAD_INITIALIZER(forkhook_list);
Index: kern/sys_aio.c
===================================================================
RCS file: /cvsroot/src/sys/kern/sys_aio.c,v
retrieving revision 1.40
diff -u -p -r1.40 sys_aio.c
--- kern/sys_aio.c 5 Sep 2014 09:20:59 -0000 1.40
+++ kern/sys_aio.c 7 Jan 2016 23:55:29 -0000
@@ -131,7 +131,7 @@ aio_fini(bool interface)
sysctl_teardown(&aio_sysctl);
KASSERT(aio_jobs_count == 0);
- exithook_disestablish(aio_ehook);
+ exithook_disestablish(aio_ehook, EXITHOOK_AFTER_CLOSE);
pool_destroy(&aio_job_pool);
pool_destroy(&aio_lio_pool);
return 0;
@@ -149,7 +149,7 @@ aio_init(void)
"aio_jobs_pool", &pool_allocator_nointr, IPL_NONE);
pool_init(&aio_lio_pool, sizeof(struct lio_req), 0, 0, 0,
"aio_lio_pool", &pool_allocator_nointr, IPL_NONE);
- aio_ehook = exithook_establish(aio_exit, NULL);
+ aio_ehook = exithook_establish(aio_exit, NULL, EXITHOOK_AFTER_CLOSE);
error = sysctl_aio_init();
if (error != 0) {
Index: kern/sysv_sem.c
===================================================================
RCS file: /cvsroot/src/sys/kern/sysv_sem.c,v
retrieving revision 1.95
diff -u -p -r1.95 sysv_sem.c
--- kern/sysv_sem.c 6 Nov 2015 02:26:42 -0000 1.95
+++ kern/sysv_sem.c 7 Jan 2016 23:55:46 -0000
@@ -153,7 +153,7 @@ static int
seminit_exithook(void)
{
- hook = exithook_establish(semexit, NULL);
+ hook = exithook_establish(semexit, NULL, EXITHOOK_AFTER_CLOSE);
return 0;
}
@@ -172,7 +172,7 @@ semfini(void)
/* Remove the exit hook */
if (hook)
- exithook_disestablish(hook);
+ exithook_disestablish(hook, EXITHOOK_AFTER_CLOSE);
/* Destroy all our condvars */
for (i = 0; i < seminfo.semmni; i++) {
Index: rump/librump/rumpkern/lwproc.c
===================================================================
RCS file: /cvsroot/src/sys/rump/librump/rumpkern/lwproc.c,v
retrieving revision 1.35
diff -u -p -r1.35 lwproc.c
--- rump/librump/rumpkern/lwproc.c 18 Apr 2015 15:49:18 -0000 1.35
+++ rump/librump/rumpkern/lwproc.c 7 Jan 2016 23:56:08 -0000
@@ -117,7 +117,8 @@ lwproc_proc_free(struct proc *p)
chgproccnt(kauth_cred_getuid(cred), -1);
rump_proc_vfs_release(p);
- doexithooks(p);
+ doexithooks(p, EXITHOOK_BEFORE_CLOSE);
+ doexithooks(p, EXITHOOK_AFTER_CLOSE);
lim_free(p->p_limit);
pstatsfree(p->p_stats);
kauth_cred_free(p->p_cred);
Index: sys/systm.h
===================================================================
RCS file: /cvsroot/src/sys/sys/systm.h,v
retrieving revision 1.269
diff -u -p -r1.269 systm.h
--- sys/systm.h 29 Oct 2015 00:27:08 -0000 1.269
+++ sys/systm.h 7 Jan 2016 23:56:23 -0000
@@ -375,9 +375,11 @@ void doexechooks(struct proc *);
/*
* Exit hooks. Subsystems may want to do cleanup when a process exits.
*/
-void *exithook_establish(void (*)(struct proc *, void *), void *);
-void exithook_disestablish(void *);
-void doexithooks(struct proc *);
+#define EXITHOOK_BEFORE_CLOSE 0
+#define EXITHOOK_AFTER_CLOSE 1
+void *exithook_establish(void (*)(struct proc *, void *), void *, int);
+void exithook_disestablish(void *, int);
+void doexithooks(struct proc *, int);
/*
* Fork hooks. Subsystems may want to do special processing when a process
Index: dev/filemon/filemon.c
===================================================================
RCS file: /cvsroot/src/sys/dev/filemon/filemon.c,v
retrieving revision 1.24
diff -u -p -r1.24 filemon.c
--- dev/filemon/filemon.c 5 Jan 2016 22:08:54 -0000 1.24
+++ dev/filemon/filemon.c 8 Jan 2016 04:25:08 -0000
@@ -89,6 +89,45 @@ static TAILQ_HEAD(, filemon) filemons_in
static int logLevel = LOG_DEBUG;
#endif
+/*
+ * Call-back procedure for fd_close, to ensure we don't have any
+ * references to the file descriptor being closed.
+ */
+void filemon_fd_close(int);
+
+void
+filemon_fd_close(int fd)
+{
+ struct filemon *filemon;
+ filedesc_t *fdp;
+ fdtab_t *dt;
+ fdfile_t *ff;
+
+ fdp = curproc->p_fd;
+ dt = fdp->fd_dt;
+ ff = dt->dt_ff[fd];
+ KASSERT(fd >= NDFDFILE ||
+ ff == (fdfile_t *)(fdp->fd_dfdfile[fd]));
+ if (ff == NULL || ff->ff_file == NULL)
+ return;
+
+ /*
+ * search all filemon's to see if any is logging to this file
+ */
+ rw_enter(&filemon_mtx, RW_WRITER);
+ TAILQ_FOREACH(filemon, &filemons_inuse, fm_link) {
+ if (filemon->fm_fp == ff->ff_file &&
+ filemon->fm_fd == fd) {
+ rw_enter(&filemon->fm_mtx, RW_WRITER);
+ fd_putfile(fd);
+ filemon->fm_fd = -1;
+ filemon->fm_fp = NULL;
+ rw_exit(&filemon->fm_mtx);
+ }
+ }
+ rw_exit(&filemon_mtx);
+}
+
void
filemon_output(struct filemon * filemon, char *msg, size_t len)
{
@@ -404,19 +443,39 @@ filemon_modcmd(modcmd_t cmd, void *data)
#endif
error = filemon_load(data);
+ if (error != 0)
+ break;
+
#ifdef _MODULE
- if (!error)
- error = devsw_attach("filemon", NULL, &bmajor,
- &filemon_cdevsw, &cmajor);
+ error = devsw_attach("filemon", NULL, &bmajor,
+ &filemon_cdevsw, &cmajor);
+ if (error != 0) {
+ filemon_unload();
+ break;
+ }
#endif
+
+ fd_close_filemon = filemon_fd_close;
+
break;
case MODULE_CMD_FINI:
+
+#ifdef _MODULE
+ error = devsw_detach(NULL, &filemon_cdevsw);
+ if (error != 0)
+ break;
+#endif
+
error = filemon_unload();
+ if (error != 0) {
#ifdef _MODULE
- if (!error)
- error = devsw_detach(NULL, &filemon_cdevsw);
+ devsw_attach("filemon", NULL, &bmajor,
+ &filemon_cdevsw, &cmajor);
#endif
+ break;
+ }
+ fd_close_filemon = NULL;
break;
case MODULE_CMD_STAT:
cvs [diff aborted]: no such directory `sys/kern'
cvs [diff aborted]: no such directory `sys/sys'
Home |
Main Index |
Thread Index |
Old Index