Hi,
So, after taking a closer look at this, I cannot understand how it's
possible. Yama's task_lock call is against "current", not "child",
which is what ptrace_may_access() is locking. And the same code makes
sure that current != child. Yama would never get called if current ==
child.
How did you reproduce this situation?
Thanks,
-Kees
On Thu, Jul 26, 2012 at 6:47 AM, Fengguang Wu <[email protected]> wrote:
> Hi Kees,
>
> Here is a recursive lock possibility:
>
> ptrace_may_access()
> => task_lock(task);
> yama_ptrace_access_check()
> get_task_comm()
> => task_lock(task);
>
> [ 60.230444] =============================================
> [ 60.232078] [ INFO: possible recursive locking detected ]
> [ 60.232078] 3.5.0+ #281 Not tainted
> [ 60.232078] ---------------------------------------------
> [ 60.232078] trinity-child0/17019 is trying to acquire lock:
> [ 60.232078] (&(&p->alloc_lock)->rlock){+.+...}, at: [<c1176ffa>] get_task_comm+0x4a/0xf0
> [ 60.232078]
> [ 60.232078] but task is already holding lock:
> [ 60.232078] (&(&p->alloc_lock)->rlock){+.+...}, at: [<c10653fa>] ptrace_may_access+0x4a/0xf0
> [ 60.232078]
> [ 60.232078] other info that might help us debug this:
> [ 60.232078] Possible unsafe locking scenario:
> [ 60.232078]
> [ 60.232078] CPU0
> [ 60.232078] ----
> [ 60.232078] lock(&(&p->alloc_lock)->rlock);
> [ 60.232078] lock(&(&p->alloc_lock)->rlock);
> [ 60.232078]
> [ 60.232078] *** DEADLOCK ***
> [ 60.232078]
> [ 60.232078] May be due to missing lock nesting notation
> [ 60.232078]
> [ 60.232078] 3 locks held by trinity-child0/17019:
> [ 60.232078] #0: (&p->lock){+.+.+.}, at: [<c11a9683>] seq_read+0x33/0x6b0
> [ 60.232078] #1: (&sig->cred_guard_mutex){+.+.+.}, at: [<c11ff8ae>] lock_trace+0x2e/0xb0
> [ 60.232078] #2: (&(&p->alloc_lock)->rlock){+.+...}, at: [<c10653fa>] ptrace_may_access+0x4a/0xf0
> [ 60.232078]
> [ 60.232078] stack backtrace:
> [ 60.232078] Pid: 17019, comm: trinity-child0 Not tainted 3.5.0+ #281
> [ 60.232078] Call Trace:
> [ 60.232078] [<c10c6238>] __lock_acquire+0x1498/0x14f0
> [ 60.232078] [<c10be7e7>] ? trace_hardirqs_off+0x27/0x40
> [ 60.232078] [<c10c6360>] lock_acquire+0xd0/0x110
> [ 60.232078] [<c1176ffa>] ? get_task_comm+0x4a/0xf0
> [ 60.232078] [<c1422290>] _raw_spin_lock+0x60/0x110
> [ 60.232078] [<c1176ffa>] ? get_task_comm+0x4a/0xf0
> [ 60.232078] [<c1176ffa>] get_task_comm+0x4a/0xf0
> [ 60.232078] [<c1246798>] yama_ptrace_access_check+0x468/0x4a0
> [ 60.232078] [<c124648f>] ? yama_ptrace_access_check+0x15f/0x4a0
> [ 60.232078] [<c124209a>] security_ptrace_access_check+0x1a/0x30
> [ 60.232078] [<c1065229>] __ptrace_may_access+0x189/0x310
> [ 60.232078] [<c10650cc>] ? __ptrace_may_access+0x2c/0x310
> [ 60.232078] [<c106542d>] ptrace_may_access+0x7d/0xf0
> [ 60.232078] [<c11ff8ea>] lock_trace+0x6a/0xb0
> [ 60.232078] [<c11ffa46>] proc_pid_stack+0x76/0x170
> [ 60.232078] [<c1201064>] proc_single_show+0x74/0x100
> [ 60.232078] [<c11a97b3>] seq_read+0x163/0x6b0
> [ 60.232078] [<c105bf70>] ? do_setitimer+0x220/0x330
> [ 60.232078] [<c11a9650>] ? seq_lseek+0x1f0/0x1f0
> [ 60.232078] [<c116b55a>] vfs_read+0xca/0x280
> [ 60.232078] [<c11a9650>] ? seq_lseek+0x1f0/0x1f0
> [ 60.232078] [<c116b776>] sys_read+0x66/0xe0
> [ 60.232078] [<c1423d9d>] syscall_call+0x7/0xb
> [ 60.232078] [<c1420000>] ? __schedule+0x2a0/0xc80
>
> Thanks,
> Fengguang
--
Kees Cook
Chrome OS Security
On Thu, Aug 09, 2012 at 06:39:34PM -0700, Kees Cook wrote:
> Hi,
>
> So, after taking a closer look at this, I cannot understand how it's
> possible. Yama's task_lock call is against "current", not "child",
> which is what ptrace_may_access() is locking. And the same code makes
> sure that current != child. Yama would never get called if current ==
> child.
>
> How did you reproduce this situation?
This warning can be triggered with Dave Jones' trinity tool:
git://git.codemonkey.org.uk/trinity
That's a very dangerous tool, please only run it as normal user in a
backed up and chrooted test box. I personally run it inside an initrd.
If you are interested in reproducing this, I can send you the ready
made initrd in private email.
Thanks,
Fengguang
> On Thu, Jul 26, 2012 at 6:47 AM, Fengguang Wu <[email protected]> wrote:
> > Hi Kees,
> >
> > Here is a recursive lock possibility:
> >
> > ptrace_may_access()
> > => task_lock(task);
> > yama_ptrace_access_check()
> > get_task_comm()
> > => task_lock(task);
> >
> > [ 60.230444] =============================================
> > [ 60.232078] [ INFO: possible recursive locking detected ]
> > [ 60.232078] 3.5.0+ #281 Not tainted
> > [ 60.232078] ---------------------------------------------
> > [ 60.232078] trinity-child0/17019 is trying to acquire lock:
> > [ 60.232078] (&(&p->alloc_lock)->rlock){+.+...}, at: [<c1176ffa>] get_task_comm+0x4a/0xf0
> > [ 60.232078]
> > [ 60.232078] but task is already holding lock:
> > [ 60.232078] (&(&p->alloc_lock)->rlock){+.+...}, at: [<c10653fa>] ptrace_may_access+0x4a/0xf0
> > [ 60.232078]
> > [ 60.232078] other info that might help us debug this:
> > [ 60.232078] Possible unsafe locking scenario:
> > [ 60.232078]
> > [ 60.232078] CPU0
> > [ 60.232078] ----
> > [ 60.232078] lock(&(&p->alloc_lock)->rlock);
> > [ 60.232078] lock(&(&p->alloc_lock)->rlock);
> > [ 60.232078]
> > [ 60.232078] *** DEADLOCK ***
> > [ 60.232078]
> > [ 60.232078] May be due to missing lock nesting notation
> > [ 60.232078]
> > [ 60.232078] 3 locks held by trinity-child0/17019:
> > [ 60.232078] #0: (&p->lock){+.+.+.}, at: [<c11a9683>] seq_read+0x33/0x6b0
> > [ 60.232078] #1: (&sig->cred_guard_mutex){+.+.+.}, at: [<c11ff8ae>] lock_trace+0x2e/0xb0
> > [ 60.232078] #2: (&(&p->alloc_lock)->rlock){+.+...}, at: [<c10653fa>] ptrace_may_access+0x4a/0xf0
> > [ 60.232078]
> > [ 60.232078] stack backtrace:
> > [ 60.232078] Pid: 17019, comm: trinity-child0 Not tainted 3.5.0+ #281
> > [ 60.232078] Call Trace:
> > [ 60.232078] [<c10c6238>] __lock_acquire+0x1498/0x14f0
> > [ 60.232078] [<c10be7e7>] ? trace_hardirqs_off+0x27/0x40
> > [ 60.232078] [<c10c6360>] lock_acquire+0xd0/0x110
> > [ 60.232078] [<c1176ffa>] ? get_task_comm+0x4a/0xf0
> > [ 60.232078] [<c1422290>] _raw_spin_lock+0x60/0x110
> > [ 60.232078] [<c1176ffa>] ? get_task_comm+0x4a/0xf0
> > [ 60.232078] [<c1176ffa>] get_task_comm+0x4a/0xf0
> > [ 60.232078] [<c1246798>] yama_ptrace_access_check+0x468/0x4a0
> > [ 60.232078] [<c124648f>] ? yama_ptrace_access_check+0x15f/0x4a0
> > [ 60.232078] [<c124209a>] security_ptrace_access_check+0x1a/0x30
> > [ 60.232078] [<c1065229>] __ptrace_may_access+0x189/0x310
> > [ 60.232078] [<c10650cc>] ? __ptrace_may_access+0x2c/0x310
> > [ 60.232078] [<c106542d>] ptrace_may_access+0x7d/0xf0
> > [ 60.232078] [<c11ff8ea>] lock_trace+0x6a/0xb0
> > [ 60.232078] [<c11ffa46>] proc_pid_stack+0x76/0x170
> > [ 60.232078] [<c1201064>] proc_single_show+0x74/0x100
> > [ 60.232078] [<c11a97b3>] seq_read+0x163/0x6b0
> > [ 60.232078] [<c105bf70>] ? do_setitimer+0x220/0x330
> > [ 60.232078] [<c11a9650>] ? seq_lseek+0x1f0/0x1f0
> > [ 60.232078] [<c116b55a>] vfs_read+0xca/0x280
> > [ 60.232078] [<c11a9650>] ? seq_lseek+0x1f0/0x1f0
> > [ 60.232078] [<c116b776>] sys_read+0x66/0xe0
> > [ 60.232078] [<c1423d9d>] syscall_call+0x7/0xb
> > [ 60.232078] [<c1420000>] ? __schedule+0x2a0/0xc80
> >
> > Thanks,
> > Fengguang
>
>
>
> --
> Kees Cook
> Chrome OS Security
On Thu, Aug 9, 2012 at 6:52 PM, Fengguang Wu <[email protected]> wrote:
> On Thu, Aug 09, 2012 at 06:39:34PM -0700, Kees Cook wrote:
>> Hi,
>>
>> So, after taking a closer look at this, I cannot understand how it's
>> possible. Yama's task_lock call is against "current", not "child",
>> which is what ptrace_may_access() is locking. And the same code makes
>> sure that current != child. Yama would never get called if current ==
>> child.
>>
>> How did you reproduce this situation?
>
> This warning can be triggered with Dave Jones' trinity tool:
>
> git://git.codemonkey.org.uk/trinity
>
> That's a very dangerous tool, please only run it as normal user in a
> backed up and chrooted test box. I personally run it inside an initrd.
> If you are interested in reproducing this, I can send you the ready
> made initrd in private email.
Well, even with your initrd, I can't reproduce this. You're running
this against a stock kernel? I can't see how the path you've shown can
possible happen. It could only happen if "task" was "current", but
there is an explicit test for that in ptrace_may_access(). Based on
the traceback, this is from reading /proc/$pid/stack (or
/proc/$pid/task/$tid/stack), rather than a direct ptrace() call, but
the code path for task != current still stands.
I've tried both normal and "trinity -c read" and I haven't seen the
trace you found. :(
If you can isolate the case further, I'm happy to fix it, but
currently, I don't see a path where this can deadlock.
Thanks,
-Kees
>> On Thu, Jul 26, 2012 at 6:47 AM, Fengguang Wu <[email protected]> wrote:
>> > Here is a recursive lock possibility:
>> >
>> > ptrace_may_access()
>> > => task_lock(task);
>> > yama_ptrace_access_check()
>> > get_task_comm()
>> > => task_lock(task);
>> >
>> > [ 60.230444] =============================================
>> > [ 60.232078] [ INFO: possible recursive locking detected ]
>> > [ 60.232078] 3.5.0+ #281 Not tainted
>> > [ 60.232078] ---------------------------------------------
>> > [ 60.232078] trinity-child0/17019 is trying to acquire lock:
>> > [ 60.232078] (&(&p->alloc_lock)->rlock){+.+...}, at: [<c1176ffa>] get_task_comm+0x4a/0xf0
>> > [ 60.232078]
>> > [ 60.232078] but task is already holding lock:
>> > [ 60.232078] (&(&p->alloc_lock)->rlock){+.+...}, at: [<c10653fa>] ptrace_may_access+0x4a/0xf0
>> > [ 60.232078]
>> > [ 60.232078] other info that might help us debug this:
>> > [ 60.232078] Possible unsafe locking scenario:
>> > [ 60.232078]
>> > [ 60.232078] CPU0
>> > [ 60.232078] ----
>> > [ 60.232078] lock(&(&p->alloc_lock)->rlock);
>> > [ 60.232078] lock(&(&p->alloc_lock)->rlock);
>> > [ 60.232078]
>> > [ 60.232078] *** DEADLOCK ***
>> > [ 60.232078]
>> > [ 60.232078] May be due to missing lock nesting notation
>> > [ 60.232078]
>> > [ 60.232078] 3 locks held by trinity-child0/17019:
>> > [ 60.232078] #0: (&p->lock){+.+.+.}, at: [<c11a9683>] seq_read+0x33/0x6b0
>> > [ 60.232078] #1: (&sig->cred_guard_mutex){+.+.+.}, at: [<c11ff8ae>] lock_trace+0x2e/0xb0
>> > [ 60.232078] #2: (&(&p->alloc_lock)->rlock){+.+...}, at: [<c10653fa>] ptrace_may_access+0x4a/0xf0
>> > [ 60.232078]
>> > [ 60.232078] stack backtrace:
>> > [ 60.232078] Pid: 17019, comm: trinity-child0 Not tainted 3.5.0+ #281
>> > [ 60.232078] Call Trace:
>> > [ 60.232078] [<c10c6238>] __lock_acquire+0x1498/0x14f0
>> > [ 60.232078] [<c10be7e7>] ? trace_hardirqs_off+0x27/0x40
>> > [ 60.232078] [<c10c6360>] lock_acquire+0xd0/0x110
>> > [ 60.232078] [<c1176ffa>] ? get_task_comm+0x4a/0xf0
>> > [ 60.232078] [<c1422290>] _raw_spin_lock+0x60/0x110
>> > [ 60.232078] [<c1176ffa>] ? get_task_comm+0x4a/0xf0
>> > [ 60.232078] [<c1176ffa>] get_task_comm+0x4a/0xf0
>> > [ 60.232078] [<c1246798>] yama_ptrace_access_check+0x468/0x4a0
>> > [ 60.232078] [<c124648f>] ? yama_ptrace_access_check+0x15f/0x4a0
>> > [ 60.232078] [<c124209a>] security_ptrace_access_check+0x1a/0x30
>> > [ 60.232078] [<c1065229>] __ptrace_may_access+0x189/0x310
>> > [ 60.232078] [<c10650cc>] ? __ptrace_may_access+0x2c/0x310
>> > [ 60.232078] [<c106542d>] ptrace_may_access+0x7d/0xf0
>> > [ 60.232078] [<c11ff8ea>] lock_trace+0x6a/0xb0
>> > [ 60.232078] [<c11ffa46>] proc_pid_stack+0x76/0x170
>> > [ 60.232078] [<c1201064>] proc_single_show+0x74/0x100
>> > [ 60.232078] [<c11a97b3>] seq_read+0x163/0x6b0
>> > [ 60.232078] [<c105bf70>] ? do_setitimer+0x220/0x330
>> > [ 60.232078] [<c11a9650>] ? seq_lseek+0x1f0/0x1f0
>> > [ 60.232078] [<c116b55a>] vfs_read+0xca/0x280
>> > [ 60.232078] [<c11a9650>] ? seq_lseek+0x1f0/0x1f0
>> > [ 60.232078] [<c116b776>] sys_read+0x66/0xe0
>> > [ 60.232078] [<c1423d9d>] syscall_call+0x7/0xb
>> > [ 60.232078] [<c1420000>] ? __schedule+0x2a0/0xc80
--
Kees Cook
Chrome OS Security
On Tue, Aug 14, 2012 at 02:16:52PM -0700, Kees Cook wrote:
> On Thu, Aug 9, 2012 at 6:52 PM, Fengguang Wu <[email protected]> wrote:
> > On Thu, Aug 09, 2012 at 06:39:34PM -0700, Kees Cook wrote:
> >> Hi,
> >>
> >> So, after taking a closer look at this, I cannot understand how it's
> >> possible. Yama's task_lock call is against "current", not "child",
> >> which is what ptrace_may_access() is locking. And the same code makes
> >> sure that current != child. Yama would never get called if current ==
> >> child.
> >>
> >> How did you reproduce this situation?
> >
> > This warning can be triggered with Dave Jones' trinity tool:
> >
> > git://git.codemonkey.org.uk/trinity
> >
> > That's a very dangerous tool, please only run it as normal user in a
> > backed up and chrooted test box. I personally run it inside an initrd.
> > If you are interested in reproducing this, I can send you the ready
> > made initrd in private email.
>
> Well, even with your initrd, I can't reproduce this. You're running
> this against a stock kernel? I can't see how the path you've shown can
Yes, it happens on 3.6-rc1.
> possible happen. It could only happen if "task" was "current", but
> there is an explicit test for that in ptrace_may_access(). Based on
> the traceback, this is from reading /proc/$pid/stack (or
> /proc/$pid/task/$tid/stack), rather than a direct ptrace() call, but
> the code path for task != current still stands.
>
> I've tried both normal and "trinity -c read" and I haven't seen the
> trace you found. :(
>
> If you can isolate the case further, I'm happy to fix it, but
> currently, I don't see a path where this can deadlock.
Even if it's proved to be a false warning, it's still very worthwhile
to apply Oleg's fix to quiet the warning. Such warnings will mislead
my bisect script. The sooner it's fixed, the better. And I like Oleg's
fix because it makes things more simple and a little bit faster.
btw, I see some different warnings when digging through the boot logs:
(x86_64-randconfig-b050)
[ 128.725667]
[ 128.728649] =============================================
[ 128.733989] [ INFO: possible recursive locking detected ]
[ 128.733989] 3.6.0-rc1 #1 Not tainted
[ 128.733989] ---------------------------------------------
[ 128.733989] trinity-child0/523 is trying to acquire lock:
[ 128.733989] (&(&p->alloc_lock)->rlock){+.+...}, at: [<ffffffff810e0481>] get_task_comm+0x20/0x47
[ 128.733989]
[ 128.733989] but task is already holding lock:
[ 128.733989] (&(&p->alloc_lock)->rlock){+.+...}, at: [<ffffffff810572ab>] sys_ptrace+0x158/0x313
[ 128.733989]
[ 128.733989] other info that might help us debug this:
[ 128.733989] Possible unsafe locking scenario:
[ 128.733989]
[ 128.733989] CPU0
[ 128.733989] ----
[ 128.733989] lock(&(&p->alloc_lock)->rlock);
[ 128.733989] lock(&(&p->alloc_lock)->rlock);
[ 128.733989]
[ 128.733989] *** DEADLOCK ***
[ 128.733989]
[ 128.733989] May be due to missing lock nesting notation
[ 128.733989]
[ 128.733989] 2 locks held by trinity-child0/523:
[ 128.733989] #0: (&sig->cred_guard_mutex){+.+.+.}, at: [<ffffffff81057290>] sys_ptrace+0x13d/0x313
[ 128.733989] #1: (&(&p->alloc_lock)->rlock){+.+...}, at: [<ffffffff810572ab>] sys_ptrace+0x158/0x313
[ 128.733989]
[ 128.733989] stack backtrace:
[ 128.733989] Pid: 523, comm: trinity-child0 Not tainted 3.6.0-rc1 #1
[ 128.733989] Call Trace:
[ 128.733989] [<ffffffff81085649>] __lock_acquire+0xbe0/0xcfb
[ 128.733989] [<ffffffff81084884>] ? mark_lock+0x2d/0x212
[ 128.733989] [<ffffffff81084884>] ? mark_lock+0x2d/0x212
[ 128.733989] [<ffffffff8108639d>] lock_acquire+0x82/0x9d
[ 128.733989] [<ffffffff810e0481>] ? get_task_comm+0x20/0x47
[ 128.733989] [<ffffffff81a35ddf>] _raw_spin_lock+0x3b/0x4a
[ 128.733989] [<ffffffff810e0481>] ? get_task_comm+0x20/0x47
[ 128.733989] [<ffffffff810e0481>] get_task_comm+0x20/0x47
[ 128.733989] [<ffffffff81392c01>] yama_ptrace_access_check+0x16a/0x1c7
[ 128.733989] [<ffffffff810864e3>] ? lock_release+0x12b/0x157
[ 128.733989] [<ffffffff81390bfb>] security_ptrace_access_check+0xe/0x10
[ 128.733989] [<ffffffff81056e2b>] __ptrace_may_access+0x109/0x11b
[ 128.733989] [<ffffffff810572b8>] sys_ptrace+0x165/0x313
[ 128.733989] [<ffffffff81a37079>] system_call_fastpath+0x16/0x1b
[ 128.823670] ptrace of pid 522 was attempted by: trinity-child0 (pid 523)
(x86_64-randconfig-k056)
[ 87.057392]
[ 87.058009] =============================================
[ 87.058009] [ INFO: possible recursive locking detected ]
[ 87.058009] 3.6.0-rc1-00011-gf8cdda8 #2 Not tainted
[ 87.058009] ---------------------------------------------
[ 87.058009] trinity-child0/328 is trying to acquire lock:
[ 87.058009] (&(&p->alloc_lock)->rlock){+.+...}, at: [<ffffffff81104632>] spin_lock+0x9/0xb
[ 87.058009]
[ 87.058009] but task is already holding lock:
[ 87.058009] (&(&p->alloc_lock)->rlock){+.+...}, at: [<ffffffff8106cb40>] ptrace_attach+0xa4/0x208
[ 87.058009]
[ 87.058009] other info that might help us debug this:
[ 87.058009] Possible unsafe locking scenario:
[ 87.058009]
[ 87.058009] CPU0
[ 87.058009] ----
[ 87.058009] lock(&(&p->alloc_lock)->rlock);
[ 87.058009] lock(&(&p->alloc_lock)->rlock);
[ 87.058009]
[ 87.058009] *** DEADLOCK ***
[ 87.058009]
[ 87.058009] May be due to missing lock nesting notation
[ 87.058009]
[ 87.058009] 2 locks held by trinity-child0/328:
[ 87.058009] #0: (&sig->cred_guard_mutex){+.+.+.}, at: [<ffffffff8106cb25>] ptrace_attach+0x89/0x208
[ 87.058009] #1: (&(&p->alloc_lock)->rlock){+.+...}, at: [<ffffffff8106cb40>] ptrace_attach+0xa4/0x208
[ 87.058009]
[ 87.058009] stack backtrace:
[ 87.058009] Pid: 328, comm: trinity-child0 Not tainted 3.6.0-rc1-00011-gf8cdda8 #2
[ 87.058009] Call Trace:
[ 87.058009] [<ffffffff810a104e>] __lock_acquire+0xbe0/0xcfb
[ 87.058009] [<ffffffff810a07d3>] ? __lock_acquire+0x365/0xcfb
[ 87.058009] [<ffffffff810a0289>] ? mark_lock+0x2d/0x212
[ 87.058009] [<ffffffff810a0289>] ? mark_lock+0x2d/0x212
[ 87.058009] [<ffffffff810a1da2>] lock_acquire+0x82/0x9d
[ 87.058009] [<ffffffff81104632>] ? spin_lock+0x9/0xb
[ 87.058009] [<ffffffff816848af>] _raw_spin_lock+0x3b/0x4a
[ 87.058009] [<ffffffff81104632>] ? spin_lock+0x9/0xb
[ 87.058009] [<ffffffff81684a42>] ? _raw_spin_unlock_irqrestore+0x48/0x5c
[ 87.058009] [<ffffffff81104632>] spin_lock+0x9/0xb
[ 87.058009] [<ffffffff811058f3>] get_task_comm+0x20/0x47
[ 87.058009] [<ffffffff81239447>] yama_ptrace_access_check+0x15b/0x1a4
[ 87.058009] [<ffffffff812379fb>] security_ptrace_access_check+0xe/0x10
[ 87.058009] [<ffffffff8106ca8a>] __ptrace_may_access+0x110/0x122
[ 87.058009] [<ffffffff8106cb4d>] ptrace_attach+0xb1/0x208
[ 87.058009] [<ffffffff8106cfd0>] sys_ptrace+0x5c/0xb9
[ 87.058009] [<ffffffff81685b79>] system_call_fastpath+0x16/0x1b
[ 87.122562] ptrace of pid 326 was attempted by: trinity-child0 (pid 328)
[ 90.259448] ptrace of pid 332 was attempted by: trinity-child0 (pid 335)
Thanks,
Fengguang
On Tue, Aug 14, 2012 at 8:01 PM, Fengguang Wu <[email protected]> wrote:
> On Tue, Aug 14, 2012 at 02:16:52PM -0700, Kees Cook wrote:
>> On Thu, Aug 9, 2012 at 6:52 PM, Fengguang Wu <[email protected]> wrote:
>> > On Thu, Aug 09, 2012 at 06:39:34PM -0700, Kees Cook wrote:
>> >> Hi,
>> >>
>> >> So, after taking a closer look at this, I cannot understand how it's
>> >> possible. Yama's task_lock call is against "current", not "child",
>> >> which is what ptrace_may_access() is locking. And the same code makes
>> >> sure that current != child. Yama would never get called if current ==
>> >> child.
>> >>
>> >> How did you reproduce this situation?
>> >
>> > This warning can be triggered with Dave Jones' trinity tool:
>> >
>> > git://git.codemonkey.org.uk/trinity
>> >
>> > That's a very dangerous tool, please only run it as normal user in a
>> > backed up and chrooted test box. I personally run it inside an initrd.
>> > If you are interested in reproducing this, I can send you the ready
>> > made initrd in private email.
>>
>> Well, even with your initrd, I can't reproduce this. You're running
>> this against a stock kernel? I can't see how the path you've shown can
>
> Yes, it happens on 3.6-rc1.
>
>> possible happen. It could only happen if "task" was "current", but
>> there is an explicit test for that in ptrace_may_access(). Based on
>> the traceback, this is from reading /proc/$pid/stack (or
>> /proc/$pid/task/$tid/stack), rather than a direct ptrace() call, but
>> the code path for task != current still stands.
>>
>> I've tried both normal and "trinity -c read" and I haven't seen the
>> trace you found. :(
>>
>> If you can isolate the case further, I'm happy to fix it, but
>> currently, I don't see a path where this can deadlock.
>
> Even if it's proved to be a false warning, it's still very worthwhile
> to apply Oleg's fix to quiet the warning. Such warnings will mislead
> my bisect script. The sooner it's fixed, the better. And I like Oleg's
> fix because it makes things more simple and a little bit faster.
>
> btw, I see some different warnings when digging through the boot logs:
>
> (x86_64-randconfig-b050)
> [ 128.725667]
> [ 128.728649] =============================================
> [ 128.733989] [ INFO: possible recursive locking detected ]
> [ 128.733989] 3.6.0-rc1 #1 Not tainted
> [ 128.733989] ---------------------------------------------
> [ 128.733989] trinity-child0/523 is trying to acquire lock:
> [ 128.733989] (&(&p->alloc_lock)->rlock){+.+...}, at: [<ffffffff810e0481>] get_task_comm+0x20/0x47
> [ 128.733989]
> [ 128.733989] but task is already holding lock:
> [ 128.733989] (&(&p->alloc_lock)->rlock){+.+...}, at: [<ffffffff810572ab>] sys_ptrace+0x158/0x313
> [ 128.733989]
> [ 128.733989] other info that might help us debug this:
> [ 128.733989] Possible unsafe locking scenario:
> [ 128.733989]
> [ 128.733989] CPU0
> [ 128.733989] ----
> [ 128.733989] lock(&(&p->alloc_lock)->rlock);
> [ 128.733989] lock(&(&p->alloc_lock)->rlock);
> [ 128.733989]
> [ 128.733989] *** DEADLOCK ***
> [ 128.733989]
> [ 128.733989] May be due to missing lock nesting notation
> [ 128.733989]
> [ 128.733989] 2 locks held by trinity-child0/523:
> [ 128.733989] #0: (&sig->cred_guard_mutex){+.+.+.}, at: [<ffffffff81057290>] sys_ptrace+0x13d/0x313
> [ 128.733989] #1: (&(&p->alloc_lock)->rlock){+.+...}, at: [<ffffffff810572ab>] sys_ptrace+0x158/0x313
> [ 128.733989]
> [ 128.733989] stack backtrace:
> [ 128.733989] Pid: 523, comm: trinity-child0 Not tainted 3.6.0-rc1 #1
> [ 128.733989] Call Trace:
> [ 128.733989] [<ffffffff81085649>] __lock_acquire+0xbe0/0xcfb
> [ 128.733989] [<ffffffff81084884>] ? mark_lock+0x2d/0x212
> [ 128.733989] [<ffffffff81084884>] ? mark_lock+0x2d/0x212
> [ 128.733989] [<ffffffff8108639d>] lock_acquire+0x82/0x9d
> [ 128.733989] [<ffffffff810e0481>] ? get_task_comm+0x20/0x47
> [ 128.733989] [<ffffffff81a35ddf>] _raw_spin_lock+0x3b/0x4a
> [ 128.733989] [<ffffffff810e0481>] ? get_task_comm+0x20/0x47
> [ 128.733989] [<ffffffff810e0481>] get_task_comm+0x20/0x47
> [ 128.733989] [<ffffffff81392c01>] yama_ptrace_access_check+0x16a/0x1c7
> [ 128.733989] [<ffffffff810864e3>] ? lock_release+0x12b/0x157
> [ 128.733989] [<ffffffff81390bfb>] security_ptrace_access_check+0xe/0x10
> [ 128.733989] [<ffffffff81056e2b>] __ptrace_may_access+0x109/0x11b
> [ 128.733989] [<ffffffff810572b8>] sys_ptrace+0x165/0x313
> [ 128.733989] [<ffffffff81a37079>] system_call_fastpath+0x16/0x1b
> [ 128.823670] ptrace of pid 522 was attempted by: trinity-child0 (pid 523)
Okay, I've now managed to reproduce this locally. I added a bunch of
debugging, and I think I understand what's going on. This warning is,
actually, a false positive. It is correct in that the _class_ of locks
get used recursively (the task_struct->alloc_lock), but they are
separate instantiations ("task" is never "current").
So Oleg's suggestion of removing the locking around the reading of
->comm is wrong since it really does need the lock. I've read the bit
on declaring nested locking, but it doesn't seem to apply here. I have
no idea what the correct solution for this is since the code already
verifies that the same task_struct instance will never be the locked
twice. How can I teach the lockdep checker about this?
-Kees
--
Kees Cook
Chrome OS Security
On Tue, 2012-08-14 at 22:56 -0700, Kees Cook wrote:
> So Oleg's suggestion of removing the locking around the reading of
> ->comm is wrong since it really does need the lock.
There's tons of code reading comm without locking.. you're saying that
all is broken?
On 08/14, Kees Cook wrote:
>
> Okay, I've now managed to reproduce this locally. I added a bunch of
> debugging, and I think I understand what's going on. This warning is,
> actually, a false positive.
Sure. I mean that yes, this warning doesn't mean we already hit deadlock.
> get used recursively (the task_struct->alloc_lock), but they are
> separate instantiations ("task" is never "current").
Yes. But suppose that we have 2 tasks T1 and T2,
- T1 does ptrace(PTRACE_ATTACH, T2);
- T2 does ptrace(PTRACE_ATTACH, T1);
at the same time. This can lead to the "real" deadlock, no?
> So Oleg's suggestion of removing the locking around the reading of
> ->comm is wrong since it really does need the lock.
Nothing bad can happen without the lock. Yes, printk() can print
some string "in between" if we race with set_task_comm() but this
is all.
BTW, set_task_comm()->wmb() and memset() should die. There are
not needed afaics, and the comment is misleading.
Oleg.
On Wed, 2012-08-15 at 15:01 +0200, Oleg Nesterov wrote:
> BTW, set_task_comm()->wmb() and memset() should die. There are
> not needed afaics, and the comment is misleading.
As long as we guarantee there's always a terminating '\0', now strlcpy()
doesn't pad the result, however if we initialize the ->comm to all 0s in
fork() or thereabouts, we should get this guarantee from the strlcpy()
since that will never replace the last byte with anything but 0.
That barrier is indeed completely pointless as there's no pairing
barrier anywhere.
On 08/15, Peter Zijlstra wrote:
>
> On Wed, 2012-08-15 at 15:01 +0200, Oleg Nesterov wrote:
> > BTW, set_task_comm()->wmb() and memset() should die. There are
> > not needed afaics, and the comment is misleading.
>
> As long as we guarantee there's always a terminating '\0',
Yes, but we already have this guarantee?
Unless of course some buggy code does something wrong with task->comm[],
but nobody should do this.
IOW, task->comm[TASK_COMM_LEN - 1] is always 0, no?
> now strlcpy()
> doesn't pad the result,
afaics set_task_comm()->strlcpy() doesn't change the last byte too.
> however if we initialize the ->comm to all 0s in
> fork()
fork() is special, yes. ->comm is copied by dup_task_struct() and
the new task_struct can have everything in ->comm. But nobody can
see the new task yet, and nobody can play with its ->comm.
Or I misunderstood?
> That barrier is indeed completely pointless as there's no pairing
> barrier anywhere.
Yes, agreed.
Oleg.
On Wed, Aug 15, 2012 at 10:56 AM, Oleg Nesterov <[email protected]> wrote:
> On 08/15, Peter Zijlstra wrote:
>>
>> On Wed, 2012-08-15 at 15:01 +0200, Oleg Nesterov wrote:
>> > BTW, set_task_comm()->wmb() and memset() should die. There are
>> > not needed afaics, and the comment is misleading.
>>
>> As long as we guarantee there's always a terminating '\0',
>
> Yes, but we already have this guarantee?
>
> Unless of course some buggy code does something wrong with task->comm[],
> but nobody should do this.
>
> IOW, task->comm[TASK_COMM_LEN - 1] is always 0, no?
>
>> now strlcpy()
>> doesn't pad the result,
>
> afaics set_task_comm()->strlcpy() doesn't change the last byte too.
>
>> however if we initialize the ->comm to all 0s in
>> fork()
>
> fork() is special, yes. ->comm is copied by dup_task_struct() and
> the new task_struct can have everything in ->comm. But nobody can
> see the new task yet, and nobody can play with its ->comm.
>
> Or I misunderstood?
>
>> That barrier is indeed completely pointless as there's no pairing
>> barrier anywhere.
>
> Yes, agreed.
It sounds like get_task_comm shouldn't have locking at all then? It
should just do a length-limited copy and make sure there is a trailing
0-byte?
-Kees
--
Kees Cook
Chrome OS Security
On 08/15, Kees Cook wrote:
>
> It sounds like get_task_comm shouldn't have locking at all then? It
> should just do a length-limited copy
Without task_lock() get_task_comm() can copy incomplete new name.
Honestly, I do not know any user which "strictly" needs the correct
name. may be proc.
> and make sure there is a trailing
> 0-byte?
get_task_comm()->strncpy() should always see (and copy) 0-byte.
comm[TASK_COMM_LEN - 1] == '\0' and this byte is never changed.
set_task_comm()->strlcpy() can write to this byte, but it can
only write 0 again.
Or I am totally confused ;)
Oleg.
On Wed, Aug 15, 2012 at 11:17 AM, Oleg Nesterov <[email protected]> wrote:
> On 08/15, Kees Cook wrote:
>>
>> It sounds like get_task_comm shouldn't have locking at all then? It
>> should just do a length-limited copy
>
> Without task_lock() get_task_comm() can copy incomplete new name.
>
> Honestly, I do not know any user which "strictly" needs the correct
> name. may be proc.
Right, which is my point -- if the race to read against
set_task_comm() isn't useful to anything, why lock in get_task_comm at
all?
>
>> and make sure there is a trailing
>> 0-byte?
>
> get_task_comm()->strncpy() should always see (and copy) 0-byte.
> comm[TASK_COMM_LEN - 1] == '\0' and this byte is never changed.
>
> set_task_comm()->strlcpy() can write to this byte, but it can
> only write 0 again.
Right, and set_task_comm even does a memset() of 0 over the whole area
before the strlcpy too.
Regardless, it sounds like just using ->comm directly is fine; I'll
send a patch.
-Kees
--
Kees Cook
Chrome OS Security
> It sounds like get_task_comm shouldn't have locking at all then? It
> should just do a length-limited copy and make sure there is a trailing
> 0-byte?
It has locking so that it has a consistent state and more importantly it
has an accessor function
Directly accessing it is asking for bugs in future. If you hold the
needed lock then just add an
__get_task_comm()
method that asserts the lock is held. That way the rest of the behaviour
remains properly encapsulated for when someone changes it.
Alan
On Wed, Aug 15, 2012 at 11:44 AM, Alan Cox <[email protected]> wrote:
>> It sounds like get_task_comm shouldn't have locking at all then? It
>> should just do a length-limited copy and make sure there is a trailing
>> 0-byte?
>
> It has locking so that it has a consistent state and more importantly it
> has an accessor function
>
> Directly accessing it is asking for bugs in future. If you hold the
> needed lock then just add an
>
> __get_task_comm()
>
> method that asserts the lock is held. That way the rest of the behaviour
> remains properly encapsulated for when someone changes it.
But what's been discussed is that no lock is needed if the caller
doesn't care about the accuracy of the contents, which is the
situation I'm in. Looking at other readers of ->comm, they just either
use it directly or copy it directly. Only when accuracy matters does
the kernel use get_task_comm.
-Kees
--
Kees Cook
Chrome OS Security