On Wed, Nov 02, 2016 at 03:38:53PM -0600, Ben Hutchings wrote: > 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) Aren't those spinlocks? I don't think those would be appropriate here, considering that the cred_guard_light can be held during filesystem access during execve(). > but with PREEMPT_RT enabled they become simple > mutexes. So I don't think we should rely on that to avoid deadlock. Well, I don't think it's really necessary - as far as I can tell, the only locking operations that would be executed with the cred_guard_light held would be taking the sighand lock, filesystem stuff and LSM stuff.
Attachment:
signature.asc
Description: Digital signature
