| Message ID | 20211120112738.45980-5-laoar.shao@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | task comm cleanups | expand |
| Context | Check | Description |
|---|---|---|
| bpf/vmtest-bpf-next | pending | VM_Test |
| bpf/vmtest-bpf-next-PR | pending | PR summary |
| bpf/vmtest-bpf | fail | VM_Test |
| bpf/vmtest-bpf-PR | fail | PR summary |
| netdev/tree_selection | success | Not a local patch, async |
On Sat, 20 Nov 2021 11:27:35 +0000 Yafang Shao <laoar.shao@gmail.com> wrote: > diff --git a/include/linux/elfcore-compat.h b/include/linux/elfcore-compat.h > index e272c3d452ce..54feb64e9b5d 100644 > --- a/include/linux/elfcore-compat.h > +++ b/include/linux/elfcore-compat.h > @@ -43,6 +43,11 @@ struct compat_elf_prpsinfo > __compat_uid_t pr_uid; > __compat_gid_t pr_gid; > compat_pid_t pr_pid, pr_ppid, pr_pgrp, pr_sid; > + /* > + * The hard-coded 16 is derived from TASK_COMM_LEN, but it can't be > + * changed as it is exposed to userspace. We'd better make it hard-coded > + * here. Didn't I once suggest having a macro called something like: TASK_COMM_LEN_16 ? https://lore.kernel.org/all/20211014221409.5da58a42@oasis.local.home/ -- Steve > + */ > char pr_fname[16]; > char pr_psargs[ELF_PRARGSZ]; > }; > diff --git a/include/linux/elfcore.h b/include/linux/elfcore.h > index 957ebec35aad..746e081879a5 100644 > --- a/include/linux/elfcore.h > +++ b/include/linux/elfcore.h > @@ -65,6 +65,11 @@ struct elf_prpsinfo > __kernel_gid_t pr_gid; > pid_t pr_pid, pr_ppid, pr_pgrp, pr_sid; > /* Lots missing */ > + /* > + * The hard-coded 16 is derived from TASK_COMM_LEN, but it can't be > + * changed as it is exposed to userspace. We'd better make it hard-coded > + * here. > + */ > char pr_fname[16]; /* filename of executable */ > char pr_psargs[ELF_PRARGSZ]; /* initial part of arg list */ > };
On Tue, Nov 30, 2021 at 12:01 AM Steven Rostedt <rostedt@goodmis.org> wrote: > > On Sat, 20 Nov 2021 11:27:35 +0000 > Yafang Shao <laoar.shao@gmail.com> wrote: > > > diff --git a/include/linux/elfcore-compat.h b/include/linux/elfcore-compat.h > > index e272c3d452ce..54feb64e9b5d 100644 > > --- a/include/linux/elfcore-compat.h > > +++ b/include/linux/elfcore-compat.h > > @@ -43,6 +43,11 @@ struct compat_elf_prpsinfo > > __compat_uid_t pr_uid; > > __compat_gid_t pr_gid; > > compat_pid_t pr_pid, pr_ppid, pr_pgrp, pr_sid; > > + /* > > + * The hard-coded 16 is derived from TASK_COMM_LEN, but it can't be > > + * changed as it is exposed to userspace. We'd better make it hard-coded > > + * here. > > Didn't I once suggest having a macro called something like: > > TASK_COMM_LEN_16 ? > > > https://lore.kernel.org/all/20211014221409.5da58a42@oasis.local.home/ Hi Steven, TASK_COMM_LEN_16 is a good idea, but not all hard-coded 16 can be replaced by this macro (which is defined in include/sched.h). For example, the comm[16] in include/uapi/linux/cn_proc.h can't be replaced as it is a uapi header which can't include linux/sched.h. That's why I prefer to keep the hard-coded 16 as-is. There are three options, - option 1 comment on all the hard-coded 16 to explain why it is hard-coded - option 2 replace the hard-coded 16 that can be replaced and comment on the others which can't be replaced. - option 3 replace the hard-coded 16 that can be replaced and specifically define TASK_COMM_LEN_16 in other files which can't include linux/sched.h. Which one do you prefer ? > > > > + */ > > char pr_fname[16]; > > char pr_psargs[ELF_PRARGSZ]; > > }; > > diff --git a/include/linux/elfcore.h b/include/linux/elfcore.h > > index 957ebec35aad..746e081879a5 100644 > > --- a/include/linux/elfcore.h > > +++ b/include/linux/elfcore.h > > @@ -65,6 +65,11 @@ struct elf_prpsinfo > > __kernel_gid_t pr_gid; > > pid_t pr_pid, pr_ppid, pr_pgrp, pr_sid; > > /* Lots missing */ > > + /* > > + * The hard-coded 16 is derived from TASK_COMM_LEN, but it can't be > > + * changed as it is exposed to userspace. We'd better make it hard-coded > > + * here. > > + */ > > char pr_fname[16]; /* filename of executable */ > > char pr_psargs[ELF_PRARGSZ]; /* initial part of arg list */ > > };
On Tue, 30 Nov 2021 11:01:27 +0800 Yafang Shao <laoar.shao@gmail.com> wrote: > There are three options, > - option 1 > comment on all the hard-coded 16 to explain why it is hard-coded > - option 2 > replace the hard-coded 16 that can be replaced and comment on the > others which can't be replaced. > - option 3 > replace the hard-coded 16 that can be replaced and specifically > define TASK_COMM_LEN_16 in other files which can't include > linux/sched.h. > > Which one do you prefer ? > Option 3. Since TASK_COMM_LEN_16 is, by it's name, already hard coded to 16, it doesn't really matter if you define it in more than one location. Or we could define it in another header that include/sched.h can include. The idea of having TASK_COMM_LEN_16 is to easily grep for it, and also know exactly what it is used for when people see it being used. -- Steve
On Tue, Nov 30, 2021 at 10:22 PM Steven Rostedt <rostedt@goodmis.org> wrote: > > On Tue, 30 Nov 2021 11:01:27 +0800 > Yafang Shao <laoar.shao@gmail.com> wrote: > > > There are three options, > > - option 1 > > comment on all the hard-coded 16 to explain why it is hard-coded > > - option 2 > > replace the hard-coded 16 that can be replaced and comment on the > > others which can't be replaced. > > - option 3 > > replace the hard-coded 16 that can be replaced and specifically > > define TASK_COMM_LEN_16 in other files which can't include > > linux/sched.h. > > > > Which one do you prefer ? > > > > Option 3. Since TASK_COMM_LEN_16 is, by it's name, already hard coded to > 16, it doesn't really matter if you define it in more than one location. > > Or we could define it in another header that include/sched.h can include. > > The idea of having TASK_COMM_LEN_16 is to easily grep for it, and also know > exactly what it is used for when people see it being used. > I will send a separate patch (or patchset) to replace all the old hard-coded 16 with TASK_COMM_LEN_16 based on the -mm tree. -- Thanks Yafang
diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c index f8c7f26f1fbb..b9a33cc34d6b 100644 --- a/fs/binfmt_elf.c +++ b/fs/binfmt_elf.c @@ -1585,7 +1585,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; } diff --git a/include/linux/elfcore-compat.h b/include/linux/elfcore-compat.h index e272c3d452ce..54feb64e9b5d 100644 --- a/include/linux/elfcore-compat.h +++ b/include/linux/elfcore-compat.h @@ -43,6 +43,11 @@ struct compat_elf_prpsinfo __compat_uid_t pr_uid; __compat_gid_t pr_gid; compat_pid_t pr_pid, pr_ppid, pr_pgrp, pr_sid; + /* + * The hard-coded 16 is derived from TASK_COMM_LEN, but it can't be + * changed as it is exposed to userspace. We'd better make it hard-coded + * here. + */ char pr_fname[16]; char pr_psargs[ELF_PRARGSZ]; }; diff --git a/include/linux/elfcore.h b/include/linux/elfcore.h index 957ebec35aad..746e081879a5 100644 --- a/include/linux/elfcore.h +++ b/include/linux/elfcore.h @@ -65,6 +65,11 @@ struct elf_prpsinfo __kernel_gid_t pr_gid; pid_t pr_pid, pr_ppid, pr_pgrp, pr_sid; /* Lots missing */ + /* + * The hard-coded 16 is derived from TASK_COMM_LEN, but it can't be + * changed as it is exposed to userspace. We'd better make it hard-coded + * here. + */ char pr_fname[16]; /* filename of executable */ char pr_psargs[ELF_PRARGSZ]; /* initial part of arg list */ };
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 verification 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. Some comments are added to explain why we use the hard-coded 16. Suggested-by: Kees Cook <keescook@chromium.org> Signed-off-by: Yafang Shao <laoar.shao@gmail.com> Reviewed-by: David Hildenbrand <david@redhat.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 +- include/linux/elfcore-compat.h | 5 +++++ include/linux/elfcore.h | 5 +++++ 3 files changed, 11 insertions(+), 1 deletion(-)