code calls rad_fork(1) (such as rad_check_ts()) This means that no entry is stored in thread_pool.waiters This means that rad_waitpid() calls reap_children() which means that subsequent calls to waitpid() -- since there is no thread_pool.waiters entry for the pid -- return an ECHILD, which leads rad_check_ts() to print this message radlog(L_ERR, "Check-TS: unknown error in waitpid()"); So basically rad_fork(1) and rad_waitpid() and waitpid() are now unreliable. Furthermore, using rad_fork(0) without rad_waitpid(pid,...,...) will eventualy result in a full thread_pool.waiters hashtable and calls to rad_fork(0) will fail. So waitpid(0,...,...) in reap_children() buys nothing except elimination of zombie processes by trading it in for causing calls of rad_fork(0) to fail. (why 0 and not -1 main waitpid..... -1 meaning wait for any child process. 0 meaning wait for any child process whose process group ID is equal to that of the calling process. ) I am getting errors forking my scripts off with rlm_exec -- apparently thread_pool.waiters is getting full somehow, since this is logged. Error: Couldn't fork /usr/local/sbin/radius-dns-update.sh: No child processes Which isnt a documented errno for fork() grep "No child" /usr/include/ -r /usr/include/asm-generic/errno-base.h:#define ECHILD 10 /* No child processes */ The fix? Here are some ideas. 1) reap_children() should only call waitpid() for entries in thread_pool.waiters -- this was the old way, should be fairly easy to revert to. This was changed due to zombies being created by creative forking in rlm_perl. I think that problem should have properly been resolved by using rad_fork() or by building its own long lived fork/wait mechanism or by properly daemonizing the child process. -- rad_fork(1)s or fork()s that are not followed by waitpid()s will result in zombies (which is a bug owned by the code doing the call to fork) IMHO this makes the most sense -- freeradius should not try to fix other code bugs by introducing all the breakage as descibed above. 2) reap_children() should create a thread_pool.waiters entry for pids that have exited for subsequent retrieval of status by rad_waitpid -- rad_fork(1)s or fork()s that are not followed by waitpid()s will result in thread_pool.waiters running out of elements. -- waitpid() is still unreliable -- rad_fork(1) is still (nearly) meaningless This can be handled by rad_fork() behaving differently, such as deleting all thread_pool.waiters elements that created by reap_children() if the thread_pool.waiters reaches its limit. This is a series of nasty kludges and workarounds for a problem that should exist in the first place. Attached find proof-of-concept patch 3) calls to rad_fork(1) should also result in creation of thread_pool.waiters element -- This doesnt completely solve anything and 2 is still needed. -- rad_fork(1) is meaningless 4) rad_fork() should mark elements in the hash table as deletable when the table is full -- after all, this is what exec_wait == 0 means, right? Perhaps to make it as correct as possible, it should only delete a percentage of the table, sorted by oldest entry. That might be hard. Why have a waiters pool anyway when calling waitpid(0 ? exec_wait == 1 processes get waited upon and their status discarded exec_wait == 0 processes get waited upon and their status kept until an explicit call to rad_waitpid() with the pid. #! /bin/sh /usr/share/dpatch/dpatch-run ## 360-rad_check_ts_waitpid.dpatch by <joe@nameserver3.ttec.com> ## ## All lines beginning with `## DP:' are a description of the patch. ## DP: No description. @DPATCH@ diff -urNad freeradius-2.0.0~/src/main/threads.c freeradius-2.0.0/src/main/threads.c --- freeradius-2.0.0~/src/main/threads.c 2007-01-17 09:00:25.000000000 -0500 +++ freeradius-2.0.0/src/main/threads.c 2007-01-17 09:02:37.000000000 -0500 @@ -113,10 +113,12 @@ RAD_REQUEST_FUNP fun; } request_queue_t; +#define MAX_WAITERS 1024 typedef struct thread_fork_t { pid_t pid; int status; int exited; + int reaper_created; } thread_fork_t; @@ -257,10 +259,10 @@ pid_t pid; int status; thread_fork_t mytf, *tf; - - if (lrad_hash_table_num_elements(thread_pool.waiters) == 0) return; - - pthread_mutex_lock(&thread_pool.wait_mutex); + int num_waiters = lrad_hash_table_num_elements(thread_pool.waiters); + + if (!num_waiters) + return; while (1) { pid = waitpid(0, &status, WNOHANG); @@ -268,12 +270,26 @@ mytf.pid = pid; tf = lrad_hash_table_finddata(thread_pool.waiters, &mytf); - if (!tf) continue; - - tf->status = status; + if (!tf) + { + tf = rad_malloc(sizeof(*tf)); + memset(tf, 0, sizeof(*tf)); + tf->pid = pid; + tf->status = status; + tf->exited = 1; + tf->reaper_created = 1; + if (!lrad_hash_table_insert(thread_pool.waiters, tf)) + { + free(tf); + tf = NULL; + } + if (++num_waiters >= MAX_WAITERS) + break; + continue; + } tf->exited = 1; + tf->status = status; } - pthread_mutex_unlock(&thread_pool.wait_mutex); } @@ -1049,16 +1065,29 @@ /* * Thread wrapper for fork(). */ +static int reap_reaper_children(void * context, void * data) +{ + thread_fork_t *tf = data; + if (tf->reaper_created) + lrad_hash_table_delete(thread_pool.waiters, tf); + return 0; +} + pid_t rad_fork(int exec_wait) { pid_t child_pid; + int num_waiters; if (exec_wait) return fork(); reap_children(); /* be nice to non-wait thingies */ - if (lrad_hash_table_num_elements(thread_pool.waiters) >= 1024) { - return -1; + if (lrad_hash_table_num_elements(thread_pool.waiters) >= MAX_WAITERS) { + lrad_hash_table_walk(thread_pool.waiters, reap_reaper_children, NULL); + if ((num_waiters = lrad_hash_table_num_elements(thread_pool.waiters)) >= MAX_WAITERS) { + radlog(L_ERR, "too many child waiters: %d", num_waiters); + return -1; + } } /* @@ -1104,8 +1133,8 @@ pid_t rad_waitpid(pid_t pid, int *status, int options) { thread_fork_t mytf, *tf; + pid_t child_pid = 0; - reap_children(); /* be nice to non-wait thingies */ if (pid <= 0) return -1; @@ -1117,22 +1146,21 @@ tf = lrad_hash_table_finddata(thread_pool.waiters, &mytf); if (!tf) { /* not found. It's a problem... */ - pthread_mutex_unlock(&thread_pool.wait_mutex); - return waitpid(pid, status, options); + child_pid = waitpid(pid, status, options); } - - if (tf->exited) { + else if (tf->exited) { *status = tf->status; lrad_hash_table_delete(thread_pool.waiters, &mytf); - pthread_mutex_unlock(&thread_pool.wait_mutex); - return pid; + child_pid = pid; } - /* - * Don't wait, and it hasn't exited. Return. + * else { + * Don't wait, and it hasn't exited. Return 0. + * } */ + reap_children(); /* be nice to non-wait thingies */ pthread_mutex_unlock(&thread_pool.wait_mutex); - return 0; + return child_pid; } #else /* HAVE_PTHREAD_H */
Joe Maimon wrote:
code calls rad_fork(1) (such as rad_check_ts())
This means that no entry is stored in thread_pool.waiters
It looks like that's a bug in rad_fork. It should probably be doing if (!exec_wait) return fork; Oops.
(why 0 and not -1 main waitpid.....
-1 meaning wait for any child process.
0 meaning wait for any child process whose process group ID is equal to that of the calling process.
I don't have strong opinions here.
The fix? Here are some ideas.
Try adding the '!' to rad_fork. The problem that horrible code in threads.c is trying to solve is getting the child exit condition to the correct thread. The waitpid() function call doesn't have an argument saying "wait only for programs forked from this thread.". So one thread calling waitpid() can get the exit condition from a child forked from another thread. The solution (such as it is) is a global list of children. When a thread forks a process, and indicates that it cares about the exit condition, that PID is placed into the global list. When any thread calls waitpid(), it looks up the resulting PID in the list. If it's found, then the child exit status is updated. There's also the case of a thread that wants to call waitpid(), but another thread has already grabbed the exit code, and updated the global list. Hence rad_waitpid(). That looks in the list, and if the PID is found, clears the entry and returns. If an entry isn't found, it calls waitpid (as above), and updates the list with the status of any child that has exited. There's another case, where a child forks a thread, and doesn't care about the exit status. In that case, the child PID isn't put into the list. I suppose the code could be made a little simpler by removing the argument to rad_fork(). That way, any thread that cared about the exit condition would call rad_fork(), and threads that didn't care would call fork(). I think that covers all cases, without involving massive code changes. So all instances of rad_fork() should be audited, to ensure that they're being used correctly. Some can probably be changed to use fork(). Alan DeKok. -- http://deployingradius.com - The web site of the book http://deployingradius.com/blog/ - The blog
Alan DeKok wrote:
Joe Maimon wrote:
code calls rad_fork(1) (such as rad_check_ts())
This means that no entry is stored in thread_pool.waiters
It looks like that's a bug in rad_fork. It should probably be doing
if (!exec_wait) return fork;
Oops.
That makes more sense. I still have my doubts that calling waipid(0 is the right thing to do.
Joe Maimon wrote:
That makes more sense.
I'll fix that, then.
I still have my doubts that calling waipid(0 is the right thing to do.
radiusd calls setpgid() itself for a reason. If a child process calls setpgid(), then we don't want radiusd to steal the exit conditions from those processes. Alan DeKok. -- http://deployingradius.com - The web site of the book http://deployingradius.com/blog/ - The blog
Joe Maimon wrote:
That makes more sense.
Please try the attached patch. It turns out that the only calls to rad_fork() are in src/main, and everything else calls radius_exec_program(). Alan DeKok. -- http://deployingradius.com - The web site of the book http://deployingradius.com/blog/ - The blog Index: src/include/radiusd.h =================================================================== RCS file: /source/radiusd/src/include/radiusd.h,v retrieving revision 1.156.2.4 diff -u -r1.156.2.4 radiusd.h --- src/include/radiusd.h 11 Apr 2005 23:45:21 -0000 1.156.2.4 +++ src/include/radiusd.h 18 Jan 2007 15:05:15 -0000 @@ -405,7 +405,7 @@ extern int thread_pool_init(void); extern int thread_pool_clean(time_t now); extern int thread_pool_addrequest(REQUEST *, RAD_REQUEST_FUNP); -extern pid_t rad_fork(int exec_wait); +extern pid_t rad_fork(void); extern pid_t rad_waitpid(pid_t pid, int *status, int options); extern int total_active_threads(void); Index: src/main/threads.c =================================================================== RCS file: /source/radiusd/src/main/threads.c,v retrieving revision 1.77.2.7 diff -u -r1.77.2.7 threads.c --- src/main/threads.c 17 Oct 2005 18:38:07 -0000 1.77.2.7 +++ src/main/threads.c 18 Jan 2007 15:05:15 -0000 @@ -982,12 +982,10 @@ /* * Thread wrapper for fork(). */ -pid_t rad_fork(int exec_wait) +pid_t rad_fork(void) { pid_t child_pid; - if (exec_wait) return fork(); - /* * Ensure that children are reaped always. */ Index: src/main/exec.c =================================================================== RCS file: /source/radiusd/src/main/exec.c,v retrieving revision 1.42.2.1.2.2 diff -u -r1.42.2.1.2.2 exec.c --- src/main/exec.c 6 Oct 2006 23:39:21 -0000 1.42.2.1.2.2 +++ src/main/exec.c 18 Jan 2007 15:05:16 -0000 @@ -311,7 +311,13 @@ output_pairs = NULL; } - if ((pid = rad_fork(exec_wait)) == 0) { + if (exec_wait) { + pid = rad_fork(); /* remember PID */ + } else { + pid = fork(); /* don't wait */ + } + + if (pid == 0) { #define MAX_ENVP 1024 int devnull; char *envp[MAX_ENVP]; Index: src/main/session.c =================================================================== RCS file: /source/radiusd/src/main/session.c,v retrieving revision 1.21.2.1.2.1 diff -u -r1.21.2.1.2.1 session.c --- src/main/session.c 24 Mar 2006 17:54:57 -0000 1.21.2.1.2.1 +++ src/main/session.c 18 Jan 2007 15:05:16 -0000 @@ -155,7 +155,7 @@ /* * Fork. */ - if ((pid = rad_fork(1)) < 0) { /* do wait for the fork'd result */ + if ((pid = rad_fork()) < 0) { /* do wait for the fork'd result */ radlog(L_ERR, "Accounting: Failed in fork(): Cannot run checkrad\n"); return -1; }
Alan DeKok wrote:
Joe Maimon wrote:
That makes more sense.
Please try the attached patch.
Lots of zombies in a short time If rad_fork() is not called frequently does reap_children() [or the waitpid() inside reap_children()] run in a predictable fashion?
It turns out that the only calls to rad_fork() are in src/main, and everything else calls radius_exec_program().
Alan DeKok. -- http://deployingradius.com - The web site of the book http://deployingradius.com/blog/ - The blog
------------------------------------------------------------------------
Index: src/include/radiusd.h =================================================================== RCS file: /source/radiusd/src/include/radiusd.h,v retrieving revision 1.156.2.4 diff -u -r1.156.2.4 radiusd.h --- src/include/radiusd.h 11 Apr 2005 23:45:21 -0000 1.156.2.4 +++ src/include/radiusd.h 18 Jan 2007 15:05:15 -0000 @@ -405,7 +405,7 @@ extern int thread_pool_init(void); extern int thread_pool_clean(time_t now); extern int thread_pool_addrequest(REQUEST *, RAD_REQUEST_FUNP); -extern pid_t rad_fork(int exec_wait); +extern pid_t rad_fork(void); extern pid_t rad_waitpid(pid_t pid, int *status, int options); extern int total_active_threads(void);
Index: src/main/threads.c =================================================================== RCS file: /source/radiusd/src/main/threads.c,v retrieving revision 1.77.2.7 diff -u -r1.77.2.7 threads.c --- src/main/threads.c 17 Oct 2005 18:38:07 -0000 1.77.2.7 +++ src/main/threads.c 18 Jan 2007 15:05:15 -0000 @@ -982,12 +982,10 @@ /* * Thread wrapper for fork(). */ -pid_t rad_fork(int exec_wait) +pid_t rad_fork(void) { pid_t child_pid;
- if (exec_wait) return fork(); - /* * Ensure that children are reaped always. */ Index: src/main/exec.c =================================================================== RCS file: /source/radiusd/src/main/exec.c,v retrieving revision 1.42.2.1.2.2 diff -u -r1.42.2.1.2.2 exec.c --- src/main/exec.c 6 Oct 2006 23:39:21 -0000 1.42.2.1.2.2 +++ src/main/exec.c 18 Jan 2007 15:05:16 -0000 @@ -311,7 +311,13 @@ output_pairs = NULL; }
- if ((pid = rad_fork(exec_wait)) == 0) { + if (exec_wait) { + pid = rad_fork(); /* remember PID */ + } else { + pid = fork(); /* don't wait */ + } + + if (pid == 0) { #define MAX_ENVP 1024 int devnull; char *envp[MAX_ENVP]; Index: src/main/session.c =================================================================== RCS file: /source/radiusd/src/main/session.c,v retrieving revision 1.21.2.1.2.1 diff -u -r1.21.2.1.2.1 session.c --- src/main/session.c 24 Mar 2006 17:54:57 -0000 1.21.2.1.2.1 +++ src/main/session.c 18 Jan 2007 15:05:16 -0000 @@ -155,7 +155,7 @@ /* * Fork. */ - if ((pid = rad_fork(1)) < 0) { /* do wait for the fork'd result */ + if ((pid = rad_fork()) < 0) { /* do wait for the fork'd result */ radlog(L_ERR, "Accounting: Failed in fork(): Cannot run checkrad\n"); return -1; }
------------------------------------------------------------------------
- List info/subscribe/unsubscribe? See http://www.freeradius.org/list/devel.html
Joe Maimon wrote:
Alan DeKok wrote:
Joe Maimon wrote:
That makes more sense.
Please try the attached patch.
Lots of zombies in a short time
If rad_fork() is not called frequently does reap_children() [or the waitpid() inside reap_children()] run in a predictable fashion?
It works a whole lot better if I remove the conditional return before waitpid() in reap_children()
Joe Maimon wrote:
It works a whole lot better if I remove the conditional return before waitpid() in reap_children()
I'm not sure what that means... which code are you referring to? Alan DeKok. -- http://deployingradius.com - The web site of the book http://deployingradius.com/blog/ - The blog
Alan DeKok wrote:
Joe Maimon wrote:
It works a whole lot better if I remove the conditional return before waitpid() in reap_children()
I'm not sure what that means... which code are you referring to?
Alan DeKok.
This is the patch I am running with. #! /bin/sh /usr/share/dpatch/dpatch-run ## 370-rad_fork.dpatch by <joe@nameserver3.ttec.com> ## ## All lines beginning with `## DP:' are a description of the patch. ## DP: No description. @DPATCH@ diff -urNad freeradius-2.0.0~/src/include/radiusd.h freeradius-2.0.0/src/include/radiusd.h --- freeradius-2.0.0~/src/include/radiusd.h 2007-01-19 04:47:11.000000000 -0500 +++ freeradius-2.0.0/src/include/radiusd.h 2007-01-19 05:36:01.000000000 -0500 @@ -447,7 +447,7 @@ extern int thread_pool_init(int spawn_flag); extern int thread_pool_clean(time_t now); extern int thread_pool_addrequest(REQUEST *, RAD_REQUEST_FUNP); -extern pid_t rad_fork(int exec_wait); +extern pid_t rad_fork(void); extern pid_t rad_waitpid(pid_t pid, int *status, int options); extern int total_active_threads(void); diff -urNad freeradius-2.0.0~/src/main/exec.c freeradius-2.0.0/src/main/exec.c --- freeradius-2.0.0~/src/main/exec.c 2007-01-19 04:47:11.000000000 -0500 +++ freeradius-2.0.0/src/main/exec.c 2007-01-19 05:36:01.000000000 -0500 @@ -226,7 +226,13 @@ output_pairs = NULL; } - if ((pid = rad_fork(exec_wait)) == 0) { + if (exec_wait) { + pid = rad_fork(); /* remember PID */ + } else { + pid = fork(); /* don't wait */ + } + + if (pid == 0) { #define MAX_ENVP 1024 int devnull; char *envp[MAX_ENVP]; diff -urNad freeradius-2.0.0~/src/main/session.c freeradius-2.0.0/src/main/session.c --- freeradius-2.0.0~/src/main/session.c 2007-01-19 04:47:11.000000000 -0500 +++ freeradius-2.0.0/src/main/session.c 2007-01-19 05:36:01.000000000 -0500 @@ -168,7 +168,7 @@ /* * Fork. */ - if ((pid = rad_fork(1)) < 0) { /* do wait for the fork'd result */ + if ((pid = rad_fork()) < 0) { /* do wait for the fork'd result */ radlog(L_ERR, "Accounting: Failed in fork(): Cannot run checkrad\n"); return 2; } diff -urNad freeradius-2.0.0~/src/main/threads.c freeradius-2.0.0/src/main/threads.c --- freeradius-2.0.0~/src/main/threads.c 2007-01-19 04:47:11.000000000 -0500 +++ freeradius-2.0.0/src/main/threads.c 2007-01-19 05:40:50.000000000 -0500 @@ -258,11 +258,10 @@ int status; thread_fork_t mytf, *tf; - if (lrad_hash_table_num_elements(thread_pool.waiters) == 0) return; pthread_mutex_lock(&thread_pool.wait_mutex); - while (1) { + do { pid = waitpid(0, &status, WNOHANG); if (pid <= 0) break; @@ -272,7 +271,7 @@ tf->status = status; tf->exited = 1; - } + } while (lrad_hash_table_num_elements(thread_pool.waiters)); pthread_mutex_unlock(&thread_pool.wait_mutex); } @@ -1049,11 +1048,10 @@ /* * Thread wrapper for fork(). */ -pid_t rad_fork(int exec_wait) +pid_t rad_fork(void) { pid_t child_pid; - if (exec_wait) return fork(); reap_children(); /* be nice to non-wait thingies */
Joe Maimon wrote:
This is the patch I am running with.
Added, thanks. Alan DeKok. -- http://deployingradius.com - The web site of the book http://deployingradius.com/blog/ - The blog
participants (2)
-
Alan DeKok -
Joe Maimon