From: Petr Mladek <pmladek@suse.com>
To: David Hildenbrand <david@redhat.com>
Cc: Yafang Shao <laoar.shao@gmail.com>,
akpm@linux-foundation.org, netdev@vger.kernel.org,
bpf@vger.kernel.org, linux-perf-users@vger.kernel.org,
linux-fsdevel@vger.kernel.org, linux-mm@kvack.org,
linux-kernel@vger.kernel.org, oliver.sang@intel.com,
lkp@intel.com, Kees Cook <keescook@chromium.org>,
Mathieu Desnoyers <mathieu.desnoyers@efficios.com>,
Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>,
Andrii Nakryiko <andrii.nakryiko@gmail.com>,
Michal Miroslaw <mirq-linux@rere.qmqm.pl>,
Peter Zijlstra <peterz@infradead.org>,
Steven Rostedt <rostedt@goodmis.org>,
Matthew Wilcox <willy@infradead.org>,
Al Viro <viro@zeniv.linux.org.uk>
Subject: Re: [PATCH 4/7] fs/binfmt_elf: use get_task_comm instead of open-coded string copy
Date: Thu, 11 Nov 2021 12:34:57 +0100 [thread overview]
Message-ID: <YYz/4bSdSXR3Palz@alley> (raw)
In-Reply-To: <b495d38d-5cdd-8a33-b9d3-de721095ccab@redhat.com>
On Thu 2021-11-11 11:06:04, David Hildenbrand wrote:
> On 11.11.21 11:03, David Hildenbrand wrote:
> > On 08.11.21 09:38, Yafang Shao wrote:
> >> It is better to use get_task_comm() instead of the open coded string
> >> copy as we do in other places.
> >>
> >> struct elf_prpsinfo is used to dump the task information in userspace
> >> coredump or kernel vmcore. Below is the verfication of vmcore,
> >>
> >> crash> ps
> >> PID PPID CPU TASK ST %MEM VSZ RSS COMM
> >> 0 0 0 ffffffff9d21a940 RU 0.0 0 0 [swapper/0]
> >>> 0 0 1 ffffa09e40f85e80 RU 0.0 0 0 [swapper/1]
> >>> 0 0 2 ffffa09e40f81f80 RU 0.0 0 0 [swapper/2]
> >>> 0 0 3 ffffa09e40f83f00 RU 0.0 0 0 [swapper/3]
> >>> 0 0 4 ffffa09e40f80000 RU 0.0 0 0 [swapper/4]
> >>> 0 0 5 ffffa09e40f89f80 RU 0.0 0 0 [swapper/5]
> >> 0 0 6 ffffa09e40f8bf00 RU 0.0 0 0 [swapper/6]
> >>> 0 0 7 ffffa09e40f88000 RU 0.0 0 0 [swapper/7]
> >>> 0 0 8 ffffa09e40f8de80 RU 0.0 0 0 [swapper/8]
> >>> 0 0 9 ffffa09e40f95e80 RU 0.0 0 0 [swapper/9]
> >>> 0 0 10 ffffa09e40f91f80 RU 0.0 0 0 [swapper/10]
> >>> 0 0 11 ffffa09e40f93f00 RU 0.0 0 0 [swapper/11]
> >>> 0 0 12 ffffa09e40f90000 RU 0.0 0 0 [swapper/12]
> >>> 0 0 13 ffffa09e40f9bf00 RU 0.0 0 0 [swapper/13]
> >>> 0 0 14 ffffa09e40f98000 RU 0.0 0 0 [swapper/14]
> >>> 0 0 15 ffffa09e40f9de80 RU 0.0 0 0 [swapper/15]
> >>
> >> It works well as expected.
> >>
> >> Suggested-by: Kees Cook <keescook@chromium.org>
> >> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> >> Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> >> Cc: Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>
> >> Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com>
> >> Cc: Michal Miroslaw <mirq-linux@rere.qmqm.pl>
> >> Cc: Peter Zijlstra <peterz@infradead.org>
> >> Cc: Steven Rostedt <rostedt@goodmis.org>
> >> Cc: Matthew Wilcox <willy@infradead.org>
> >> Cc: David Hildenbrand <david@redhat.com>
> >> Cc: Al Viro <viro@zeniv.linux.org.uk>
> >> Cc: Kees Cook <keescook@chromium.org>
> >> Cc: Petr Mladek <pmladek@suse.com>
> >> ---
> >> fs/binfmt_elf.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> >> index a813b70f594e..138956fd4a88 100644
> >> --- a/fs/binfmt_elf.c
> >> +++ b/fs/binfmt_elf.c
> >> @@ -1572,7 +1572,7 @@ static int fill_psinfo(struct elf_prpsinfo *psinfo, struct task_struct *p,
> >> SET_UID(psinfo->pr_uid, from_kuid_munged(cred->user_ns, cred->uid));
> >> SET_GID(psinfo->pr_gid, from_kgid_munged(cred->user_ns, cred->gid));
> >> rcu_read_unlock();
> >> - strncpy(psinfo->pr_fname, p->comm, sizeof(psinfo->pr_fname));
> >> + get_task_comm(psinfo->pr_fname, p);
> >>
> >> return 0;
> >> }
> >>
> >
> > We have a hard-coded "pr_fname[16]" as well, not sure if we want to
> > adjust that to use TASK_COMM_LEN?
>
> But if the intention is to chance TASK_COMM_LEN later, we might want to
> keep that unchanged.
It seems that len will not change in the end. Another solution is
going to be used for the long names, see
https://lore.kernel.org/r/20211108084142.4692-1-laoar.shao@gmail.com.
> (replacing the 16 by a define might still be a good idea, similar to how
> it's done for ELF_PRARGSZ, but just a thought)
If the code would need some tweaking when the size changes, you could
still use TASK_COMM_LEN and trigger a compilation error when the size
gets modified. For example, static_assert(TASK_COMM_LEN == 16);
It will make it clear that it needs attention if the size is ever modified.
Best Regards,
Petr
next prev parent reply other threads:[~2021-11-11 11:35 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-11-08 8:38 [PATCH 0/7] task comm cleanups Yafang Shao
2021-11-08 8:38 ` [PATCH 1/7] fs/exec: make __set_task_comm always set a nul terminated string Yafang Shao
2021-11-10 8:28 ` David Hildenbrand
2021-11-10 9:05 ` Yafang Shao
2021-11-11 9:58 ` David Hildenbrand
2021-11-10 20:17 ` Kees Cook
2021-11-08 8:38 ` [PATCH 2/7] fs/exec: make __get_task_comm always get " Yafang Shao
2021-11-11 9:59 ` David Hildenbrand
2021-11-08 8:38 ` [PATCH 3/7] drivers/infiniband: use get_task_comm instead of open-coded string copy Yafang Shao
2021-11-11 10:01 ` David Hildenbrand
2021-11-08 8:38 ` [PATCH 4/7] fs/binfmt_elf: " Yafang Shao
2021-11-11 10:03 ` David Hildenbrand
2021-11-11 10:06 ` David Hildenbrand
2021-11-11 11:34 ` Petr Mladek [this message]
2021-11-11 11:47 ` David Hildenbrand
2021-11-12 1:08 ` Yafang Shao
2021-11-12 1:03 ` Yafang Shao
2021-11-08 8:38 ` [PATCH 5/7] samples/bpf/test_overhead_kprobe_kern: make it adopt to task comm size change Yafang Shao
2021-11-08 18:20 ` Andrii Nakryiko
2021-11-11 10:07 ` David Hildenbrand
2021-11-08 8:38 ` [PATCH 6/7] tools/bpf/bpftool/skeleton: use bpf_probe_read_kernel_str to get task comm Yafang Shao
2021-11-11 10:08 ` David Hildenbrand
2021-11-08 8:38 ` [PATCH 7/7] tools/testing/selftests/bpf: make it adopt to task comm size change Yafang Shao
2021-11-11 10:11 ` David Hildenbrand
2021-11-09 18:28 ` [PATCH 0/7] task comm cleanups Kees Cook
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YYz/4bSdSXR3Palz@alley \
--to=pmladek@suse.com \
--cc=akpm@linux-foundation.org \
--cc=andrii.nakryiko@gmail.com \
--cc=arnaldo.melo@gmail.com \
--cc=bpf@vger.kernel.org \
--cc=david@redhat.com \
--cc=keescook@chromium.org \
--cc=laoar.shao@gmail.com \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=linux-perf-users@vger.kernel.org \
--cc=lkp@intel.com \
--cc=mathieu.desnoyers@efficios.com \
--cc=mirq-linux@rere.qmqm.pl \
--cc=netdev@vger.kernel.org \
--cc=oliver.sang@intel.com \
--cc=peterz@infradead.org \
--cc=rostedt@goodmis.org \
--cc=viro@zeniv.linux.org.uk \
--cc=willy@infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.