Re: [PATCH v5 4/6] sched: Extend task command name with TASK_COMM_EXT_LEN

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] blktrace binary API breakage due to increased payload size for BLK_TN_PROCESS events.
- [Medium] Audit logs expose the full 64-byte extended task name, violating the commit message's claim to cap all existing userspace APIs to 16 bytes.
- [Medium] The tracepoint `bpf_testmod_test_read` copies the task name without guaranteeing NUL termination, leading to an out-of-bounds read when printing.
- [Low] The patch series removed the `get_task_comm` macro but failed to update its callers in the Smack LSM, resulting in an implicit function declaration and a compilation error.
--

Hi,

I have some questions about this patch.

commit 4fa4b0c6a1f339946bd26a6d2c55d7f5cfb4c1f3
Author: André Almeida <andrealmeid@igalia.com>
Subject: sched: Extend task command name with TASK_COMM_EXT_LEN

The commit increases the task command name from 16 to 64 bytes. It aims to
support long names for userspace threads while capping existing userspace APIs
to TASK_COMM_LEN to avoid buffer overflows.

> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index c7cdbb3e4aa68..c41d61e9babf9 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
[ ... ]
> @@ -1187,7 +1188,7 @@ struct task_struct {
>  	 * - set it with set_task_comm() to ensure it is always
>  	 *   NUL-terminated and zero-padded
>  	 */
> -	char				comm[TASK_COMM_LEN];
> +	char				comm[TASK_COMM_EXT_LEN];
>  
>  	struct nameidata		*nameidata;

[Severity: Critical]
With the struct task_struct comm field expanding to 64 bytes, does this break
the blktrace binary ABI?

In kernel/trace/blktrace.c:trace_note_tsk():

	trace_note(bt, tsk->pid, BLK_TN_PROCESS, tsk->comm,
		   sizeof(tsk->comm), 0);

This will now write 64 bytes to the UAPI trace buffer instead of 16 bytes,
which can cause parsing errors or buffer overflows in userspace tools like
blkparse.

[Severity: Medium]
Will out-of-bounds reads happen in existing tracepoints that copy directly
from task->comm using TASK_COMM_LEN?

For example, in tools/testing/selftests/bpf/test_kmods/bpf_testmod-events.h:

	memcpy(__entry->comm, task->comm, TASK_COMM_LEN);

If a task name is 16 bytes or longer, there won't be a NUL terminator in
the first 16 bytes, which might cause TP_printk("%s") to read past the 16
bytes into adjacent trace event fields.

[ ... ]
> -#define get_task_comm(buf, tsk) ({			\
> -	BUILD_BUG_ON(sizeof(buf) < TASK_COMM_LEN);	\
> -	strscpy_pad(buf, (tsk)->comm);			\
> -	buf;						\
> -})

[Severity: Low]
Does removing get_task_comm() break the build if CONFIG_SECURITY_SMACK=y?
Callers in security/smack/smack_access.c:smack_log() and 
security/smack/smack_lsm.c:smk_bu_current() still seem to use this macro.

> diff --git a/kernel/audit.c b/kernel/audit.c
> index 9412af9144bc9..75bcc0d43c558 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -1667,7 +1667,8 @@ static void audit_log_multicast(int group, const char *op, int err)
>  	audit_put_tty(tty);
>  	audit_log_task_context(ab); /* subj= */
>  	audit_log_format(ab, " comm=");
> -	audit_log_untrustedstring(ab, get_task_comm(comm, current));
> +	strscpy_pad(comm, current->comm);
> +	audit_log_untrustedstring(ab, comm);

[Severity: Medium]
Since comm is declared as char comm[sizeof(current->comm)] at the top of
this function, and current->comm is now 64 bytes, does this expose the full
64-byte task name in the audit log?

The commit message mentions capping all existing userspace APIs to
TASK_COMM_LEN.

This same pattern appears in kernel/audit.c:audit_log_task_info(),
kernel/auditsc.c:audit_log_task(), and
security/lsm_audit.c:dump_common_audit_data().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260827-tonyk-long_name-v5-0-5fa843782a00@igalia.com?part=4