linux-kernel - [PATCH] fork.c: copy_process(): fix cleanup WRT perf_event_free

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <20140926210652.GA27199@erable>
Date:	Fri, 26 Sep 2014 21:06:52 +0000
From:	Sylvain 'ythier' Hitier <sylvain.hitier@...il.com>
To:	linux-kernel@...r.kernel.org,
	Andrew Morton <akpm@...ux-foundation.org>,
	Oleg Nesterov <oleg@...hat.com>,
	Ingo Molnar <mingo@...nel.org>,
	Peter Zijlstra <peterz@...radead.org>
Subject: [PATCH] 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//
 }

Signed-off-by: Sylvain "ythier" Hitier <sylvain.hitier@...il.com>
---
 kernel/fork.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/kernel/fork.c b/kernel/fork.c
index 0cf9cdb..a91e47d 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -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!
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/