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 423CBC77B7C for ; Thu, 29 Aug 2024 06:30:58 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 75E4E8D0008; Thu, 29 Aug 2024 02:30:57 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 70D8E8D0002; Thu, 29 Aug 2024 02:30:57 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 588138D0008; Thu, 29 Aug 2024 02:30:57 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0015.hostedemail.com [216.40.44.15]) by kanga.kvack.org (Postfix) with ESMTP id 3794C8D0002 for ; Thu, 29 Aug 2024 02:30:57 -0400 (EDT) Received: from smtpin14.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id D4593120ABC for ; Thu, 29 Aug 2024 06:30:56 +0000 (UTC) X-FDA: 82504310112.14.6ECD28C Received: from mail-qv1-f41.google.com (mail-qv1-f41.google.com [209.85.219.41]) by imf07.hostedemail.com (Postfix) with ESMTP id 1A5F54001D for ; Thu, 29 Aug 2024 06:30:53 +0000 (UTC) Authentication-Results: imf07.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=Q8mkzL30; spf=pass (imf07.hostedemail.com: domain of laoar.shao@gmail.com designates 209.85.219.41 as permitted sender) smtp.mailfrom=laoar.shao@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1724912966; 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=DVHRuwAUcEYymH+J/b0R0e+3iNvUH3y6v7Jow7lKvQY=; b=aJTj9BIOGyfeEjrzqjXTniE7NA1nY6leppymXRHWTVbm9QZLAIbAdFNqLlZMJYF/RNnTeK lppwUQ8IsLsyOJxXqqzWXekscuqD/kdt4YUL1Xv07ow/yYQQriZ7VHNIId8TOl/y6tt+2x RqUN8Zjp9EHKTJMT6xBUyAbV/YVmDlU= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1724912966; a=rsa-sha256; cv=none; b=2ulo4Q4xUw5Rd13bvAQlEKTpTGw9RV/4WifrCWh0bgBS7bYzB7u5Gj/wJtOHJd33wNj5kp 7sejOqPK9Jq7XZ+HSLk7Hskqc04X0QHxeihXhRv9aDMAVbus72kSrLi3Ab4KzDr2JRXsGm 4ilVEyeoObCMnI5xcrqG4dYtn3XHUGY= ARC-Authentication-Results: i=1; imf07.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=Q8mkzL30; spf=pass (imf07.hostedemail.com: domain of laoar.shao@gmail.com designates 209.85.219.41 as permitted sender) smtp.mailfrom=laoar.shao@gmail.com; dmarc=pass (policy=none) header.from=gmail.com Received: by mail-qv1-f41.google.com with SMTP id 6a1803df08f44-6bf84c3d043so1797486d6.3 for ; Wed, 28 Aug 2024 23:30:53 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1724913053; x=1725517853; darn=kvack.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=DVHRuwAUcEYymH+J/b0R0e+3iNvUH3y6v7Jow7lKvQY=; b=Q8mkzL30mTu+ALJd7IG2r+QCuUzzak5FjGYIWarBmrmqyAlJpx5nliHhchg+9yFiH1 ZuNJ3O+r2zWaHrBDCrjstfr2MhiaLU6cpqoye0nBVL8jcaDnUQqYSeO12HepNVzqtBpL WKbzuZ7IiMdsh7s6rQDHes1gafIqGBYtCfze1aJ5NKz+ZezvSZ/pHZhEBzJtwL3lWMDI 1ZHWa/ANn0dAWHWcczX0rhnIjVbd5yoRbECuf+d98Yp9Cxtxytfy6ghU9Pm776tWJNmJ H/7nus74KTb2TQ8ty/OC0BIC+C4ltGViJlrKG8iu+EH4mScMSfpRymoqd4Z/X3jHKUjL TQ5g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1724913053; x=1725517853; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=DVHRuwAUcEYymH+J/b0R0e+3iNvUH3y6v7Jow7lKvQY=; b=OE4+GkZtL2iS+XNDxaLkrIqWyFkSN+rLduVe4fk02uUr3IJYwdX3WVKDWLzpu2Wx7G JF9laPIM0ByYlUJyQl6XbWijZQHmzWISVSudHEcAd3T8fq9fg/CDX+BQp5xGG2OBA70k i4RAOyJLCns1XfVEU8wQBQrcvNZmdrTqnA+DTWiZDBD3jsvp2aaETWaK9rJmxFridWSo 5syP2wBeLJvabGDajgnwMTnHlwBSDTiFIsKsGPP+TC1c+sjWNdK+rg8/prKgw6VLQRjy OX0Hv3Hh+3b8UZhYRZ6+jcv21YiGn2LQP2+Gb9eNX1N/jXQAJ2e3xEefEfbWYjStdVCh 3Xuw== X-Forwarded-Encrypted: i=1; AJvYcCVKTk3x0qmQgh3kQ97tSGBRaUJoOUBdXjrywAJUP5nmnlFn6gRdGFFMHjhRQDcqU060ODbyc6L1Fw==@kvack.org X-Gm-Message-State: AOJu0YwQ8smtSlZZiQvCTJ8eoa4cwfF1he4eejt4EbBdfF0V2vbVhd2Z qyFZf1FGbBn/Zd68ZjpO+sdN6FgIKdz0fQNVERpCQaKLvKU9mnlAMdZ35SloB+mVYP1/wwJLTwL 7lBc+DTnRgDScxzovgXCbbO/Oxjw= X-Google-Smtp-Source: AGHT+IHAaYj3VKJPFC+rsCIttyDdBV4rNcUvYcw6ZzQZduvgmQVvDq94rvCCqajdqQxNsCbTeYDYWfFYA4N5Ml43Gtc= X-Received: by 2002:a05:6214:2d4a:b0:6bb:ab4a:dbdb with SMTP id 6a1803df08f44-6c33e5e7de8mr21034396d6.1.1724913053063; Wed, 28 Aug 2024 23:30:53 -0700 (PDT) MIME-Version: 1.0 References: <20240828030321.20688-1-laoar.shao@gmail.com> <20240828030321.20688-2-laoar.shao@gmail.com> <8A36564D-56E3-469B-B201-0BD7C11D6EFC@kernel.org> In-Reply-To: <8A36564D-56E3-469B-B201-0BD7C11D6EFC@kernel.org> From: Yafang Shao Date: Thu, 29 Aug 2024 14:30:14 +0800 Message-ID: Subject: Re: [PATCH v8 1/8] Get rid of __get_task_comm() To: Kees Cook Cc: akpm@linux-foundation.org, 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" Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Queue-Id: 1A5F54001D X-Stat-Signature: tqm4ed8598rhx98nphwcthpmc9ty6aya X-Rspamd-Server: rspam09 X-Rspam-User: X-HE-Tag: 1724913053-351972 X-HE-Meta: U2FsdGVkX1++JqOoFb1+HaRuCb5n5wPpl//EdCMZkI9gLYGbNWwszA5faGP36JBjSL3QVW+Ijphh5mJdMRDHPCikMZPHGuYB7eSgNKVelq6z9MOGvsQK9Nd3VJvPMmRlVP+bArfRVqQRkuVFi9nX1KCFe9XSDqRblV/FJS5lWJmJWGfWrwviFzrdC2k/Vv11G06jcH1HeiRaNXPRxF1aLtGdN++WrZMVL2bu68yNbWo+WDNta06S2G7yiReA56jxoa7xdBrbDyoGHmR6+poUh7TtO0uU2nZNPSTh7OJdQ+n53BEi3dqi83PmpgzTDCegh+g7hl8l2tFC+EE+9akdyP/eHd521gtpv6v/R7smpoHn+kIHq8ifKjt8u99jV190nK0Pap44Fh8He94fWWwjKqLglPHJ/UVzsgpW8xqfsDTscVYftco/gNwOFK0jMlNOwEAUIKYTsIqoQ9efn/Cs3psssmIsawYQfBMYnoqHuibWpRovgmYUo64EiCOWt76zwk+7MJIiebEnakFOkBFNEO15JiwsiVzq7cSrXGTySR53QLjecZXe1Ny2CmH4MC4jpMylAilwBjL+x+pUDFlX2cH+kiZaI81sYhJUIfOqmwbi/ZPNcTSrqB87Zb2B96vKqrEBb2zq58U2C4lscSS5Tpvp+1htk6gG9SJdMfXFzbBxHW1rof7uG5cu5fTX3zbp1+tMl/UMbmAwuo8pVLn83ZIkDveMX3c+TWXCK4wPXSKEijxPRMWwWN+UgsYPvoG5ggHV6+paawfVoKkfhwMtcehlUBDazh13kiQEgqNjU276JdfvjqbgVHw+aWHnPh3VXtIxb5VYv3tcydqcyyaj29JZU1YFpVztsBo0keTEa4Rh0QBBJDIRH8u7LZ4HUzecb3oVbSUm/hFsf8GvM3xKd99t77ASwsDWTIv5UhlVgI9TlCsWTz2EyPqzYb7gkNfYpj6WnE7ZTTL2v2gDK3s pHx4FSfa 7m5l1DkBoDzSJG0AuG+3llh+9sbmk9teKkgHQ7qrusm8bt3RqUQrK8Amk1rDhVLVktMVv8JFv590H8IW62FGB9GEAiJJn6RcmNdy5JA+Y06KtcbNu+VmrRHS39zL49Ff8MQZALGZ3OTMxIx1S0vlIJBTCTUXQM8qBtcR0ibRZcrXnTM11uP0VGZ8PKFu2HiXdlzRARAddlRdv+dvmpJnouAQ7toN4F0zG7cwjAI3IrlirLf/TCwc/o3Qroog6evshOpM2mNmLu5WC/fcGYz9tDt3J2B2AjwVfNMIwmLfKOXfHOxXSkzWWDicM7iZDhx1lAh//Ra3x9HIsHfVHbOAdFitybtzSnTTRXaqF8Gh8jala5MXl7BrAvy21ClOG4WlwZ+sSiJryjvTBe+TSEmAR+tkodc1eAuUjQhAjTfzu4eyCkYlqI7NO6Sei2tVcq2ArsHugCD7m4ICg6CnsgSKc7arQ3kW2z9y98JedvhBP6UOiG95l1DZTEJomfsUQ3ACSb1NP4F5f4lssTGjX0kRjBtBeGzPLKqRyXHywKl+42dJSOgubZC04ra6P2ZpmTSQ4Uoo316NFsXbaXzhOR14d6QNmpHPCjGW9UZ89RV7Sm03Mi0TH95hYrvQDncGlpHXBt5rWYSynqeul/UHwrWoLNKS4eyLLIfHKE3HIvYVTn+khze0poU3+KjDV2BSnDXCju29XCuS5u+E0CdTi8FhKHggluhda/c8k3xmYXRLqWRg6jclcBv+TSYF1upMKi3GIvuK4p3m8i9Ix9GNy6o9Tn9WguvSbkIXMlhkiggnCAASVriW1nGUNL9+xMg== 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 Wed, Aug 28, 2024 at 10:04=E2=80=AFPM Kees Cook wrote: > > > > On August 27, 2024 8:03:14 PM PDT, Yafang Shao wro= te: > >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 locki= ng > > : was always wrong for readers (for writers it probably does make sens= e > > : 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 val= id > > array. > > Sorry, that's not a correct evaluation. See below. > > > > >- 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 sit= e. > > This is also not an appropriate rationale. We don't make the kernel "more= buggy" not purpose. ;) See below. > > > > >Suggested-by: Linus Torvalds > >Link: https://lore.kernel.org/all/CAHk-=3DwivfrF0_zvf+oj6=3D=3DSh=3D-npJ= ooP8chLPEfaFV0oNYTTBA@mail.gmail.com [0] > >Link: https://lore.kernel.org/all/CAHk-=3DwhWtUC-AjmGJveAETKOMeMFSTwKwu9= 9v7+b6AyHMmaDFA@mail.gmail.com/ > >Suggested-by: Alejandro Colomar > >Link: https://lore.kernel.org/all/2jxak5v6dfxlpbxhpm3ey7oup4g2lnr3ueurfb= osf5wdo65dk4@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 | 32 ++++++++++++++++++++++++++------ > > kernel/kthread.c | 2 +- > > 4 files changed, 28 insertions(+), 18 deletions(-) > > > >diff --git a/fs/exec.c b/fs/exec.c > >index 50e76cc633c4..8a23171bc3c3 100644 > >--- a/fs/exec.c > >+++ b/fs/exec.c > >@@ -1264,16 +1264,6 @@ static int unshare_sighand(struct task_struct *me= ) > > return 0; > > } > > > >-char *__get_task_comm(char *buf, size_t buf_size, struct task_struct *t= sk) > >-{ > >- 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 exec= utable > > * 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_= struct *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); > > > > 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 f8d150343d42..c40b95a79d80 100644 > >--- a/include/linux/sched.h > >+++ b/include/linux/sched.h > >@@ -1096,9 +1096,12 @@ 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 and > >+ * zero-padded > >+ * - task_lock() to ensure the operation is atomic and the name= is > >+ * fully updated. > > */ > > char comm[TASK_COMM_LEN]; > > > >@@ -1914,10 +1917,27 @@ static inline void set_task_comm(struct task_str= uct *tsk, const char *from) > > __set_task_comm(tsk, from, false); > > } > > > >-extern char *__get_task_comm(char *to, size_t len, struct task_struct *= tsk); > >+/* > >+ * - Why not use task_lock()? > >+ * User space can randomly change their names anyway, so locking for = readers > >+ * 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 co= mm is > >+ * always NUL-terminated and zero-padded. Therefore the race conditio= n between > >+ * reader and writer is not an issue. > >+ * > >+ * - Why not use strscpy_pad()? > >+ * While strscpy_pad() prevents writing garbage past the NUL terminat= or, which > >+ * is useful when using the task name as a key in a hash map, most us= e cases > >+ * don't require this. Zero-padding might confuse users if it=E2=80= =99s unnecessary, > >+ * and not zeroing might even make it easier to expose bugs. If you n= eed a > >+ * zero-padded task name, please handle that explicitly at the call s= ite. > > I really don't like this part of the change. You don't know that existing= callers don't depend on the padding. Please invert this logic: get_task_co= mm() must use strscpy_pad(). Calls NOT wanting padding can call strscpy() t= hemselves. > > >+ * > >+ * - ARRAY_SIZE() can help ensure that @buf is indeed an array. > > This doesn't need checking here; strscpy() will already do that. > > >+ */ > > #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 con= tinue to be the correct size: current callers do not perform any return val= ue analysis, so they cannot accidentally start having situations where the = destination string might be truncated. Again, anyone wanting to avoid that = restriction can use strscpy() directly and check the return value. Hello Kees, Thanks for your input. Alejandro has addressed all the other changes except for the removal of BUILD_BUG_ON(). I have a question regarding this: if we're using it to avoid truncation, why not write it like this? BUILD_BUG_ON(sizeof(buf) < TASK_COMM_LEN); This way, it ensures that the size is at least as large as TASK_COMM_LEN. -- Regards Yafang