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 mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 99000C433FE for ; Sat, 13 Nov 2021 15:48:24 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 26F3761156 for ; Sat, 13 Nov 2021 15:48:24 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 26F3761156 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=kvack.org Received: by kanga.kvack.org (Postfix) id 7CDD26B0075; Sat, 13 Nov 2021 10:48:23 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 77D616B0078; Sat, 13 Nov 2021 10:48:23 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 6475F6B007B; Sat, 13 Nov 2021 10:48:23 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0201.hostedemail.com [216.40.44.201]) by kanga.kvack.org (Postfix) with ESMTP id 56B946B0075 for ; Sat, 13 Nov 2021 10:48:23 -0500 (EST) Received: from smtpin23.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay04.hostedemail.com (Postfix) with ESMTP id EF7E4811E2 for ; Sat, 13 Nov 2021 15:48:22 +0000 (UTC) X-FDA: 78804338844.23.64B4555 Received: from mail-qv1-f53.google.com (mail-qv1-f53.google.com [209.85.219.53]) by imf04.hostedemail.com (Postfix) with ESMTP id 2B6BE50000BC for ; Sat, 13 Nov 2021 15:48:11 +0000 (UTC) Received: by mail-qv1-f53.google.com with SMTP id s9so8283161qvk.12 for ; Sat, 13 Nov 2021 07:48:22 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=oXbQA6YsK/Yb8I1ufwTowmeWehU5ZLa6WqLwd/WJnR0=; b=ImBzKERFqdjBGrGwyXdvLPxokVVH/YOD6syxy5Ry7EoZ7JA1ZgRpH36/Ir2IBUSx8W gS0HmCEFLrPn8WeyKPwEHIKMJ8MsoQKXk07euxX0IcdQLNiTUxjGZQqX9d7aI435mJq7 QePMM1Q6vqImAExonnt61+8iV7HOUhqKsJvwCo+s5DSG0p8dfVub4pnX3y3g9rWeXJGP s9Mi2AmGn/L6seJC8NjdI8dnnTh/v07eZIEcb9LSYEY+5wguwcQdBUGlxdKg1fR7SASX PlYXoa7CPenBQheitAgVSHtWNxpkfdc6wMn6uumIY9JQJs7Eb1xfWp0PpdG4DXYQNu/x ADAw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=oXbQA6YsK/Yb8I1ufwTowmeWehU5ZLa6WqLwd/WJnR0=; b=xV41Dt3pQjXJyVHj/c+DTb/ViNBXfSjlS5lNHVMM16bMWvCT6fkdOGAqFVntua2oL3 nlP6qx7hwD9VcVpcLMeHXnijG/KHnza2ykOzm7WOB93VIY2tr4PJ2nicVnu0qVue31Gi SEsD/UyjXFDmdb+B7dOpelXIgNMd+1c0WhBvNlA2sxEVh4b5KtTu3fnVQDqfHK3OcqAs MFVD49+7GKXU6egqWIp87aPVfrdEg1WV8yCmNIV1b7CduPuFJigueKoyR6mIZnZjYWxQ QMq2x4UABQsgvKdhwDUEaReVA6N32Qk5AKCxybFdEsQRgPUV/bTTKiSgDSX+cJtltvC9 pOtw== X-Gm-Message-State: AOAM531KL+LFuni6ie/EWiz0wq5m93J9NNvxr1tdJ+g15cpr355B3pQc IjU759yfpAXhZb4GW/KuhQLIEuTbtmrYp3+TkVo= X-Google-Smtp-Source: ABdhPJwHPNIdUDZB1WofY0EyhE09P++WglgHkH4uOkSjWwuZbEIP1MOmjqdsr8nWmrB6p2F41NZVSXMsSppxTlGYQ68= X-Received: by 2002:a05:6214:f2d:: with SMTP id iw13mr23317601qvb.13.1636818501919; Sat, 13 Nov 2021 07:48:21 -0800 (PST) MIME-Version: 1.0 References: <20211108084142.4692-1-laoar.shao@gmail.com> In-Reply-To: From: Yafang Shao Date: Sat, 13 Nov 2021 23:47:45 +0800 Message-ID: Subject: Re: [PATCH] kthread: dynamically allocate memory to store kthread's full name To: Petr Mladek Cc: Andrew Morton , netdev , bpf , "linux-perf-use." , linux-fsdevel@vger.kernel.org, Linux MM , LKML , kernel test robot , kbuild test robot , Steven Rostedt , Mathieu Desnoyers , Arnaldo Carvalho de Melo , Alexei Starovoitov , Andrii Nakryiko , Michal Miroslaw , Peter Zijlstra , Matthew Wilcox , David Hildenbrand , Al Viro , Kees Cook Content-Type: text/plain; charset="UTF-8" X-Rspamd-Server: rspam05 X-Rspamd-Queue-Id: 2B6BE50000BC X-Stat-Signature: foe931esr9pgdexj36kntbrsbu65nsds Authentication-Results: imf04.hostedemail.com; dkim=pass header.d=gmail.com header.s=20210112 header.b=ImBzKERF; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf04.hostedemail.com: domain of laoar.shao@gmail.com designates 209.85.219.53 as permitted sender) smtp.mailfrom=laoar.shao@gmail.com X-HE-Tag: 1636818491-39694 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: On Fri, Nov 12, 2021 at 11:34 PM Petr Mladek wrote: > > On Mon 2021-11-08 08:41:42, Yafang Shao wrote: > > When I was implementing a new per-cpu kthread cfs_migration, I found the > > comm of it "cfs_migration/%u" is truncated due to the limitation of > > TASK_COMM_LEN. For example, the comm of the percpu thread on CPU10~19 are > > all with the same name "cfs_migration/1", which will confuse the user. This > > issue is not critical, because we can get the corresponding CPU from the > > task's Cpus_allowed. But for kthreads correspoinding to other hardware > > devices, it is not easy to get the detailed device info from task comm, > > for example, > > > > After this change, the full name of these truncated kthreads will be > > displayed via /proc/[pid]/comm: > > > > --- a/fs/proc/array.c > > +++ b/fs/proc/array.c > > @@ -102,6 +103,8 @@ void proc_task_name(struct seq_file *m, struct task_struct *p, bool escape) > > > > if (p->flags & PF_WQ_WORKER) > > wq_worker_comm(tcomm, sizeof(tcomm), p); > > Just for record. I though that this patch obsoleted wq_worker_comm() > but it did not. wq_worker_comm() returns different values > depending on the last proceed work item and has to stay. > Right. worker comm is changed dynamically, which is combined by (task_comm+worker_desc) or (task_comm-worker_desc). I planned to remove the whole worker->desc and set it dynamically to the new kthread full_name but I found it may not be a good idea. > > + else if (p->flags & PF_KTHREAD) > > + get_kthread_comm(tcomm, sizeof(tcomm), p); > > else > > __get_task_comm(tcomm, sizeof(tcomm), p); > > > > --- a/kernel/kthread.c > > +++ b/kernel/kthread.c > > @@ -121,6 +135,7 @@ void free_kthread_struct(struct task_struct *k) > > Hmm, there is the following comment: > > /* > * Can be NULL if this kthread was created by kernel_thread() > * or if kmalloc() in kthread() failed. > */ > kthread = to_kthread(k); > > And indeed, set_kthread_struct() is called only by kthread() > and init_idle(). > > For example, call_usermodehelper_exec_sync() calls kernel_thread() > but given @fn does not call set_kthread_struct(). Also init_idle() > continues even when the allocation failed. > Yes, it really can be NULL. > > > #ifdef CONFIG_BLK_CGROUP > > WARN_ON_ONCE(kthread && kthread->blkcg_css); > > #endif > > + kfree(kthread->full_name); > > Hence, we have to make sure that it is not NULL here. I suggest > something like: > Agreed. I will do it. > void free_kthread_struct(struct task_struct *k) > { > struct kthread *kthread; > > /* > * Can be NULL if this kthread was created by kernel_thread() > * or if kmalloc() in kthread() failed. > */ > kthread = to_kthread(k); > if (!kthread) > return; > > #ifdef CONFIG_BLK_CGROUP > WARN_ON_ONCE(kthread->blkcg_css); > #endif > kfree(kthread->full_name); > kfree(kthread); > } > > > Side note: The possible NULL pointer looks dangerous to > me. to_kthread() is dereferenced without any check on > several locations. > > For example, kthread_create_on_cpu() looks safe. It is a kthread > crated by kthread(). It will exists only when the allocation > succeeded. > > kthread_stop() is probably safe only because it used only for > the classic kthreads created by kthread(). But the API > is not safe. > > kthread_use_mm() is probably used only by classic kthreads as > well. But it is less clear to me. > > All this unsafe APIs looks like a ticking bomb to me. But > it is beyond this patchset. > I will analyze it in depth and try to dismantle this ticking bomb. -- Thanks Yafang