From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 139291BF43 for ; Tue, 16 Jan 2024 15:23:40 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=redhat.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b="aRNiqkiA" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1705418619; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=95o70husdyrbHX9ZG97EzcVPleiqbXjcOXKaS79W3CE=; b=aRNiqkiA+4W3dmq2Z07xl+ilfUTn4pwcWRs9uKe/xVpID6fWOZ/NfhejtcW7tdie3fSQWp msPn+PSWmDQexW9qZs15LeyYSMo0vxMpmzlwMTt743vs++GybWUuEgxKmbxIyrsY+YKJNc Cr/RSTZxLl++RsxmFubCoRQX5m57Ze0= Received: from mimecast-mx02.redhat.com (mx-ext.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-313-k8Dnsa1DNG-rwjU6HiPbdA-1; Tue, 16 Jan 2024 10:23:37 -0500 X-MC-Unique: k8Dnsa1DNG-rwjU6HiPbdA-1 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id B08983C29A72; Tue, 16 Jan 2024 15:23:32 +0000 (UTC) Received: from dhcp-27-174.brq.redhat.com (unknown [10.45.224.96]) by smtp.corp.redhat.com (Postfix) with SMTP id 0014F2026D6F; Tue, 16 Jan 2024 15:23:23 +0000 (UTC) Received: by dhcp-27-174.brq.redhat.com (nbSMTP-1.00) for uid 1000 oleg@redhat.com; Tue, 16 Jan 2024 16:22:20 +0100 (CET) Date: Tue, 16 Jan 2024 16:22:11 +0100 From: Oleg Nesterov To: Bernd Edlinger Cc: Alexander Viro , Alexey Dobriyan , Kees Cook , Andy Lutomirski , Will Drewry , Christian Brauner , Andrew Morton , Michal Hocko , Serge Hallyn , James Morris , Randy Dunlap , Suren Baghdasaryan , Yafang Shao , Helge Deller , "Eric W. Biederman" , Adrian Reber , Thomas Gleixner , Jens Axboe , Alexei Starovoitov , "linux-fsdevel@vger.kernel.org" , "linux-kernel@vger.kernel.org" , linux-kselftest@vger.kernel.org, linux-mm@kvack.org, tiozhang , Luis Chamberlain , "Paulo Alcantara (SUSE)" , Sergey Senozhatsky , Frederic Weisbecker , YueHaibing , Paul Moore , Aleksa Sarai , Stefan Roesch , Chao Yu , xu xin , Jeff Layton , Jan Kara , David Hildenbrand , Dave Chinner , Shuah Khan , Zheng Yejian , Elena Reshetova , David Windsor , Mateusz Guzik , Ard Biesheuvel , "Joel Fernandes (Google)" , "Matthew Wilcox (Oracle)" , Hans Liljestrand Subject: Re: [PATCH v14] exec: Fix dead-lock in de_thread with ptrace_attach Message-ID: <20240116152210.GA12342@redhat.com> References: Precedence: bulk X-Mailing-List: linux-fsdevel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.24 (2015-08-30) X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.4 I'll try to recall this problem and actually read the patch tommorrow... Hmm. but it doesn't apply to Linus's tree, you need to rebase it. In particular, please note the recent commit 5431fdd2c181dd2eac2 ("ptrace: Convert ptrace_attach() to use lock guards") On 01/15, Bernd Edlinger wrote: > > The problem happens when a tracer tries to ptrace_attach > to a multi-threaded process, that does an execve in one of > the threads at the same time, without doing that in a forked > sub-process. That means: There is a race condition, when one > or more of the threads are already ptraced, but the thread > that invoked the execve is not yet traced. Now in this > case the execve locks the cred_guard_mutex and waits for > de_thread to complete. But that waits for the traced > sibling threads to exit, and those have to wait for the > tracer to receive the exit signal, but the tracer cannot > call wait right now, because it is waiting for the ptrace > call to complete, and this never does not happen. > The traced process and the tracer are now in a deadlock > situation, and can only be killed by a fatal signal. This looks very confusing to me. And even misleading. So IIRC the problem is "simple". de_thread() sleeps with cred_guard_mutex waiting for other threads to exit and pass release_task/__exit_signal. If one of the sub-threads is traced, debugger should do ptrace_detach() or wait() to release this tracee, the killed tracee won't autoreap. Now. If debugger tries to take the same cred_guard_mutex before detach/wait we have a deadlock. This is not specific to ptrace_attach(), proc_pid_attr_write() takes this lock too. Right? Or are there other issues? > -static int de_thread(struct task_struct *tsk) > +static int de_thread(struct task_struct *tsk, struct linux_binprm *bprm) > { > struct signal_struct *sig = tsk->signal; > struct sighand_struct *oldsighand = tsk->sighand; > spinlock_t *lock = &oldsighand->siglock; > + struct task_struct *t = tsk; > + bool unsafe_execve_in_progress = false; > > if (thread_group_empty(tsk)) > goto no_thread_group; > @@ -1066,6 +1068,19 @@ static int de_thread(struct task_struct *tsk) > if (!thread_group_leader(tsk)) > sig->notify_count--; > > + while_each_thread(tsk, t) { for_other_threads() > + if (unlikely(t->ptrace) > + && (t != tsk->group_leader || !t->exit_state)) > + unsafe_execve_in_progress = true; The !t->exit_state is not right... This sub-thread can already be a zombie with ->exit_state != 0 but see above, it won't be reaped until the debugger does wait(). > + if (unlikely(unsafe_execve_in_progress)) { > + spin_unlock_irq(lock); > + sig->exec_bprm = bprm; > + mutex_unlock(&sig->cred_guard_mutex); > + spin_lock_irq(lock); I don't understand why do we need to unlock and lock siglock here... But my main question is why do we need the unsafe_execve_in_progress boolean. If this patch is correct and de_thread() can drop and re-acquire cread_guard_mutex when one of the threads is traced, then why can't we do this unconditionally ? Oleg.