This patchset is part of the patchset "extend task comm from 16 to 24"[1].
Now we have different opinion that dynamically allocates memory to store
kthread's long name into a separate pointer, so I decide to take the useful
cleanups apart from the original patchset and send it separately[2].
These useful cleanups can make the usage around task comm less
error-prone. Furthermore, it will be useful if we want to extend task
comm in the future.
All of the patches except patch #4 have either a reviewed-by or a
acked-by now. I have verfied that the vmcore/crash works well after
patch #4.
[1]. https://lore.kernel.org/lkml/[email protected]/
[2]. https://lore.kernel.org/lkml/CALOAHbAx55AUo3bm8ZepZSZnw7A08cvKPdPyNTf=E_tPqmw5hw@mail.gmail.com/
Cc: Mathieu Desnoyers <[email protected]>
Cc: Arnaldo Carvalho de Melo <[email protected]>
Cc: Alexei Starovoitov <[email protected]>
Cc: Andrii Nakryiko <[email protected]>
Cc: Michal Miroslaw <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: Matthew Wilcox <[email protected]>
Cc: David Hildenbrand <[email protected]>
Cc: Al Viro <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: Petr Mladek <[email protected]>
Yafang Shao (7):
fs/exec: make __set_task_comm always set a nul terminated string
fs/exec: make __get_task_comm always get a nul terminated string
drivers/infiniband: use get_task_comm instead of open-coded string
copy
fs/binfmt_elf: use get_task_comm instead of open-coded string copy
samples/bpf/test_overhead_kprobe_kern: make it adopt to task comm size
change
tools/bpf/bpftool/skeleton: use bpf_probe_read_kernel_str to get task
comm
tools/testing/selftests/bpf: make it adopt to task comm size change
drivers/infiniband/hw/qib/qib.h | 2 +-
drivers/infiniband/hw/qib/qib_file_ops.c | 2 +-
fs/binfmt_elf.c | 2 +-
fs/exec.c | 5 +++--
include/linux/sched.h | 9 +++++++--
samples/bpf/offwaketime_kern.c | 4 ++--
samples/bpf/test_overhead_kprobe_kern.c | 11 ++++++-----
samples/bpf/test_overhead_tp_kern.c | 5 +++--
tools/bpf/bpftool/skeleton/pid_iter.bpf.c | 4 ++--
.../testing/selftests/bpf/progs/test_stacktrace_map.c | 6 +++---
tools/testing/selftests/bpf/progs/test_tracepoint.c | 6 +++---
11 files changed, 32 insertions(+), 24 deletions(-)
--
2.17.1
bpf_probe_read_kernel_str() will add a nul terminator to the dst, then
we don't care about if the dst size is big enough. This patch also
replaces the hard-coded 16 with TASK_COMM_LEN to make it adopt to task
comm size change.
Signed-off-by: Yafang Shao <[email protected]>
Reviewed-by: Kees Cook <[email protected]>
Cc: Mathieu Desnoyers <[email protected]>
Cc: Arnaldo Carvalho de Melo <[email protected]>
Cc: Alexei Starovoitov <[email protected]>
Cc: Andrii Nakryiko <[email protected]>
Cc: Michal Miroslaw <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: Matthew Wilcox <[email protected]>
Cc: David Hildenbrand <[email protected]>
Cc: Al Viro <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: Petr Mladek <[email protected]>
---
samples/bpf/offwaketime_kern.c | 4 ++--
samples/bpf/test_overhead_kprobe_kern.c | 11 ++++++-----
samples/bpf/test_overhead_tp_kern.c | 5 +++--
3 files changed, 11 insertions(+), 9 deletions(-)
diff --git a/samples/bpf/offwaketime_kern.c b/samples/bpf/offwaketime_kern.c
index 4866afd054da..eb4d94742e6b 100644
--- a/samples/bpf/offwaketime_kern.c
+++ b/samples/bpf/offwaketime_kern.c
@@ -113,11 +113,11 @@ static inline int update_counts(void *ctx, u32 pid, u64 delta)
/* taken from /sys/kernel/debug/tracing/events/sched/sched_switch/format */
struct sched_switch_args {
unsigned long long pad;
- char prev_comm[16];
+ char prev_comm[TASK_COMM_LEN];
int prev_pid;
int prev_prio;
long long prev_state;
- char next_comm[16];
+ char next_comm[TASK_COMM_LEN];
int next_pid;
int next_prio;
};
diff --git a/samples/bpf/test_overhead_kprobe_kern.c b/samples/bpf/test_overhead_kprobe_kern.c
index f6d593e47037..8fdd2c9c56b2 100644
--- a/samples/bpf/test_overhead_kprobe_kern.c
+++ b/samples/bpf/test_overhead_kprobe_kern.c
@@ -6,6 +6,7 @@
*/
#include <linux/version.h>
#include <linux/ptrace.h>
+#include <linux/sched.h>
#include <uapi/linux/bpf.h>
#include <bpf/bpf_helpers.h>
#include <bpf/bpf_tracing.h>
@@ -22,17 +23,17 @@ int prog(struct pt_regs *ctx)
{
struct signal_struct *signal;
struct task_struct *tsk;
- char oldcomm[16] = {};
- char newcomm[16] = {};
+ char oldcomm[TASK_COMM_LEN] = {};
+ char newcomm[TASK_COMM_LEN] = {};
u16 oom_score_adj;
u32 pid;
tsk = (void *)PT_REGS_PARM1(ctx);
pid = _(tsk->pid);
- bpf_probe_read_kernel(oldcomm, sizeof(oldcomm), &tsk->comm);
- bpf_probe_read_kernel(newcomm, sizeof(newcomm),
- (void *)PT_REGS_PARM2(ctx));
+ bpf_probe_read_kernel_str(oldcomm, sizeof(oldcomm), &tsk->comm);
+ bpf_probe_read_kernel_str(newcomm, sizeof(newcomm),
+ (void *)PT_REGS_PARM2(ctx));
signal = _(tsk->signal);
oom_score_adj = _(signal->oom_score_adj);
return 0;
diff --git a/samples/bpf/test_overhead_tp_kern.c b/samples/bpf/test_overhead_tp_kern.c
index eaa32693f8fc..80edadacb692 100644
--- a/samples/bpf/test_overhead_tp_kern.c
+++ b/samples/bpf/test_overhead_tp_kern.c
@@ -4,6 +4,7 @@
* modify it under the terms of version 2 of the GNU General Public
* License as published by the Free Software Foundation.
*/
+#include <linux/sched.h>
#include <uapi/linux/bpf.h>
#include <bpf/bpf_helpers.h>
@@ -11,8 +12,8 @@
struct task_rename {
__u64 pad;
__u32 pid;
- char oldcomm[16];
- char newcomm[16];
+ char oldcomm[TASK_COMM_LEN];
+ char newcomm[TASK_COMM_LEN];
__u16 oom_score_adj;
};
SEC("tracepoint/task/task_rename")
--
2.17.1
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 <[email protected]>
Signed-off-by: Yafang Shao <[email protected]>
Cc: Mathieu Desnoyers <[email protected]>
Cc: Arnaldo Carvalho de Melo <[email protected]>
Cc: Andrii Nakryiko <[email protected]>
Cc: Michal Miroslaw <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: Matthew Wilcox <[email protected]>
Cc: David Hildenbrand <[email protected]>
Cc: Al Viro <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: Petr Mladek <[email protected]>
---
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;
}
--
2.17.1
bpf_probe_read_kernel_str() will add a nul terminator to the dst, then
we don't care about if the dst size is big enough.
Signed-off-by: Yafang Shao <[email protected]>
Acked-by: Andrii Nakryiko <[email protected]>
Cc: Mathieu Desnoyers <[email protected]>
Cc: Arnaldo Carvalho de Melo <[email protected]>
Cc: Alexei Starovoitov <[email protected]>
Cc: Andrii Nakryiko <[email protected]>
Cc: Michal Miroslaw <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: Matthew Wilcox <[email protected]>
Cc: David Hildenbrand <[email protected]>
Cc: Al Viro <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: Petr Mladek <[email protected]>
---
tools/bpf/bpftool/skeleton/pid_iter.bpf.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/bpf/bpftool/skeleton/pid_iter.bpf.c b/tools/bpf/bpftool/skeleton/pid_iter.bpf.c
index d9b420972934..f70702fcb224 100644
--- a/tools/bpf/bpftool/skeleton/pid_iter.bpf.c
+++ b/tools/bpf/bpftool/skeleton/pid_iter.bpf.c
@@ -71,8 +71,8 @@ int iter(struct bpf_iter__task_file *ctx)
e.pid = task->tgid;
e.id = get_obj_id(file->private_data, obj_type);
- bpf_probe_read_kernel(&e.comm, sizeof(e.comm),
- task->group_leader->comm);
+ bpf_probe_read_kernel_str(&e.comm, sizeof(e.comm),
+ task->group_leader->comm);
bpf_seq_write(ctx->meta->seq, &e, sizeof(e));
return 0;
--
2.17.1
On Mon, Nov 8, 2021 at 12:39 AM Yafang Shao <[email protected]> wrote:
>
> bpf_probe_read_kernel_str() will add a nul terminator to the dst, then
> we don't care about if the dst size is big enough. This patch also
> replaces the hard-coded 16 with TASK_COMM_LEN to make it adopt to task
> comm size change.
>
> Signed-off-by: Yafang Shao <[email protected]>
> Reviewed-by: Kees Cook <[email protected]>
> Cc: Mathieu Desnoyers <[email protected]>
> Cc: Arnaldo Carvalho de Melo <[email protected]>
> Cc: Alexei Starovoitov <[email protected]>
> Cc: Andrii Nakryiko <[email protected]>
> Cc: Michal Miroslaw <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> Cc: Steven Rostedt <[email protected]>
> Cc: Matthew Wilcox <[email protected]>
> Cc: David Hildenbrand <[email protected]>
> Cc: Al Viro <[email protected]>
> Cc: Kees Cook <[email protected]>
> Cc: Petr Mladek <[email protected]>
> ---
LGTM.
Acked-by: Andrii Nakryiko <[email protected]>
> samples/bpf/offwaketime_kern.c | 4 ++--
> samples/bpf/test_overhead_kprobe_kern.c | 11 ++++++-----
> samples/bpf/test_overhead_tp_kern.c | 5 +++--
> 3 files changed, 11 insertions(+), 9 deletions(-)
>
[...]
On Mon, Nov 08, 2021 at 08:38:33AM +0000, Yafang Shao wrote:
> This patchset is part of the patchset "extend task comm from 16 to 24"[1].
> Now we have different opinion that dynamically allocates memory to store
> kthread's long name into a separate pointer, so I decide to take the useful
> cleanups apart from the original patchset and send it separately[2].
>
> These useful cleanups can make the usage around task comm less
> error-prone. Furthermore, it will be useful if we want to extend task
> comm in the future.
>
> All of the patches except patch #4 have either a reviewed-by or a
> acked-by now. I have verfied that the vmcore/crash works well after
> patch #4.
>
> [1]. https://lore.kernel.org/lkml/[email protected]/
> [2]. https://lore.kernel.org/lkml/CALOAHbAx55AUo3bm8ZepZSZnw7A08cvKPdPyNTf=E_tPqmw5hw@mail.gmail.com/
Thanks for collecting this! It all looks good to me.
Andrew, can you take these?
-Kees
--
Kees Cook
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 <[email protected]>
> Signed-off-by: Yafang Shao <[email protected]>
> Cc: Mathieu Desnoyers <[email protected]>
> Cc: Arnaldo Carvalho de Melo <[email protected]>
> Cc: Andrii Nakryiko <[email protected]>
> Cc: Michal Miroslaw <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> Cc: Steven Rostedt <[email protected]>
> Cc: Matthew Wilcox <[email protected]>
> Cc: David Hildenbrand <[email protected]>
> Cc: Al Viro <[email protected]>
> Cc: Kees Cook <[email protected]>
> Cc: Petr Mladek <[email protected]>
> ---
> 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 <[email protected]>
--
Thanks,
David / dhildenb
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 <[email protected]>
>> Signed-off-by: Yafang Shao <[email protected]>
>> Cc: Mathieu Desnoyers <[email protected]>
>> Cc: Arnaldo Carvalho de Melo <[email protected]>
>> Cc: Andrii Nakryiko <[email protected]>
>> Cc: Michal Miroslaw <[email protected]>
>> Cc: Peter Zijlstra <[email protected]>
>> Cc: Steven Rostedt <[email protected]>
>> Cc: Matthew Wilcox <[email protected]>
>> Cc: David Hildenbrand <[email protected]>
>> Cc: Al Viro <[email protected]>
>> Cc: Kees Cook <[email protected]>
>> Cc: Petr Mladek <[email protected]>
>> ---
>> 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)
--
Thanks,
David / dhildenb
On 08.11.21 09:38, Yafang Shao wrote:
> bpf_probe_read_kernel_str() will add a nul terminator to the dst, then
> we don't care about if the dst size is big enough. This patch also
> replaces the hard-coded 16 with TASK_COMM_LEN to make it adopt to task
> comm size change.
>
> Signed-off-by: Yafang Shao <[email protected]>
> Reviewed-by: Kees Cook <[email protected]>
> Cc: Mathieu Desnoyers <[email protected]>
> Cc: Arnaldo Carvalho de Melo <[email protected]>
> Cc: Alexei Starovoitov <[email protected]>
> Cc: Andrii Nakryiko <[email protected]>
> Cc: Michal Miroslaw <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> Cc: Steven Rostedt <[email protected]>
> Cc: Matthew Wilcox <[email protected]>
> Cc: David Hildenbrand <[email protected]>
> Cc: Al Viro <[email protected]>
> Cc: Kees Cook <[email protected]>
> Cc: Petr Mladek <[email protected]>
> ---
> samples/bpf/offwaketime_kern.c | 4 ++--
> samples/bpf/test_overhead_kprobe_kern.c | 11 ++++++-----
> samples/bpf/test_overhead_tp_kern.c | 5 +++--
> 3 files changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/samples/bpf/offwaketime_kern.c b/samples/bpf/offwaketime_kern.c
> index 4866afd054da..eb4d94742e6b 100644
> --- a/samples/bpf/offwaketime_kern.c
> +++ b/samples/bpf/offwaketime_kern.c
> @@ -113,11 +113,11 @@ static inline int update_counts(void *ctx, u32 pid, u64 delta)
> /* taken from /sys/kernel/debug/tracing/events/sched/sched_switch/format */
> struct sched_switch_args {
> unsigned long long pad;
> - char prev_comm[16];
> + char prev_comm[TASK_COMM_LEN];
> int prev_pid;
> int prev_prio;
> long long prev_state;
> - char next_comm[16];
> + char next_comm[TASK_COMM_LEN];
> int next_pid;
> int next_prio;
> };
> diff --git a/samples/bpf/test_overhead_kprobe_kern.c b/samples/bpf/test_overhead_kprobe_kern.c
> index f6d593e47037..8fdd2c9c56b2 100644
> --- a/samples/bpf/test_overhead_kprobe_kern.c
> +++ b/samples/bpf/test_overhead_kprobe_kern.c
> @@ -6,6 +6,7 @@
> */
> #include <linux/version.h>
> #include <linux/ptrace.h>
> +#include <linux/sched.h>
> #include <uapi/linux/bpf.h>
> #include <bpf/bpf_helpers.h>
> #include <bpf/bpf_tracing.h>
> @@ -22,17 +23,17 @@ int prog(struct pt_regs *ctx)
> {
> struct signal_struct *signal;
> struct task_struct *tsk;
> - char oldcomm[16] = {};
> - char newcomm[16] = {};
> + char oldcomm[TASK_COMM_LEN] = {};
> + char newcomm[TASK_COMM_LEN] = {};
> u16 oom_score_adj;
> u32 pid;
>
> tsk = (void *)PT_REGS_PARM1(ctx);
>
> pid = _(tsk->pid);
> - bpf_probe_read_kernel(oldcomm, sizeof(oldcomm), &tsk->comm);
> - bpf_probe_read_kernel(newcomm, sizeof(newcomm),
> - (void *)PT_REGS_PARM2(ctx));
> + bpf_probe_read_kernel_str(oldcomm, sizeof(oldcomm), &tsk->comm);
> + bpf_probe_read_kernel_str(newcomm, sizeof(newcomm),
> + (void *)PT_REGS_PARM2(ctx));
It's a shame we have to do a manual copy here ...
Changes LGTM
Reviewed-by: David Hildenbrand <[email protected]>
--
Thanks,
David / dhildenb
On 08.11.21 09:38, Yafang Shao wrote:
> bpf_probe_read_kernel_str() will add a nul terminator to the dst, then
> we don't care about if the dst size is big enough.
>
> Signed-off-by: Yafang Shao <[email protected]>
> Acked-by: Andrii Nakryiko <[email protected]>
> Cc: Mathieu Desnoyers <[email protected]>
> Cc: Arnaldo Carvalho de Melo <[email protected]>
> Cc: Alexei Starovoitov <[email protected]>
> Cc: Andrii Nakryiko <[email protected]>
> Cc: Michal Miroslaw <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> Cc: Steven Rostedt <[email protected]>
> Cc: Matthew Wilcox <[email protected]>
> Cc: David Hildenbrand <[email protected]>
> Cc: Al Viro <[email protected]>
> Cc: Kees Cook <[email protected]>
> Cc: Petr Mladek <[email protected]>
Reviewed-by: David Hildenbrand <[email protected]>
--
Thanks,
David / dhildenb
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 <[email protected]>
> >> Signed-off-by: Yafang Shao <[email protected]>
> >> Cc: Mathieu Desnoyers <[email protected]>
> >> Cc: Arnaldo Carvalho de Melo <[email protected]>
> >> Cc: Andrii Nakryiko <[email protected]>
> >> Cc: Michal Miroslaw <[email protected]>
> >> Cc: Peter Zijlstra <[email protected]>
> >> Cc: Steven Rostedt <[email protected]>
> >> Cc: Matthew Wilcox <[email protected]>
> >> Cc: David Hildenbrand <[email protected]>
> >> Cc: Al Viro <[email protected]>
> >> Cc: Kees Cook <[email protected]>
> >> Cc: Petr Mladek <[email protected]>
> >> ---
> >> 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/[email protected].
> (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 <[email protected]>
>>>> Signed-off-by: Yafang Shao <[email protected]>
>>>> Cc: Mathieu Desnoyers <[email protected]>
>>>> Cc: Arnaldo Carvalho de Melo <[email protected]>
>>>> Cc: Andrii Nakryiko <[email protected]>
>>>> Cc: Michal Miroslaw <[email protected]>
>>>> Cc: Peter Zijlstra <[email protected]>
>>>> Cc: Steven Rostedt <[email protected]>
>>>> Cc: Matthew Wilcox <[email protected]>
>>>> Cc: David Hildenbrand <[email protected]>
>>>> Cc: Al Viro <[email protected]>
>>>> Cc: Kees Cook <[email protected]>
>>>> Cc: Petr Mladek <[email protected]>
>>>> ---
>>>> 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/[email protected].
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.
--
Thanks,
David / dhildenb
On Thu, Nov 11, 2021 at 7:35 PM Petr Mladek <[email protected]> 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 <[email protected]>
> > >> Signed-off-by: Yafang Shao <[email protected]>
> > >> Cc: Mathieu Desnoyers <[email protected]>
> > >> Cc: Arnaldo Carvalho de Melo <[email protected]>
> > >> Cc: Andrii Nakryiko <[email protected]>
> > >> Cc: Michal Miroslaw <[email protected]>
> > >> Cc: Peter Zijlstra <[email protected]>
> > >> Cc: Steven Rostedt <[email protected]>
> > >> Cc: Matthew Wilcox <[email protected]>
> > >> Cc: David Hildenbrand <[email protected]>
> > >> Cc: Al Viro <[email protected]>
> > >> Cc: Kees Cook <[email protected]>
> > >> Cc: Petr Mladek <[email protected]>
> > >> ---
> > >> 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/[email protected].
>
> > (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];
--
Thanks
Yafang
On Thu, Nov 11, 2021 at 7:47 PM David Hildenbrand <[email protected]> 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 <[email protected]>
> >>>> Signed-off-by: Yafang Shao <[email protected]>
> >>>> Cc: Mathieu Desnoyers <[email protected]>
> >>>> Cc: Arnaldo Carvalho de Melo <[email protected]>
> >>>> Cc: Andrii Nakryiko <[email protected]>
> >>>> Cc: Michal Miroslaw <[email protected]>
> >>>> Cc: Peter Zijlstra <[email protected]>
> >>>> Cc: Steven Rostedt <[email protected]>
> >>>> Cc: Matthew Wilcox <[email protected]>
> >>>> Cc: David Hildenbrand <[email protected]>
> >>>> Cc: Al Viro <[email protected]>
> >>>> Cc: Kees Cook <[email protected]>
> >>>> Cc: Petr Mladek <[email protected]>
> >>>> ---
> >>>> 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/[email protected].
>
> 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.
--
Thanks
Yafang