| Message ID | 20211108083840.4627-5-laoar.shao@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | task comm cleanups | expand |
| Context | Check | Description |
|---|---|---|
| bpf/vmtest-bpf | success | VM_Test |
| bpf/vmtest-bpf-PR | success | PR summary |
| bpf/vmtest-bpf-next | success | VM_Test |
| bpf/vmtest-bpf-next-PR | success | PR summary |
| netdev/tree_selection | success | Not a local patch, async |
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? Anyhow Reviewed-by: David Hildenbrand <david@redhat.com>
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. (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)
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
On 11.11.21 12:34, Petr Mladek wrote: > 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. Yes, that's what I recall as well. The I read the patch subjects+descriptions in this series "make it adopt to task comm size change" and was slightly confused. Maybe we should just remove any notion of "task comm size change" from this series and instead just call it a cleanup.
On Thu, Nov 11, 2021 at 7:35 PM Petr Mladek <pmladek@suse.com> wrote: > > 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. > I think we can just add some comments to make it grepable, for example, + /* TASK_COMM_LEN */ char pr_fname[16]; or a more detailed explanation: + /* + * The hard-coded 16 is derived from TASK_COMM_LEN, but it can be changed as + * it is exposed to userspace. We'd better make it hard-coded here. + */ char pr_fname[16];
On Thu, Nov 11, 2021 at 7:47 PM David Hildenbrand <david@redhat.com> wrote: > > On 11.11.21 12:34, Petr Mladek wrote: > > 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. > > Yes, that's what I recall as well. The I read the patch > subjects+descriptions in this series "make it adopt to task comm size > change" and was slightly confused. > > Maybe we should just remove any notion of "task comm size change" from > this series and instead just call it a cleanup. > > Agreed that it may make a little confused for the one who didn't engage in this series at the first place. I will improve the subjection+description.
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; }
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(-)