On Mon, Oct 25, 2021 at 08:33:14AM +0000, 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,
>
> jbd2/nvme0n1p2-
> xfs-reclaim/sdf
>
> We can also shorten the name to work around this problem, but I find
> there are so many truncated kthreads:
>
> rcu_tasks_kthre
> rcu_tasks_rude_
> rcu_tasks_trace
> poll_mpt3sas0_s
> ext4-rsv-conver
> xfs-reclaim/sd{a, b, c, ...}
> xfs-blockgc/sd{a, b, c, ...}
> xfs-inodegc/sd{a, b, c, ...}
> audit_send_repl
> ecryptfs-kthrea
> vfio-irqfd-clea
> jbd2/nvme0n1p2-
> ...
>
> We should improve this problem fundamentally by extending comm size to
> 24 bytes. task_struct is growing rather regularly by 8 bytes.
>
> After this change, the truncated kthreads listed above will be
> displayed as:
>
> rcu_tasks_kthread
> rcu_tasks_rude_kthread
> rcu_tasks_trace_kthread
> poll_mpt3sas0_statu
> ext4-rsv-conversion
> xfs-reclaim/sdf1
> xfs-blockgc/sdf1
> xfs-inodegc/sdf1
> audit_send_reply
> ecryptfs-kthread
> vfio-irqfd-cleanup
> jbd2/nvme0n1p2-8
>
> As we have converted all the unsafe copy of task comm to the safe one,
> this change won't make any trouble to the kernel or the in-tree tools.
> The safe one and unsafe one of comm copy as follows,
>
> Unsafe Safe
> strlcpy strscpy_pad
> strncpy strscpy_pad
> bpf_probe_read_kernel bpf_probe_read_kernel_str
> bpf_core_read_str
> bpf_get_current_comm
> perf_event__prepare_comm
> prctl(2)
>
> Regarding the possible risk it may take to the out-of-tree user tools, if
> the user tools get the task comm through kernel API like prctl(2),
> bpf_get_current_comm() and etc, the tools still work well after this
> change. While If the user tools get the task comm through direct string
> copy, it must make sure the copied string should be with a nul terminator.
>
> Signed-off-by: Yafang Shao <laoar.shao@xxxxxxxxx>
> Cc: Mathieu Desnoyers <mathieu.desnoyers@xxxxxxxxxxxx>
> Cc: Arnaldo Carvalho de Melo <arnaldo.melo@xxxxxxxxx>
> Cc: Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx>
> Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> Cc: Steven Rostedt <rostedt@xxxxxxxxxxx>
> Cc: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
> Cc: Kees Cook <keescook@xxxxxxxxxxxx>
> Cc: Petr Mladek <pmladek@xxxxxxxx>
> ---
> include/linux/sched.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 124538db792c..490d12eabe44 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -279,7 +279,7 @@ struct task_group;
> * BPF programs.
> */
> enum {
> - TASK_COMM_LEN = 16,
> + TASK_COMM_LEN = 24,
> };
I suspect this should be kept in sync with the tools/ copy of sched.h
(i.e. we may need to keep the TASK_COMM_LEN_16 around in the kernel tree
too.)
>
> extern void scheduler_tick(void);
> --
> 2.17.1
>
--
Kees Cook
