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 AC064C3DA7F for ; Mon, 12 Aug 2024 08:05:38 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 21DFD6B00B6; Mon, 12 Aug 2024 04:05:38 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 1CDF36B00B7; Mon, 12 Aug 2024 04:05:38 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 096766B00B8; Mon, 12 Aug 2024 04:05:38 -0400 (EDT) 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 E0EFA6B00B6 for ; Mon, 12 Aug 2024 04:05:37 -0400 (EDT) Received: from smtpin21.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay09.hostedemail.com (Postfix) with ESMTP id 93AB8808B9 for ; Mon, 12 Aug 2024 08:05:37 +0000 (UTC) X-FDA: 82442859114.21.86F131C Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by imf23.hostedemail.com (Postfix) with ESMTP id 640CF140007 for ; Mon, 12 Aug 2024 08:05:34 +0000 (UTC) Authentication-Results: imf23.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=rnh6yX87; spf=pass (imf23.hostedemail.com: domain of alx@kernel.org designates 139.178.84.217 as permitted sender) smtp.mailfrom=alx@kernel.org; dmarc=pass (policy=none) header.from=kernel.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1723449923; 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=nmES3VXSLQcxv4YjuWqBREgUmh2dQrn9fDHrdfWe5rc=; b=jw7sJb/h9TbNAYMEu+HtEpC953csn+sMGGkqYhFSIIQbshAJYQBJP/ZOT1j6oHAegbIcrV 5wIWwPbEWRvsJYg2R7VkH91+Os8wyc7aS/seF90cquUid3KJjbf/LOLN8wTIQwPp7kAe96 kaKtZvDLX67srqOTSmUANWOesOoNXrQ= ARC-Authentication-Results: i=1; imf23.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=rnh6yX87; spf=pass (imf23.hostedemail.com: domain of alx@kernel.org designates 139.178.84.217 as permitted sender) smtp.mailfrom=alx@kernel.org; dmarc=pass (policy=none) header.from=kernel.org ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1723449923; a=rsa-sha256; cv=none; b=lbRonCrUoSrKsoVNWkrkKCHfwSblFn/6lqj0msng9RxppVI8Xa9bTsPlOhICm121V99Utk RrbQEywv4UCXpmY9R0HIeeCLw9m1rmGk5z4+tR8AsFddgIOLOnh56ge5BCOzoyo5uzoCHt ngsmjKg193uy6zHDYUTHjZHZzWh9v4E= Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by dfw.source.kernel.org (Postfix) with ESMTP id 7397A61088; Mon, 12 Aug 2024 08:05:33 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id F14B7C32782; Mon, 12 Aug 2024 08:05:28 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1723449933; bh=yJMblP9VcpG/FwkXRMSJuZ99TzHFyPGmOF9lfg0RRE8=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=rnh6yX87Uo33YSj+iSA8peN8E2d2OEgVIkIOv5MZ2AE2Vgo+L8snzXLroqvUp5m00 YEwY8o+BYv85p/xikxBhbgda09/EKQzEGVVNsXiyH1UXU1M55C9dSW/oiR2nTYie5A aCj2uUroQVXpGBlpuUQdsIzQ1lgJP6rZyHukPNis/m3QUnX15mdk88rgZvtFpirOqn LYafyds+r43n+qsPCUc3k4zFjasLVrgjaEYviNjUXenLJCHKtkbEBMePMvQkiu24jN aqkzAxClltvV+mgKjb4baqXQBTIuK9sGx2LqeHb3uywWGnxlW9W6fFrVPtLtE2UyVg mizbsRvYJHxzg== Date: Mon, 12 Aug 2024 10:05:26 +0200 From: Alejandro Colomar To: Yafang Shao Cc: akpm@linux-foundation.org, torvalds@linux-foundation.org, 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 v6 1/9] Get rid of __get_task_comm() Message-ID: References: <20240812022933.69850-1-laoar.shao@gmail.com> <20240812022933.69850-2-laoar.shao@gmail.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="62fk5rgm4hdf5yga" Content-Disposition: inline In-Reply-To: <20240812022933.69850-2-laoar.shao@gmail.com> X-Rspam-User: X-Stat-Signature: 686ck7jg3onrwdcbdjua4rpz167hgi5i X-Rspamd-Queue-Id: 640CF140007 X-Rspamd-Server: rspam11 X-HE-Tag: 1723449934-369583 X-HE-Meta: U2FsdGVkX19xqU0ME1AcrqcXj3RJC1gw299z3KbbniMflGaerqrq88uyE7lU/wtBl8Wd9l4nvdnqxb3mOEhfjJ4Kk7cdY+tV1fwXLQTQsRoeekm4mz/B/5/xljisEQW7gJIht9k+kE+BkTfZo4QMElINs2i13d6/CEDPS/d4z+vHrCZ6v8oS/eW2GYwOv1sUBeFmUJ3gALhvN04VEFOCVr/APIN5LG06MlCEWFhQ1uKlIlRvY7AOHadql0fBOgV8F7zbKcd9j0GJ/83jJ+oxTeUqs8BSxkrZnenaOvLWGh5acmTU/hAhtOP4ha5Oyoh020T1tebao+QZ8yQ86+5mRBNtbn58+Ucq2GDSKGWgvBOf5bFzUV6ASQ2EHJmf93YmZS/j+mhob1omyXpKgBnlx+PWxkWkfBAKuKAjo4g9ncZwh0JAgamJ7b9IB9XbdJTMKIU121f5xDWIFaY32OaLt29g6VMm2uK7oQRWpWf/+Zwu2srROd/GCOMVgyi1ko150MSvmKnIuxFXJtumvzjSwBKIYi/FZ8j787G2NxZt//59ZeK37ir7URqXbt9j7Bif2DQvg3WxzXWlxY7oILcTtLI73GSagbneuzeOf4w+9y3bylRF5a06HZunUOqRASulurO3GKj8/VU2RrCWDOq+H6yrmfhuRr2I9MGXnbxPqyzpOYjdA8ZsfIca/L3Z4TD1sQ/RXpWeL2R0mzQM4YCeDk7Sw683+jtY+zq42HHT80rgwAmwi4SE5mTSiAuXngDQ//Lxvx+1CQxeMdpQOGcyaL1l48FJjxlDNlJVITyQx3SDSCISPqPO9ey8DVMKGlSYoFCEN5z+zqatS/SZDyWvgmhDlBSONGKOK6LdSaSJQwClaGbJmHxE7VUVY2HoxkAGEUhQpwnVTELM/l3iaA937UosCULxqvHuPhnNsoLu5dK2Zy0Qc3sBchXE1t8zZctZ4uYtU2wdYJ6+ZI279bR XThJ792A mSgXIayq319V36hLF2beHd1llnPrrJDA2t0rcAeiXAZDCVA36n2QEoe4AyKwXYUTaANmNwdObGLUKqriei5c34V++bhBr+mN4Rji6ZhEHVncYLNRlm6MbG3hru7BNcTpGrA4Tp0OHP9ZSB7PybUByYGidtjlJPqLNeOxzCFaFJchiPaS6auH+sG8ZA56a6DD83ZW9sLSTrhGwX5qj4EXaqDWdj7h43FELWFUOiT3Fi6Qsn4dRM7QzMVpltbKUf+zMqpMoCleaR0eurZcsrWEi0bElJm5sjjX+7XtAxno4b3sBXoXXWi+UEPNBoUYxxInFT05JewIAYFtrxRi839kGXzV2eC6h3YnIHBQgWxF08Sk4MSWvSnkngOux2yFaY0oo5zDCuiKpl+R23WhBk/V6+9gXp8zV3iesuQnwZb7e7Dzkef6x7y1pzbk/nK805Nr2wDJJ70mr5USIVj4XaBXDzi6g83she4gdhk4SYOf3pDjRE3rq2l7/QcjPXefmtA+Tn8o2M7v7cL9M2vbxiSnh/1efv06sR/8wgkOCoXMBAzEHdIPSRbtxgRJuMKIkjqOroc2pCdEDxVtBui0M5Unw1Zq11xXr3djp+26hxf5QVptWQGqGq3IotmIsSTV/+bE7c8WOy3clitl4Xte7+RBLmGFIDlsL/FA5db3m7v7Ks1vLtg2VdVfX3iKpF1MuW7DeFNJT2fexnp9V9E4/F9nKUBSSofIieEKpEhzAIcUWvAYr5NrN01JiEooIR63YfxeAeBSozJ4Lxk4IyYW1bFJ+YV6VYOOxfXkwz+QmjPF9HJv8DS5Gli+HvjfEAilOmqHAaP1ZwnytWU1Osy0EK+Xqb1SdlIAzoTmnMYxik+MLE7HSmJNk0F7qb0U/zQ== 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: --62fk5rgm4hdf5yga Content-Type: text/plain; protected-headers=v1; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable From: Alejandro Colomar To: Yafang Shao Cc: akpm@linux-foundation.org, torvalds@linux-foundation.org, 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 v6 1/9] Get rid of __get_task_comm() References: <20240812022933.69850-1-laoar.shao@gmail.com> <20240812022933.69850-2-laoar.shao@gmail.com> MIME-Version: 1.0 In-Reply-To: <20240812022933.69850-2-laoar.shao@gmail.com> Hi Yafang, On Mon, Aug 12, 2024 at 10:29:25AM GMT, Yafang Shao wrote: > We want to eliminate the use of __get_task_comm() for the following > reasons: >=20 > - The task_lock() is unnecessary > Quoted from Linus [0]: > : Since user space can randomly change their names anyway, using locking > : 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 >=20 > - The BUILD_BUG_ON() doesn't add any value > The only requirement is to ensure that the destination buffer is a valid > array. >=20 > - Zeroing is not necessary in current use cases > To avoid confusion, we should remove it. Moreover, not zeroing could > potentially make it easier to uncover bugs. If the caller needs a > zero-padded task name, it should be explicitly handled at the call site. >=20 > Suggested-by: Linus Torvalds > Link: https://lore.kernel.org/all/CAHk-=3DwivfrF0_zvf+oj6=3D=3DSh=3D-npJo= oP8chLPEfaFV0oNYTTBA@mail.gmail.com [0] > Link: https://lore.kernel.org/all/CAHk-=3DwhWtUC-AjmGJveAETKOMeMFSTwKwu99= v7+b6AyHMmaDFA@mail.gmail.com/ > Suggested-by: Alejandro Colomar > Link: https://lore.kernel.org/all/2jxak5v6dfxlpbxhpm3ey7oup4g2lnr3ueurfbo= sf5wdo65dk4@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. Hallyn" > --- > fs/exec.c | 10 ---------- > fs/proc/array.c | 2 +- > include/linux/sched.h | 31 +++++++++++++++++++++++++------ > kernel/kthread.c | 2 +- > 4 files changed, 27 insertions(+), 18 deletions(-) >=20 > diff --git a/fs/exec.c b/fs/exec.c > index a47d0e4c54f6..2e468ddd203a 100644 > --- a/fs/exec.c > +++ b/fs/exec.c > @@ -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); This comment is correct (see other comments below). (Except that pedantically, I'd write it as NUL-terminated with a hyphen, just like zero-padded.) > - 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.c b/fs/proc/array.c > index 34a47fb0c57f..55ed3510d2bb 100644 > --- a/fs/proc/array.c > +++ b/fs/proc/array.c > @@ -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); LGTM. (This would have been good even if not removing the helper.) > =20 > if (escape) > seq_escape_str(m, tcomm, ESCAPE_SPACE | ESCAPE_SPECIAL, "\n\\"); > diff --git a/include/linux/sched.h b/include/linux/sched.h > index 33dd8d9d2b85..e0e26edbda61 100644 > --- a/include/linux/sched.h > +++ b/include/linux/sched.h > @@ -1096,9 +1096,11 @@ struct task_struct { > /* > * executable name, excluding path. > * > - * - 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 The comment above is inmprecise. It should say either "strscpy() to ensure it is always NUL-terminated", or "strscpy_pad() to ensure it is NUL-terminated and zero-padded". > + * - task_lock() to ensure the operation is atomic and the name is > + * fully updated. > */ > char comm[TASK_COMM_LEN]; > =20 > @@ -1912,10 +1914,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. For writers, locking is probably necessary, as = a race > + * condition could lead to long-term mixed results. > + * The strscpy_pad() in __set_task_comm() can ensure that the task com= m is > + * always NUL-terminated. This comment has the same imprecission that I noted above. > Therefore the race condition between reader and > + * writer is not an issue. > + * > + * - 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. Zero-padding might confuse users if it=E2=80=99= s unnecessary, > + * and not zeroing might even make it easier to expose bugs. If you ne= ed a > + * zero-padded task name, please handle that explicitly at the call si= te. > + * > + * - ARRAY_SIZE() can help ensure that @buf is indeed an array. > + */ > #define get_task_comm(buf, tsk) ({ \ > - BUILD_BUG_ON(sizeof(buf) !=3D TASK_COMM_LEN); \ > - __get_task_comm(buf, sizeof(buf), tsk); \ > + strscpy(buf, (tsk)->comm, ARRAY_SIZE(buf)); \ > + buf; \ > }) > =20 > #ifdef CONFIG_SMP > diff --git a/kernel/kthread.c b/kernel/kthread.c > index f7be976ff88a..7d001d033cf9 100644 > --- a/kernel/kthread.c > +++ b/kernel/kthread.c > @@ -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); > return; > } Other than that, LGTM. --=20 --62fk5rgm4hdf5yga Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAABCgAdFiEE6jqH8KTroDDkXfJAnowa+77/2zIFAma5wkAACgkQnowa+77/ 2zIerBAAjn3HL/5KRo0RJMvfZHqb7XVmLQlWw/O9qz5vv2ZOa7fTVN0WbAscF9HY bTJGyIikJsrsKoQpLI+ySjhJmRUS0LNg3bKxgVgYPUjt/tl3zWPd9TZ+o7dTQ3Dn +tPCRs0xF316BxTbnT66L6dFjckXQMJcM1wlUsBZqbIfdfiFq9wzbnVOCxmmn2Z3 gvDvmDg7M9E0D5dQCjow6VQNKuBe/HUT7SY9nWZllYnRZH9hRlYQ/GW7SHFSiV75 LnHWi81xvQ8F0c2hf3DSZGRSunrLTir7TRIfHL/8OpP2ckiqdsEbrgDl6jRz9f6w 6a2M9MfaQSQ0l0X+Wnr5gk0fdGoyePm1Xmx/3yrTzV/4CGNEqvcgHvkmqbz30HbF 6AoDwXK0JZdA20ANRSayUuNtv7T+ZJ3xZQbe2CTyNMGD4kaF4J1V359CZMUEx2D1 mgSypDxkGUYRDf91dN2jYcsDDtP3YbysfiTxVNViwDRgXyFVIyo8FIo8P0+q6QRG n5ebmcAD2CIqE/MKcuEr4DHoa+HD08eIYPC29VP/XoUTuq0fVSJehf3zloCtrRUh TtVlWEAeDKzZ6xkglyYH6cgMKAkM+s/v+ctXw6SgF9SJfMBXTTn33VTsqj86JGOO w/kuIw8T7OCnuVKRRRho3C7ciI5Wu7Whf8cOv/l125fp2OJq9NM= =AhLH -----END PGP SIGNATURE----- --62fk5rgm4hdf5yga--