Re: yama_ptrace_access_check(): possible recursive locking detected

2012-08-15 Thread Kees Cook
On Wed, Aug 15, 2012 at 11:44 AM, Alan Cox 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

Re: yama_ptrace_access_check(): possible recursive locking detected

2012-08-15 Thread Alan Cox
> 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

Re: yama_ptrace_access_check(): possible recursive locking detected

2012-08-15 Thread Kees Cook
On Wed, Aug 15, 2012 at 11:17 AM, Oleg Nesterov 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

Re: yama_ptrace_access_check(): possible recursive locking detected

2012-08-15 Thread Oleg Nesterov
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. >

Re: yama_ptrace_access_check(): possible recursive locking detected

2012-08-15 Thread Kees Cook
On Wed, Aug 15, 2012 at 10:56 AM, Oleg Nesterov 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

Re: yama_ptrace_access_check(): possible recursive locking detected

2012-08-15 Thread Oleg Nesterov
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

Re: yama_ptrace_access_check(): possible recursive locking detected

2012-08-15 Thread Peter Zijlstra
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

Re: yama_ptrace_access_check(): possible recursive locking detected

2012-08-15 Thread Oleg Nesterov
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

Re: yama_ptrace_access_check(): possible recursive locking detected

2012-08-15 Thread Peter Zijlstra
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? -- To unsubscribe from this list:

Re: yama_ptrace_access_check(): possible recursive locking detected

2012-08-15 Thread Peter Zijlstra
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? -- To unsubscribe from this list: send

Re: yama_ptrace_access_check(): possible recursive locking detected

2012-08-15 Thread Oleg Nesterov
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

Re: yama_ptrace_access_check(): possible recursive locking detected

2012-08-15 Thread Peter Zijlstra
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

Re: yama_ptrace_access_check(): possible recursive locking detected

2012-08-15 Thread Oleg Nesterov
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

Re: yama_ptrace_access_check(): possible recursive locking detected

2012-08-15 Thread Kees Cook
On Wed, Aug 15, 2012 at 10:56 AM, Oleg Nesterov o...@redhat.com 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

Re: yama_ptrace_access_check(): possible recursive locking detected

2012-08-15 Thread Oleg Nesterov
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

Re: yama_ptrace_access_check(): possible recursive locking detected

2012-08-15 Thread Kees Cook
On Wed, Aug 15, 2012 at 11:17 AM, Oleg Nesterov o...@redhat.com 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

Re: yama_ptrace_access_check(): possible recursive locking detected

2012-08-15 Thread Alan Cox
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

Re: yama_ptrace_access_check(): possible recursive locking detected

2012-08-15 Thread Kees Cook
On Wed, Aug 15, 2012 at 11:44 AM, Alan Cox a...@lxorguk.ukuu.org.uk 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

Re: yama_ptrace_access_check(): possible recursive locking detected

2012-08-14 Thread Kees Cook
On Tue, Aug 14, 2012 at 8:01 PM, Fengguang Wu 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 wrote: >> > On Thu, Aug 09, 2012 at 06:39:34PM -0700, Kees Cook wrote: >> >> Hi, >> >> >> >> So, after taking a closer look at this, I

Re: yama_ptrace_access_check(): possible recursive locking detected

2012-08-14 Thread Fengguang Wu
On Tue, Aug 14, 2012 at 02:16:52PM -0700, Kees Cook wrote: > On Thu, Aug 9, 2012 at 6:52 PM, Fengguang Wu 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

Re: yama_ptrace_access_check(): possible recursive locking detected

2012-08-14 Thread Kees Cook
On Thu, Aug 9, 2012 at 6:52 PM, Fengguang Wu 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

Re: yama_ptrace_access_check(): possible recursive locking detected

2012-08-14 Thread Kees Cook
On Thu, Aug 9, 2012 at 6:52 PM, Fengguang Wu fengguang...@intel.com 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

Re: yama_ptrace_access_check(): possible recursive locking detected

2012-08-14 Thread Fengguang Wu
On Tue, Aug 14, 2012 at 02:16:52PM -0700, Kees Cook wrote: On Thu, Aug 9, 2012 at 6:52 PM, Fengguang Wu fengguang...@intel.com 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

Re: yama_ptrace_access_check(): possible recursive locking detected

2012-08-14 Thread Kees Cook
On Tue, Aug 14, 2012 at 8:01 PM, Fengguang Wu fengguang...@intel.com 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 fengguang...@intel.com wrote: On Thu, Aug 09, 2012 at 06:39:34PM -0700, Kees Cook wrote: Hi, So, after

Re: yama_ptrace_access_check(): possible recursive locking detected

2012-08-09 Thread Fengguang Wu
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

Re: yama_ptrace_access_check(): possible recursive locking detected

2012-08-09 Thread Kees Cook
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

Re: yama_ptrace_access_check(): possible recursive locking detected

2012-08-09 Thread Kees Cook
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

Re: yama_ptrace_access_check(): possible recursive locking detected

2012-08-09 Thread Fengguang Wu
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 !=

Re: yama_ptrace_access_check(): possible recursive locking detected

2012-07-30 Thread Kees Cook
On Thu, Jul 26, 2012 at 8:41 AM, Oleg Nesterov wrote: > On 07/26, Fengguang Wu wrote: >> >> Here is a recursive lock possibility: >> >> ptrace_may_access() >> =>task_lock(task); >> yama_ptrace_access_check() >> get_task_comm() >> =>

Re: yama_ptrace_access_check(): possible recursive locking detected

2012-07-30 Thread Kees Cook
On Thu, Jul 26, 2012 at 8:41 AM, Oleg Nesterov o...@redhat.com wrote: On 07/26, Fengguang Wu wrote: Here is a recursive lock possibility: ptrace_may_access() =task_lock(task); yama_ptrace_access_check() get_task_comm() =

Re: yama_ptrace_access_check(): possible recursive locking detected

2012-07-26 Thread Oleg Nesterov
On 07/26, Fengguang Wu wrote: > > Here is a recursive lock possibility: > > ptrace_may_access() > =>task_lock(task); > yama_ptrace_access_check() > get_task_comm() > => task_lock(task); I think yama_ptrace_access_check() can simply use ->comm

Re: yama_ptrace_access_check(): possible recursive locking detected

2012-07-26 Thread Oleg Nesterov
On 07/26, Fengguang Wu wrote: Here is a recursive lock possibility: ptrace_may_access() =task_lock(task); yama_ptrace_access_check() get_task_comm() = task_lock(task); I think yama_ptrace_access_check() can simply use -comm Oleg.