Messages in this thread | | | Date | Sat, 27 Sep 2014 20:07:25 +0200 | From | Oleg Nesterov <> | Subject | Re: [PATCH] fork.c: copy_process(): fix cleanup WRT perf_event_free_task() |
| |
On 09/26, Sylvain 'ythier' Hitier wrote: > > retval = sched_fork(clone_flags, p); > if (retval) > // // mustn't perf_event_free_task() > goto bad_fork_cleanup_policy;
Agreed, this is wrong. Good catch.
but, unless I missed something,
> retval = perf_event_init_task(p); > if (retval) > // // mustn't perf_event_free_task() ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
this is not right and thus the patch is not right too.
Suppose that perf_event_init_task() -> perf_event_init_context(ctxn => 0) succeeds and then perf_event_init_context(ctxn => 1) fails, we need perf_event_free_task() to cleanup ->perf_event_ctxp[0].
So if perf_event_init_task() fails, we still need "goto bad_fork_cleanup_perf".
No?
Or, probably better, we need to change perf_event_init_context() to call perf_event_free_task() on failure.
Or. We can simply move memset(child->perf_event_ctxp, 0, ...) from perf_event_init_context() up. This reminds that we really need to cleanup copy_process(), in particular I think it asks for the new copy_xxx() helper which should do misc simple initializations which can't fail.
What do you think?
Oleg.
| |