Re: [PATCH] fork.c: copy_process(): fix cleanup WRT perf_event_free_task()

[Oleg Nesterov]

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.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/