On Wed, 2016-11-02 at 21:50 +0100, Jann Horn wrote:
> On Wed, Nov 02, 2016 at 07:18:06PM +0100, Oleg Nesterov wrote:
> > On 10/30, Jann Horn wrote:
> > > 
> > > This is a new per-threadgroup lock that can often be taken instead of
> > > cred_guard_mutex and has less deadlock potential. I'm doing this because
> > > Oleg Nesterov mentioned the potential for deadlocks, in particular if a
> > > debugged task is stuck in execve, trying to get rid of a ptrace-stopped
> > > thread, and the debugger attempts to inspect procfs files of the debugged
> > > task.
> > 
> > 
> > Yes, but let me repeat that we need to fix this anyway. So I don't really
> > understand why should we add yet another mutex.
> 
> 
> execve() only takes the new mutex immediately after de_thread(), so this
> problem shouldn't occur there. Basically, I think that I'm not making the
> problem worse with my patches this way.
> 
> I believe that it should be possible to convert most existing users of the
> cred_guard_mutex to the new cred_guard_light - exceptions to that that I
> see are:
> 
>  - PTRACE_ATTACH
>  - SECCOMP_FILTER_FLAG_TSYNC (sets NO_NEW_PRIVS on remote task)
> 
> Beyond that, conceptually, the new cred_guard_light could also be turned
> into a read-write mutex to prevent deadlocks between its users (where
> execve would take it for writing and everyone else would take it for
> reading), but afaik the kernel doesn't have an implementation of
> read-write mutexes yet?
[...]

It does (rw_semaphore), but with PREEMPT_RT enabled they become simple
mutexes.  So I don't think we should rely on that to avoid deadlock.

Ben.

-- 
Ben Hutchings
The world is coming to an end.	Please log off.