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 12274C5B54A for ; Wed, 28 Aug 2024 14:12:31 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 7B33F6B0088; Wed, 28 Aug 2024 10:12:31 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 73B8B6B0089; Wed, 28 Aug 2024 10:12:31 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 5DC246B008A; Wed, 28 Aug 2024 10:12:31 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0013.hostedemail.com [216.40.44.13]) by kanga.kvack.org (Postfix) with ESMTP id 3A8B86B0088 for ; Wed, 28 Aug 2024 10:12:31 -0400 (EDT) Received: from smtpin22.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay09.hostedemail.com (Postfix) with ESMTP id A3EEB81EEC for ; Wed, 28 Aug 2024 14:12:30 +0000 (UTC) X-FDA: 82501844460.22.DABEFA5 Received: from nyc.source.kernel.org (nyc.source.kernel.org [147.75.193.91]) by imf10.hostedemail.com (Postfix) with ESMTP id 0CD7BC000D for ; Wed, 28 Aug 2024 14:12:28 +0000 (UTC) Authentication-Results: imf10.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b="uYHfzpm/"; spf=pass (imf10.hostedemail.com: domain of kees@kernel.org designates 147.75.193.91 as permitted sender) smtp.mailfrom=kees@kernel.org; dmarc=pass (policy=quarantine) header.from=kernel.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1724854328; 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:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=/GuIlzSq/4340be0Nn301k4fT7jG7Vo6T7amJ0PNcag=; b=Ih/CD0v7lz/mCFTwpF304sZrmOLLbwE2lKdvBOFFY2KOPlv6r72DlbzGxWsfwkpwsrVWfF E5r81tsjpSWQ5gIEkJKHqbevqD9egVf+V3JpKPZmEcv4qpuVdVrMC/ZcbrKjKl+n40uJjU bezsVlHTAFiopZgOgrmYjm9eqTeIxvQ= ARC-Authentication-Results: i=1; imf10.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b="uYHfzpm/"; spf=pass (imf10.hostedemail.com: domain of kees@kernel.org designates 147.75.193.91 as permitted sender) smtp.mailfrom=kees@kernel.org; dmarc=pass (policy=quarantine) header.from=kernel.org ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1724854328; a=rsa-sha256; cv=none; b=JfeM5eJzqk09+cSUL3wgVHvM5PuiGfu6J/34uRa3vGLw9xRzYpY19lCeeZUXSm0q5F01AQ KzEuv3/uVc5dIRkkvtugqwe1nslqz+0OFWFgthWneSG3zHqlleqCiHno9+RK+mVHe8+A3R HP6/r9NX5jqFGKMMDfU8Av5ureffLPE= Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by nyc.source.kernel.org (Postfix) with ESMTP id 2361FA40C40; Wed, 28 Aug 2024 14:12:21 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id A81A1C98ED1; Wed, 28 Aug 2024 14:03:57 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1724853837; bh=lcHIdMYhBsJloW9fSwc6JWgDLrvYPjB0GKNuKWcKMcA=; h=Date:From:To:CC:Subject:In-Reply-To:References:From; b=uYHfzpm/VEEl9wksfaG1d5QteWMrPN+ZXmcPmB4Ia/soAnFKfgp9mhZnydUdJy6t3 5hOMGih31y5Y0fN1t96rLTITQopjIB1gRZgSkMnTAd5YlwLPxUACpAe4a5PXgNd1x2 hpU8nDF5Zc6Bc8653vJzBfHxUfifFRTQnHGsz/tw6NzvgFnHzEfDdXZeB7kEqktYAm Wc8LHvMMW1amVNWZ0JQzHloemAhzlGZNsRMSB7QJdgQgyDYRfzf/czVtJC+gsxoSV0 E1HK5namjymsUD6QBpH/p4y8aAuzx2hW1oimDQwAINC0Kv0Tg+aurxOigRs5WPigA4 Z3zuMYGKFHTcQ== Date: Wed, 28 Aug 2024 07:03:58 -0700 From: Kees Cook To: Yafang Shao , akpm@linux-foundation.org CC: torvalds@linux-foundation.org, alx@kernel.org, justinstitt@google.com, ebiederm@xmission.com, alexei.starovoitov@gmail.com, rostedt@goodmis.org, catalin.marinas@arm.com, penguin-kernel@i-love.sakura.ne.jp, linux-mm@kvack.org, linux-fsdevel@vger.kernel.org, linux-trace-kernel@vger.kernel.org, audit@vger.kernel.org, linux-security-module@vger.kernel.org, selinux@vger.kernel.org, bpf@vger.kernel.org, netdev@vger.kernel.org, dri-devel@lists.freedesktop.org, Alexander Viro , Christian Brauner , Jan Kara , Kees Cook , Matus Jokay , "Serge E. Hallyn" Subject: Re: [PATCH v8 1/8] Get rid of __get_task_comm() User-Agent: K-9 Mail for Android In-Reply-To: <20240828030321.20688-2-laoar.shao@gmail.com> References: <20240828030321.20688-1-laoar.shao@gmail.com> <20240828030321.20688-2-laoar.shao@gmail.com> Message-ID: <8A36564D-56E3-469B-B201-0BD7C11D6EFC@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Rspam-User: X-Stat-Signature: at33xd1n7xh1e819tscq3d71oo8ubnjp X-Rspamd-Queue-Id: 0CD7BC000D X-Rspamd-Server: rspam11 X-HE-Tag: 1724854348-153354 X-HE-Meta: U2FsdGVkX1/3cMQjYifWnoAp/idx8v5MgSbDeFKyjUOCggnOPuawu2ShLc9pI3pteDFYibirCFcW3l0F0Xai84O/j5HzG1GleR4kxIjPH4Yqbv1XZpPP/a0rgsfuCaXXXHEBwceVn7sEd+N47bFSODEnOruBnVEWB8uj8vvpe+n6RMfPICPMXo9V6YewT+gBauL/zyb3ixTLs5V6xjivaBiMNZFApL8r4doZJQ7RLMI4XcVDTF1fslP4LIp/laXhwVCoqP4bQIQuu7Ys6A4VWqDFAwW+2OTxv4dpjW2LoiOQC+Zdqh0YlUl4QMzSKpcyY1BrcGWVARcN3w4ZdEsCaZo79im5juyIGEFYhHYizU0AzlLFyMuzDMNmo0oZ1aCp9DFBXCaCW4eCayXu2//tWsbaV9YmyNVKlWKAOWFYHYJN5V49/wT9wkV5FdSW6N1MHa74iVDLZuuSonOPOVmo9Y8dIqO4qW4GmtnJUL+VwYOEqBBM4kHa7U+6WoVCktNUuz5V+etr6xnDeWr2KWXr/KPdmzzgB5Mcl0TGRmd7dYaxNPYCtZs0L9y9Sip+7FNI25vL144Tt3gzLuiAaWqSThW+SVqe/tLDJTCuBBCYnowNwDa4f6FmWw5HmwSLV7oTv6VJOvDxRMUafoc7M+4Up7pjg1tyMm6yd4rYJkjUT2aDn0llRWIhP48VrE5z8YwmUO7VDDhfqcDZBFVDLnZEywUTvlxXl/07AGHwiMfBclzOr4+JDSp5w0OP2L23yt+5omDz+XdSBNqNDvW5hY3/fcDYefk1tL0gUa3NC3t8Pj/QWMmKrUARLOxlgtP4ZB/o5w8/XHNh+V5F2g/0VEJZwwVTgmJu+bTZ3QEG5aW0HS2E1zyFDEABCqhdYET9n9tRwGwUPEV4XZl4XRDxk0nEAQWoFp1b3kr+6uRjPavIsYQW0I2UYAYRIRSat9KmMy6AN10t2p3ef0nxPt+czKY I7iMzLGc HEtnjPZOORzwRpvRnqQ7IYreWyIU1cMV+eLbG6TXQoJ+LPWx2se9t5U364fNnZ7MuceekV9fWDzSsDxPAtiLYSHNU+2PY399KNQ192yx7qvmTz4BM52QqIZHkSlr/af8OySxV6wjiYEejQcqB7hCSq73zy09331/++PnMzmZ0Q3c0XaajzHiRiSjrhPr7doRsVOTYYm69Q2D9ZLSLBSkeEXxIsoQTk3CaIFuIzCl8MqfRdfY+0wpnj88iheho2RcKSXkLWssyi56aFIciW9kgcrVTAoguXti6dK5any0ZjzNsK7/GmyykwxUjhxBgSfQWoVeOoGnEEi1NMLGaJfNx5GqZz3kIfKGXvAodwdzxsRKcquV3EK6Q5GgLLCyeQqM6E/X3EncQFHFvm4Zq0Y6NIxs76s9w+FKipEEzARA7q7JNdlv7BcjE7H2jk7IMsNop+rgClg1cwUe3EkR7U3KVdM1YOEXKpuTOzHv123CyQnmrxn5ocXauhs2rsSoeeJQNFaXVGR/I6EX+tEyMNLQ6L3XcMtgZKS5chSrMF8bGg1M05Vd0ZtccDiBMDHoZ7G92ZsEB5JOvg1ct54Atha/Sb3ZBxKig8qhzOUiMpN+yRhbV5hnKumTnNzWdyoUK6PWoTE8CIt0U2INfLRkNgpHadX1f/5/txOvXpQrd+ERvlzBlQ5DHn142ChkbOh8rbvy8UI4l 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 August 27, 2024 8:03:14 PM PDT, Yafang Shao = wrote: >We want to eliminate the use of __get_task_comm() for the following >reasons: > >- The task_lock() is unnecessary > Quoted from Linus [0]: > : Since user space can randomly change their names anyway, using lockin= g > : was always wrong for readers (for writers it probably does make sense > : to have some lock - although practically speaking nobody cares there > : either, but at least for a writer some kind of race could have > : long-term mixed results > >- The BUILD_BUG_ON() doesn't add any value > The only requirement is to ensure that the destination buffer is a vali= d > array=2E Sorry, that's not a correct evaluation=2E See below=2E > >- Zeroing is not necessary in current use cases > To avoid confusion, we should remove it=2E Moreover, not zeroing could > potentially make it easier to uncover bugs=2E If the caller needs a > zero-padded task name, it should be explicitly handled at the call site= =2E This is also not an appropriate rationale=2E We don't make the kernel "mor= e buggy" not purpose=2E ;) See below=2E > >Suggested-by: Linus Torvalds >Link: https://lore=2Ekernel=2Eorg/all/CAHk-=3DwivfrF0_zvf+oj6=3D=3DSh=3D-= npJooP8chLPEfaFV0oNYTTBA@mail=2Egmail=2Ecom [0] >Link: https://lore=2Ekernel=2Eorg/all/CAHk-=3DwhWtUC-AjmGJveAETKOMeMFSTwK= wu99v7+b6AyHMmaDFA@mail=2Egmail=2Ecom/ >Suggested-by: Alejandro Colomar >Link: https://lore=2Ekernel=2Eorg/all/2jxak5v6dfxlpbxhpm3ey7oup4g2lnr3ueu= rfbosf5wdo65dk4@srb3hsk72zwq >Signed-off-by: Yafang Shao >Cc: Alexander Viro >Cc: Christian Brauner >Cc: Jan Kara >Cc: Eric Biederman >Cc: Kees Cook >Cc: Alexei Starovoitov >Cc: Matus Jokay >Cc: Alejandro Colomar >Cc: "Serge E=2E Hallyn" >--- > fs/exec=2Ec | 10 ---------- > fs/proc/array=2Ec | 2 +- > include/linux/sched=2Eh | 32 ++++++++++++++++++++++++++------ > kernel/kthread=2Ec | 2 +- > 4 files changed, 28 insertions(+), 18 deletions(-) > >diff --git a/fs/exec=2Ec b/fs/exec=2Ec >index 50e76cc633c4=2E=2E8a23171bc3c3 100644 >--- a/fs/exec=2Ec >+++ b/fs/exec=2Ec >@@ -1264,16 +1264,6 @@ static int unshare_sighand(struct task_struct *me) > return 0; > } >=20 >-char *__get_task_comm(char *buf, size_t buf_size, struct task_struct *ts= k) >-{ >- task_lock(tsk); >- /* Always NUL terminated and zero-padded */ >- strscpy_pad(buf, tsk->comm, buf_size); >- task_unlock(tsk); >- return buf; >-} >-EXPORT_SYMBOL_GPL(__get_task_comm); >- > /* > * These functions flushes out all traces of the currently running execu= table > * so that a new one can be started >diff --git a/fs/proc/array=2Ec b/fs/proc/array=2Ec >index 34a47fb0c57f=2E=2E55ed3510d2bb 100644 >--- a/fs/proc/array=2Ec >+++ b/fs/proc/array=2Ec >@@ -109,7 +109,7 @@ void proc_task_name(struct seq_file *m, struct task_s= truct *p, bool escape) > else if (p->flags & PF_KTHREAD) > get_kthread_comm(tcomm, sizeof(tcomm), p); > else >- __get_task_comm(tcomm, sizeof(tcomm), p); >+ get_task_comm(tcomm, p); >=20 > if (escape) > seq_escape_str(m, tcomm, ESCAPE_SPACE | ESCAPE_SPECIAL, "\n\\"); >diff --git a/include/linux/sched=2Eh b/include/linux/sched=2Eh >index f8d150343d42=2E=2Ec40b95a79d80 100644 >--- a/include/linux/sched=2Eh >+++ b/include/linux/sched=2Eh >@@ -1096,9 +1096,12 @@ struct task_struct { > /* > * executable name, excluding path=2E > * >- * - normally initialized setup_new_exec() >- * - access it with [gs]et_task_comm() >- * - lock it with task_lock() >+ * - normally initialized begin_new_exec() >+ * - set it with set_task_comm() >+ * - strscpy_pad() to ensure it is always NUL-terminated and >+ * zero-padded >+ * - task_lock() to ensure the operation is atomic and the name is >+ * fully updated=2E > */ > char comm[TASK_COMM_LEN]; >=20 >@@ -1914,10 +1917,27 @@ static inline void set_task_comm(struct task_stru= ct *tsk, const char *from) > __set_task_comm(tsk, from, false); > } >=20 >-extern char *__get_task_comm(char *to, size_t len, struct task_struct *t= sk); >+/* >+ * - Why not use task_lock()? >+ * User space can randomly change their names anyway, so locking for r= eaders >+ * doesn't make sense=2E For writers, locking is probably necessary, a= s a race >+ * condition could lead to long-term mixed results=2E >+ * The strscpy_pad() in __set_task_comm() can ensure that the task com= m is >+ * always NUL-terminated and zero-padded=2E Therefore the race conditi= on between >+ * reader and writer is not an issue=2E >+ * >+ * - Why not use strscpy_pad()? >+ * While strscpy_pad() prevents writing garbage past the NUL terminato= r, which >+ * is useful when using the task name as a key in a hash map, most use= cases >+ * don't require this=2E Zero-padding might confuse users if it=E2=80= =99s unnecessary, >+ * and not zeroing might even make it easier to expose bugs=2E If you = need a >+ * zero-padded task name, please handle that explicitly at the call si= te=2E I really don't like this part of the change=2E You don't know that existin= g callers don't depend on the padding=2E Please invert this logic: get_task= _comm() must use strscpy_pad()=2E Calls NOT wanting padding can call strscp= y() themselves=2E >+ * >+ * - ARRAY_SIZE() can help ensure that @buf is indeed an array=2E This doesn't need checking here; strscpy() will already do that=2E=20 >+ */ > #define get_task_comm(buf, tsk) ({ \ >- BUILD_BUG_ON(sizeof(buf) !=3D TASK_COMM_LEN); \ Also, please leave the TASK_COMM_LEN test so that destination buffers cont= inue to be the correct size: current callers do not perform any return valu= e analysis, so they cannot accidentally start having situations where the d= estination string might be truncated=2E Again, anyone wanting to avoid that= restriction can use strscpy() directly and check the return value=2E >- __get_task_comm(buf, sizeof(buf), tsk); \ >+ strscpy(buf, (tsk)->comm, ARRAY_SIZE(buf)); \ >+ buf; \ > }) >=20 > #ifdef CONFIG_SMP >diff --git a/kernel/kthread=2Ec b/kernel/kthread=2Ec >index f7be976ff88a=2E=2E7d001d033cf9 100644 >--- a/kernel/kthread=2Ec >+++ b/kernel/kthread=2Ec >@@ -101,7 +101,7 @@ void get_kthread_comm(char *buf, size_t buf_size, str= uct task_struct *tsk) > struct kthread *kthread =3D to_kthread(tsk); >=20 > if (!kthread || !kthread->full_name) { >- __get_task_comm(buf, buf_size, tsk); >+ strscpy(buf, tsk->comm, buf_size); Why is this safe to not use strscpy_pad? Also, is buf_size ever not TASK_C= OMM_LEN? > return; > } >=20 --=20 Kees Cook