From patchwork Thu Sep 27 07:15:11 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: John Johansen X-Patchwork-Id: 187294 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from chlorine.canonical.com (chlorine.canonical.com [91.189.94.204]) by ozlabs.org (Postfix) with ESMTP id CB0132C00C1 for ; Thu, 27 Sep 2012 17:15:42 +1000 (EST) Received: from localhost ([127.0.0.1] helo=chlorine.canonical.com) by chlorine.canonical.com with esmtp (Exim 4.71) (envelope-from ) id 1TH8H9-00043d-Ni; Thu, 27 Sep 2012 07:12:47 +0000 Received: from youngberry.canonical.com ([91.189.89.112]) by chlorine.canonical.com with esmtp (Exim 4.71) (envelope-from ) id 1TH8H8-00043Y-CK for kernel-team@lists.ubuntu.com; Thu, 27 Sep 2012 07:12:46 +0000 Received: from static-50-53-34-211.bvtn.or.frontiernet.net ([50.53.34.211] helo=[192.168.192.110]) by youngberry.canonical.com with esmtpsa (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1TH8Jf-0003mP-PP for kernel-team@lists.ubuntu.com; Thu, 27 Sep 2012 07:15:25 +0000 Message-ID: <5063FCFF.1000505@canonical.com> Date: Thu, 27 Sep 2012 00:15:11 -0700 From: John Johansen Organization: Canonical User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:15.0) Gecko/20120911 Thunderbird/15.0.1 MIME-Version: 1.0 To: Kernel team list Subject: [PATCH][Quantal] UBUNTU SAUCE: apparmor: fix IRQ stack overflow X-Enigmail-Version: 1.4.4 X-BeenThere: kernel-team@lists.ubuntu.com X-Mailman-Version: 2.1.13 Precedence: list List-Id: Kernel team discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: kernel-team-bounces@lists.ubuntu.com Errors-To: kernel-team-bounces@lists.ubuntu.com This is a minimal patch to fix bug#1056078. The patch in the works for upstream is larger and reworks the entire replacedby logic to prevent the replacedby chains from happening, which will improve resource usage, but it is not available yet. This can be reverted when it is. BugLink: http://bugs.launchpad.net/bugs/1056078 Profile replacement can cause long chains of profiles to build up that get freed in a cascading chain of free_profile calls. Because free_profile is being called via aa_put_profile (and hence kref_put) each profile free_profile is nested (looks like recursion of free_profile). That is free_profile indirectly calls free_profile on the next profile in the chain via aa_put_profile. Break this nesting by directly walking the chain, and as long as a profile is being freed because it has no more references continue on to the next profile. This results in at most 2 levels of free_profile being called. Signed-off-by: John Johansen --- security/apparmor/policy.c | 28 +++++++++++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-) diff --git a/security/apparmor/policy.c b/security/apparmor/policy.c index 27c8161..3f01fb7 100644 --- a/security/apparmor/policy.c +++ b/security/apparmor/policy.c @@ -724,6 +724,8 @@ fail: */ static void free_profile(struct aa_profile *profile) { + struct aa_profile *p; + AA_DEBUG("%s(%p)\n", __func__, profile); if (!profile) @@ -752,7 +754,31 @@ static void free_profile(struct aa_profile *profile) aa_put_dfa(profile->xmatch); aa_put_dfa(profile->policy.dfa); - aa_put_profile(profile->replacedby); + /* put the profile reference for replacedby, but not via + * put_profile(kref_put). + * replacedby can form a long chain that can result in cascading + * frees that blows the stack because kref_put makes a nested fn + * call (it looks like recursion, with free_profile calling + * free_profile) for each profile in the chain lp#1056078. The + * long chain creation should be addressed by changes to profile + * replacement. + * This just addresses recursion of free_profile causing the + * stack to blow, when a chain is encountered. + */ + for (p = profile->replacedby; p; ) { + if (atomic_dec_and_test(&p->base.count.refcount)) { + /* no more refs on p, grab its replacedby */ + struct aa_profile *next = p->replacedby; + /* break the chain */ + p->replacedby = NULL; + /* now free p, chain is broken */ + free_profile(p); + + /* follow up with next profile in the chain */ + p = next; + } else + break; + } kzfree(profile); }