From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 92612C47DA2 for ; Wed, 17 Jan 2024 16:40:07 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 05A1A6B00FC; Wed, 17 Jan 2024 11:40:07 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 00A116B011B; Wed, 17 Jan 2024 11:40:06 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id E3B076B011C; Wed, 17 Jan 2024 11:40:06 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0012.hostedemail.com [216.40.44.12]) by kanga.kvack.org (Postfix) with ESMTP id D4D026B00FC for ; Wed, 17 Jan 2024 11:40:06 -0500 (EST) Received: from smtpin09.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay01.hostedemail.com (Postfix) with ESMTP id 7530C1C05F8 for ; Wed, 17 Jan 2024 16:40:06 +0000 (UTC) X-FDA: 81689365212.09.2FF33DE Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by imf25.hostedemail.com (Postfix) with ESMTP id 6379FA0010 for ; Wed, 17 Jan 2024 16:40:04 +0000 (UTC) Authentication-Results: imf25.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=VTWL6mFI; dmarc=pass (policy=none) header.from=redhat.com; spf=pass (imf25.hostedemail.com: domain of oleg@redhat.com designates 170.10.129.124 as permitted sender) smtp.mailfrom=oleg@redhat.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1705509604; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=9MEKaPLuC4dWjyeAAjRSNgqo7o077s9TgRJfiIfemoI=; b=UpqTogb4YcvXJ/zwoGsDSgIV4aQNx/OzlUsLB/s+fpNW1FZhVL4OcwcdOMAIu9bG4By2YA gK8uQigqVEU+BjhGxw9UmfVDrVLfKcdiGG27NPo9XOzDfKu4lp93QXEqHQwDZMUvV/bonw 4dqgIWOoGIwGyfstdqKWeQ62nDd45rU= ARC-Authentication-Results: i=1; imf25.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=VTWL6mFI; dmarc=pass (policy=none) header.from=redhat.com; spf=pass (imf25.hostedemail.com: domain of oleg@redhat.com designates 170.10.129.124 as permitted sender) smtp.mailfrom=oleg@redhat.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1705509604; a=rsa-sha256; cv=none; b=Dit6uzzkxauRdrb19sA4wS59LZo2HsV1S8jVQ4Vi+WplEnDt4nv9DzlAGTNKLFdKRq8Qq+ v3HrV3wjTgWTljSZ4OZSn00U3IZDT7VeOWGcCwlDAvO+hXfkOq/lY9wCnLMiyzIMg067I4 ArEhzopaTXd30s7VH9w/5y179hUmhQo= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1705509603; 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=9MEKaPLuC4dWjyeAAjRSNgqo7o077s9TgRJfiIfemoI=; b=VTWL6mFIKHgfiT04qpUh03rEMxX9Wg7hdpBXbDuldWaZhgMlVzSjW3hINNlgpO6bp5TH6r fv4JVIzwisLkBLWKI4MmRo/JnLsDiBiE+S0ZGVsgwR5W5sO+MOlHczdIuoF5ONz56a/saO GtfxVDL9uYWScw/ycTq9wfU8Juk+jh4= 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-389-a8vvGCybN5WY-ilt55kkNA-1; Wed, 17 Jan 2024 11:39:36 -0500 X-MC-Unique: a8vvGCybN5WY-ilt55kkNA-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.rdu2.redhat.com [10.11.54.1]) (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 921CE1C29EC6; Wed, 17 Jan 2024 16:39:31 +0000 (UTC) Received: from dhcp-27-174.brq.redhat.com (unknown [10.45.224.121]) by smtp.corp.redhat.com (Postfix) with SMTP id 8BF4E3C25; Wed, 17 Jan 2024 16:39:22 +0000 (UTC) Received: by dhcp-27-174.brq.redhat.com (nbSMTP-1.00) for uid 1000 oleg@redhat.com; Wed, 17 Jan 2024 17:38:18 +0100 (CET) Date: Wed, 17 Jan 2024 17:38:09 +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: <20240117163739.GA32526@redhat.com> References: <20240116152210.GA12342@redhat.com> 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.1 X-Rspamd-Queue-Id: 6379FA0010 X-Rspam-User: X-Rspamd-Server: rspam02 X-Stat-Signature: k4pzipoe9p1cre4uyauykrzzxt43p57p X-HE-Tag: 1705509604-381905 X-HE-Meta: U2FsdGVkX1/9vssrfiHyTYbXgkThOZk3e4zBAUkfYz/CcqujXYlgmMDr5hFqBaoy26W0VQ1qPN+B7vfs3Mrnt8MWNx9IMXYeMzHOqYdxrLpLTLz7Xh7VF4tBCPlXnygHAQPY71hpB/fUpASG8D/cQRj7fiHe9/c+xjWYbPC3LzxaseNSv5GnJFugR94mino0yyDJtWA+38fmayXsfsPF6WFBx83MgecU8z8+ukSS+4TfiYRFMfowy7VNjzxq3QorH8/iN7UsvK50JjiwuN3OOKUxGkwndsNSfMjQj/MSVNBSXm3Ua4FcQVTgC4PHE7/fXm4QFoEG74qwH/Gdc1IwoHdr1TynbJZD8DHuFWrhlJOP/y+cZq2be3C/P3Kzmq8z5M8iOuFkZUA1+d+U5nckSTV7HRKguV4U6hmhyZhhAKClEud7/oe2dSUsbq+2ULxehzlZYn09PR9qN0ZeEHp/0+LhOMjmpY75uC9lgpcTlnHJ5D9DCYT/F0tp+Xp/N13zrCdseLMCCkb3CR/Q8b5/uWyBR25uuMRHKmnKJcKjCHQHIvPO0VfdgwzLLzbnphvE7Sl1g6uViy1ezAthi+18eNLrXB9Ks/FJxSUkdzMoEgr9YBVQSjnMmGIW52HL9+B1an1DSiqQKlnx/0O2ozZnNrrakG1bkFGqrDyiDL0b1G9ym70oo0OX0lytsomqnNzZn1hsZIeHRho5esDQU6VAt4LVeiiEeMzISvwyreTZKgz/uA0m2nmvgDlWDI4LG6puYsbySTTJXe5JpHi7vy9yZtxeVoIdMYL1GMWm4K4Eh7uF3mpVJb4+bA5vc3+cB7YdT5/fOrzXI2bGyvHf9i8pNlyeKCn5q5N1PjxV+tT8sBswn1OV95N0vPT3xEimOj1iX3sJRaT7965BVCXqtwoa9OEvnGjz0WSUg05ziSEct0jiu7ukiAfrliZ44tH93MTOmzHiU070SNnTuidOM0R OP2W1OD4 Ekc6hZU9yUYjV3yJVPtLxFsK8Hy29Me8E25o5L9TH4GhNQObyXWM0paFjBSEnVdPymjmgob/Ul3if/8mPj/7zcXweRSsBNyBjKN5A08CjgAny2rudONUPsXMFH49ayv1589CyECik3ZdvEmM1DOt2ETSyOUYQ78HLMkEqyS0fF/RNkdVY2oKjYisIAbA6xpVXB05CyXfTSYpXDMtT+X9nWfB+xtggobGKIJgad2YPfrS4czTO3UOtshoiY91ihONScNNUK6Tn/sTeRj9Dk4KrUrpQN2N2bA+pA64zPN4C0xA0u6lYS6fDu6EV3EZV9m0GRPrQZAQzQrDgK4+Zas1/C5UHikiVs0pb4x/i X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: On 01/17, 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. > > > > Yes. but the tracer has to do its job, and that is ptrace_attach the > remaining treads, it does not know that it would avoid a dead-lock > when it calls wait(), instead of ptrace_attach. It does not know > that the tracee has just called execve in one of the not yet traced > threads. Hmm. I don't understand you. I agree we have a problem which should be fixed. Just the changelog looks confusing to me, imo it doesn't explain the race/problem clearly. > > 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? > > > > No, proc_pid_attr_write has no problem if it waits for cred_guard_mutex, > because it is only called from one of the sibling threads, OK, thanks, I was wrong. I forgot about "A task may only write its own attributes". So yes, ptrace_attach() is the only source of problematic mutex_lock() today. There were more in the past. > >> + 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(). > > > > I dont think so. > de_thread() handles the group_leader different than normal threads. I don't follow... I didn't say that t is a group leader. I said it can be a zombie sub-thread with ->exit_state != 0. > That means normal threads have to wait for being released from the zombie > state by the tracer: > sig->notify_count > 0, and de_thread is woken up by __exit_signal That is what I said before. Debugger should release a zombie sub-thread, it won't do __exit_signal() on its own. > >> + 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... > > That is just a precaution because I did want to release the > mutexes exactly in the reverse order as they were acquired. To me this adds the unnecessary complication. > > 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 ? > > > > I just wanted to keep the impact of the change as small as possible, But the unsafe_execve_in_progress logic increases the impact and complicates the patch. I think the fix should be as simple as possible. (to be honest, right now I don't think this is a right approach). > including > possible performance degradation due to double checking of credentials. Not sure I understand, but you can add the performance improvements later. Not to mention that this should be justified, and the for_other_threads() loop added by this patch into de_thread() is not nice performance-wise. Oleg.