Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757933Ab2ECRbe (ORCPT ); Thu, 3 May 2012 13:31:34 -0400 Received: from mail-lb0-f174.google.com ([209.85.217.174]:58199 "EHLO mail-lb0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753292Ab2ECRbb (ORCPT ); Thu, 3 May 2012 13:31:31 -0400 Date: Thu, 3 May 2012 21:31:25 +0400 From: Cyrill Gorcunov To: Sasha Levin Cc: khlebnikov@openvz.org, xemul@parallels.com, Dave Jones , "linux-kernel@vger.kernel.org" , Vasiliy Kulikov , Andrew Morton Subject: Re: c/r: broken locking when executing map_files Message-ID: <20120503173125.GF19347@moon> References: <1335979380.10666.6.camel@lappy> <20120502172756.GD2301@moon> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20120502172756.GD2301@moon> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5578 Lines: 126 On Wed, May 02, 2012 at 09:27:56PM +0400, Cyrill Gorcunov wrote: > > > > My theory is that files under map_files shouldn't be executable, > > but since I'm not sure what the usermode code for c/r does exactly, > > I should probably confirm that first. If it's indeed the case, you > > can probably skip the rest of this mail. > > Thanks Sasha, I'll take a look! Sasha, the patch below should fix the lock problem (still I would prefer to obtain confirm on patch from Vasiliy, CC'ed). Cyrill --- From: Cyrill Gorcunov Subject: fs, proc: Fix ABBA deadlock in case of execution attempt of map_files/ entries map_files/ entries are never supposed to be executed, still curious minds might try to run them, which leads to the following deadlock [ 270.895009] ====================================================== [ 270.895054] [ INFO: possible circular locking dependency detected ] [ 270.895100] 3.4.0-rc4-24406-g841e6a6 #121 Not tainted [ 270.895144] ------------------------------------------------------- [ 270.895189] bash/1556 is trying to acquire lock: [ 270.895235] (&sb->s_type->i_mutex_key#8){+.+.+.}, at: [] do_lookup+0x267/0x2b1 [ 270.895612] [ 270.895613] but task is already holding lock: [ 270.895731] (&sig->cred_guard_mutex){+.+.+.}, at: [] prepare_bprm_creds+0x2d/0x69 [ 270.896081] [ 270.896082] which lock already depends on the new lock. [ 270.896083] [ 270.896220] [ 270.896221] the existing dependency chain (in reverse order) is: [ 270.896340] [ 270.896341] -> #1 (&sig->cred_guard_mutex){+.+.+.}: [ 270.896637] [] validate_chain+0x444/0x4f4 [ 270.896734] [] __lock_acquire+0x387/0x3f8 [ 270.896847] [] lock_acquire+0x12b/0x158 [ 270.896950] [] __mutex_lock_common+0x56/0x3a9 [ 270.897056] [] mutex_lock_killable_nested+0x40/0x45 [ 270.897155] [] lock_trace+0x24/0x59 [ 270.897253] [] proc_map_files_lookup+0x5a/0x165 [ 270.897365] [] __lookup_hash+0x52/0x73 [ 270.897463] [] do_lookup+0x276/0x2b1 [ 270.897560] [] walk_component+0x3d/0x114 [ 270.897657] [] do_last+0xfc/0x540 [ 270.897753] [] path_openat+0xd3/0x306 [ 270.897864] [] do_filp_open+0x3d/0x89 [ 270.897967] [] do_sys_open+0x74/0x106 [ 270.898068] [] sys_open+0x21/0x23 [ 270.898164] [] tracesys+0xdd/0xe2 [ 270.898262] [ 270.898263] -> #0 (&sb->s_type->i_mutex_key#8){+.+.+.}: [ 270.898598] [] check_prev_add+0x6a/0x1ef [ 270.898696] [] validate_chain+0x444/0x4f4 [ 270.898810] [] __lock_acquire+0x387/0x3f8 [ 270.898908] [] lock_acquire+0x12b/0x158 [ 270.899005] [] __mutex_lock_common+0x56/0x3a9 [ 270.899103] [] mutex_lock_nested+0x40/0x45 [ 270.899200] [] do_lookup+0x267/0x2b1 [ 270.899309] [] walk_component+0x3d/0x114 [ 270.899408] [] link_path_walk+0x1f9/0x48f [ 270.899505] [] path_openat+0xb6/0x306 [ 270.899602] [] do_filp_open+0x3d/0x89 [ 270.899699] [] open_exec+0x25/0xa0 [ 270.899809] [] do_execve_common+0xea/0x2f9 [ 270.899907] [] do_execve+0x43/0x45 [ 270.900004] [] sys_execve+0x43/0x5a [ 270.900102] [] stub_execve+0x6c/0xc0 This is because prepare_bprm_creds grabs task->signal->cred_guard_mutex and when do_lookup happens we try to grab task->signal->cred_guard_mutex again in lock_trace. Fix it using plain ptrace_may_access() helper, proc_map_files_lookup is guarded by CAP_SYS_ADMIN. Reported-by: Sasha Levin Cc: Konstantin Khlebnikov Cc: Pavel Emelyanov Cc: Dave Jones Cc: Vasiliy Kulikov Signed-off-by: Cyrill Gorcunov --- fs/proc/base.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) Index: linux-2.6.git/fs/proc/base.c =================================================================== --- linux-2.6.git.orig/fs/proc/base.c +++ linux-2.6.git/fs/proc/base.c @@ -2226,16 +2226,16 @@ static struct dentry *proc_map_files_loo goto out; result = ERR_PTR(-EACCES); - if (lock_trace(task)) + if (!ptrace_may_access(task, PTRACE_MODE_READ)) goto out_put_task; result = ERR_PTR(-ENOENT); if (dname_to_vma_addr(dentry, &vm_start, &vm_end)) - goto out_unlock; + goto out_put_task; mm = get_task_mm(task); if (!mm) - goto out_unlock; + goto out_put_task; down_read(&mm->mmap_sem); vma = find_exact_vma(mm, vm_start, vm_end); @@ -2247,8 +2247,6 @@ static struct dentry *proc_map_files_loo out_no_vma: up_read(&mm->mmap_sem); mmput(mm); -out_unlock: - unlock_trace(task); out_put_task: put_task_struct(task); out: -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/