fork.c: copy_process(): fix cleanup WRT perf_event_free_task()

Date: Fri Sep 26 20:56:07 2014 +0000

fork.c: copy_process(): fix cleanup WRT perf_event_free_task()

Currently, in copy_process(), a failure of either sched_fork() or perf_event_init_task() does trigger perf_event_free_task()!

Moreover, the label bad_fork_cleanup_policy does more than what its name implies, because it includes perf_event_free_task()!

Let's explain the change with a grASCIIcally-enhanced kind-of-diff which provides an adequate context.

                                        // Read vertically this column
                                        //   |  |  |  |  |  |  |  |  |
                                        //   v  v  v  v  v  v  v  v  v

{
//SNIP//
if (clone_flags & CLONE_THREAD)
threadgroup_change_begin(current);
//SNIP//

 #ifdef CONFIG_NUMA
    p->mempolicy = mpol_dup(p->mempolicy);
    if (IS_ERR(p->mempolicy)) {
//SNIP//
        goto bad_fork_cleanup_threadgroup_lock;
    }
 #endif
//SNIP//
    retval = sched_fork(clone_flags, p);
    if (retval)
//                                      // mustn't perf_event_free_task()
        goto bad_fork_cleanup_policy;
    retval = perf_event_init_task(p);
    if (retval)
//                                      // mustn't perf_event_free_task()
        goto bad_fork_cleanup_policy;
    retval = audit_alloc(p);
    if (retval)

// // must perf_event_free_task()
// @@ Hence change this way:
- goto bad_fork_cleanup_policy;
+ goto bad_fork_cleanup_perf;
//SNIP//
bad_fork_cleanup_audit:

    audit_free(p);

// // let's clean perf up
// @@ Hence change this way:
-bad_fork_cleanup_policy:
+bad_fork_cleanup_perf:

    perf_event_free_task(p);
//                                      // no (longer) need to clean perf up
//                                      @@ Hence change this way:
+bad_fork_cleanup_policy:
 #ifdef CONFIG_NUMA
    mpol_put(p->mempolicy);

bad_fork_cleanup_threadgroup_lock:
#endif
if (clone_flags & CLONE_THREAD)
threadgroup_change_end(current);
//SNIP//
}

---
 kernel/fork.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

index 0cf9cdb..a91e47d 100644
@@ -1360,7 +1360,7 @@ static struct task_struct *copy_process(unsigned long clone_flags,
 		goto bad_fork_cleanup_policy;
 	retval = audit_alloc(p);
 	if (retval)
-		goto bad_fork_cleanup_policy;
+		goto bad_fork_cleanup_perf;
 	/* copy all the process information */
 	shm_init_task(p);
 	retval = copy_semundo(clone_flags, p);

@@ -1566,8 +1566,9 @@ bad_fork_cleanup_semundo:
 	exit_sem(p);
 bad_fork_cleanup_audit:
 	audit_free(p);
-bad_fork_cleanup_policy:
+bad_fork_cleanup_perf:
 	perf_event_free_task(p);
+bad_fork_cleanup_policy:
 #ifdef CONFIG_NUMA
 	mpol_put(p->mempolicy);
 bad_fork_cleanup_threadgroup_lock:

Regards,
Sylvain "ythier" Hitier

Business is about being busy, not being rich...
Lived 777 days in a Debian package => http://en.wikipedia.org/wiki/Apt,_Vaucluse
There's THE room for ideals in this mechanical place!

Agreed, this is wrong. Good catch.

but, unless I missed something,

                                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

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.

Ah, indeed. It was meant to be a no-op there, but its before we do that memset, so its still the inherited values, and we don't want to clean those up I think.

Agreed

Yep

I prefer the former, as the latter scatters the perf specific bits over more places. Something like so then?

---
Subject: perf: Fix perf bug in fork()

Oleg noticed that a cleanup by Sylvain actually uncovered a bug; by calling perf_event_free_task() when failing sched_fork() we will not yet have done the memset() on ->perf_event_ctxp[] and will therefore try and 'free' the inherited contexts, which are still in use by the parent process. This is bad..

---

index a232b40..4a0dbb2 100644
@@ -8078,8 +8078,10 @@ int perf_event_init_task(struct task_struct *child)
 
 	for_each_task_context_nr(ctxn) {
 		ret = perf_event_init_context(child, ctxn);
-		if (ret)
+		if (ret) {
+			perf_event_free_task(child);
 			return ret;
+		}
 	}
 
 	return 0;

index ad64248..b6cc3f2 100644
@@ -1367,7 +1367,7 @@ static struct task_struct *copy_process(unsigned long clone_flags,
 		goto bad_fork_cleanup_policy;
 	retval = audit_alloc(p);
 	if (retval)
-		goto bad_fork_cleanup_policy;
+		goto bad_fork_cleanup_perf;
 	/* copy all the process information */
 	shm_init_task(p);
 	retval = copy_semundo(clone_flags, p);

@@ -1573,8 +1573,9 @@ bad_fork_cleanup_semundo:
 	exit_sem(p);
 bad_fork_cleanup_audit:
 	audit_free(p);
-bad_fork_cleanup_policy:
+bad_fork_cleanup_perf:
 	perf_event_free_task(p);
+bad_fork_cleanup_policy:
 #ifdef CONFIG_NUMA
 	mpol_put(p->mempolicy);
 bad_fork_cleanup_threadgroup_lock:

Could this fix a couple of fuzzer triggered perf crashes perhaps?

Thanks,

Ingo

It could indeed I suppose.. you never know what paths those fuzzers manage to hit.

The patch isn't in linux-next and didn't cc stable. I think I'll squirt it Linuswards later this week unless someone stops me..

Yes, thanks, I believ this is correct.

Oleg.