Messages in this thread | | | Date | Mon, 1 Feb 2010 08:16:27 -0500 | From | Neil Horman <> | Subject | Re: [PATCH 2/2] exec: allow core_pipe recursion check to look for a value of 1 rather than 0 (v2) |
| |
On Mon, Feb 01, 2010 at 11:29:36AM +0100, Oleg Nesterov wrote: > On 01/31, Neil Horman wrote: > > > > On Sun, Jan 31, 2010 at 04:50:01PM +0100, Oleg Nesterov wrote: > > > On 01/29, Neil Horman wrote: > > > > > > > > void do_coredump(long signr, int exit_code, struct pt_regs *regs) > > > > { > > > > ... > > > > - if (call_usermodehelper_pipe(helper_argv[0], helper_argv, NULL, > > > > - &cprm.file)) { > > > > + cprm.file = NULL; > > > > > > it is already NULL, > > > > > Are we sure, it was declared on the stack. > > it must be NULL, or compiler is buggy. it was declared as "= { ... }". > > > I think its safer to ensure that its > > NULL. > > OK, agreed. I mentioned this just in case. > > > > > + if (call_usermodehelper_fns(helper_argv[0], helper_argv, NULL, > > > > + UMH_WAIT_EXEC, umh_pipe_setup, > > > > + NULL, &cprm)) { > > > > + if (cprm.file) > > > > + filp_close(cprm.file, NULL); > > > > > > Hmm. Looks like this change fixes the bug by accident. > > > > > > Before this patch, I think we leak info->stdin if kernel_thread() fails > > > in __call_usermodehelper() pathes. > > > > > I think we did that in call_usermodehelper_pipe. > > Afaics, no. Well yes, call_usermodehelper_pipe() closes write_pipe, > but I meant nobody closes read_pipe, info->stdin, if we fail before > ____call_usermodehelper() is called. > > > > Completely off-topic, but I think __call_usermodehelper(UMH_NO_WAIT) is > > > buggy. if kernel_thread() failes it should do call_usermodehelper_freeinfo(). > > > Also, UMH_WAIT_EXEC should set ->retval in this case. > > > > > I went down that path last time I changed this code, Andrew and I decided that > > yes it was buggy, but someone (can't recall how) smacked me around a bit and > > explained how it worked (some odd artifact behavior of the scheduler). Its in > > the lkml archives if you want to get the whole story. > > Hmm. I strongly believe this is buggy, and the scheduler can't help in any > way. Fortunately, kernel_thread() must "never" fail... > > Oh. And in theory, it is better to change wait_for_helper(). It should > do allow_signal(SIGCHLD) after kernel_thread(). Otherwise, kernel_thread() > can fail if user-space sends SIGCHLD to the forking thread. >
Commit 95e0d86badc410d525ea7218fd32df7bfbf9c837 has the discussion from the previous time that I messed with this code. Not sure how closely it relates, but...
Neil
>
| |