[PATCH v3 0/3] Add support for long task name

[PATCH v3 0/3] Add support for long task name

[Bhupesh]

Changes since v2:
================
- v2 can be seen here: https://lore.kernel.org/lkml/20250331121820.455916-1-bhupesh@igalia.com/
- As suggested by Yafang and Kees, picked Linus' suggested approach for
  this version (see: <https://lore.kernel.org/all/CAHk-=wjAmmHUg6vho1KjzQi2=psR30+CogFd4aXrThr2gsiS4g@mail.gmail.com/>).
- Dropped kthreads patch from this version. It would be sent out
  separately, if we have a consensus on this approach.

Changes since v1:
================
- v1 can be seen here: https://lore.kernel.org/lkml/20250314052715.610377-1-bhupesh@igalia.com/
- As suggested by Kees, added [PATCH 3/3] to have a consistent
  'full_name' entry inside 'task_struct' which both tasks and
  kthreads can use.
- Fixed the commit message to indicate that the existing ABI
  '/proc/$pid/task/$tid/comm' remains untouched and a parallel
  '/proc/$pid/task/$tid/full_name' ABI for new (interested) users.

While working with user-space debugging tools which work especially
on linux gaming platforms, I found that the task name is truncated due
to the limitation of TASK_COMM_LEN.

Now, during debug tracing, seeing truncated names is not very useful,
especially on gaming platforms where the number of tasks running can
be very high.

This patch does not touch 'TASK_COMM_LEN' at all, i.e.
'TASK_COMM_LEN' and the 16-byte design remains untouched.

In this version, as Linus suggested, we can add the
following union inside 'task_struct':
       union {
               char    comm[TASK_COMM_LEN];
               char    real_comm[REAL_TASK_COMM_LEN];
       };

and then modify '__set_task_comm()' to pass 'tsk->real_comm'
to the existing users.

So, eventually:
- users who want the existing 'TASK_COMM_LEN' behavior will get it
  (existing ABIs would continue to work),
- users who just print out 'tsk->comm' as a string will get the longer
  new "real comm",
- users who do 'sizeof(->comm)' will continue to get the old value
  because of the union.

After this change, gdb is able to show full name of the task, using a
simple app which generates threads with long names [see 1]:
  # gdb ./threadnames -ex "run info thread" -ex "detach" -ex "quit" > log
  # cat log

  NameThatIsTooLongForComm[4662]

[1]. https://github.com/lostgoat/tasknames

Bhupesh (3):
  exec: Remove obsolete comments
  treewide: Switch memcpy() users of 'task->comm' to a more safer
    implementation
  exec: Add support for 64 byte 'tsk->real_comm'

 fs/exec.c                      |  6 +++---
 include/linux/coredump.h       |  3 ++-
 include/linux/sched.h          | 14 ++++++++------
 include/trace/events/block.h   |  5 +++++
 include/trace/events/oom.h     |  1 +
 include/trace/events/osnoise.h |  1 +
 include/trace/events/sched.h   | 13 +++++++++++++
 include/trace/events/signal.h  |  1 +
 include/trace/events/task.h    |  2 ++
 9 files changed, 36 insertions(+), 10 deletions(-)

-- 
2.38.1

[PATCH v3 1/3] exec: Remove obsolete comments

[Bhupesh]

Patch 3a3f61ce5e0b ("exec: Make sure task->comm is always NUL-terminated"),
replaced 'strscpy_pad()' with 'memcpy()' implementations inside
'__set_task_comm()'.

However a few left-over comments are still there, which mention
the usage of 'strscpy_pad()' inside '__set_task_comm()'.

Remove those obsolete comments.

While at it, also remove an obsolete comment regarding 'task_lock()'
usage while handing 'task->comm'.

Signed-off-by: Bhupesh <bhupesh@igalia.com>
---
 include/linux/sched.h | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index f96ac1982893..cb219c6db179 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1159,10 +1159,8 @@ struct task_struct {
 	 *
 	 * - normally initialized begin_new_exec()
 	 * - set it with set_task_comm()
-	 *   - strscpy_pad() to ensure it is always NUL-terminated and
+	 *   - logic inside set_task_comm() will ensure it is always NUL-terminated and
 	 *     zero-padded
-	 *   - task_lock() to ensure the operation is atomic and the name is
-	 *     fully updated.
 	 */
 	char				comm[TASK_COMM_LEN];
 
@@ -1997,7 +1995,7 @@ extern void __set_task_comm(struct task_struct *tsk, const char *from, bool exec
  *   User space can randomly change their names anyway, so locking for readers
  *   doesn't make sense. For writers, locking is probably necessary, as a race
  *   condition could lead to long-term mixed results.
- *   The strscpy_pad() in __set_task_comm() can ensure that the task comm is
+ *   The logic inside __set_task_comm() should ensure that the task comm is
  *   always NUL-terminated and zero-padded. Therefore the race condition between
  *   reader and writer is not an issue.
  *
-- 
2.38.1

[PATCH v3 2/3] treewide: Switch memcpy() users of 'task->comm' to a more safer implementation

[Bhupesh]

As Linus mentioned in [1], currently we have several memcpy() use-cases
which use 'current->comm' to copy the task name over to local copies.
For an example:

 ...
 char comm[TASK_COMM_LEN];
 memcpy(comm, current->comm, TASK_COMM_LEN);
 ...

These should be modified so that we can later implement approaches
to handle the task->comm's 16-byte length limitation (TASK_COMM_LEN)
is a more modular way (follow-up patches do the same):

 ...
 char comm[TASK_COMM_LEN];
 memcpy(comm, current->comm, TASK_COMM_LEN);
 comm[TASK_COMM_LEN - 1] = 0;
 ...

The relevant 'memcpy()' users were identified using the following search
pattern:
 $ git grep 'memcpy.*->comm\>'

[1]. https://lore.kernel.org/all/CAHk-=wjAmmHUg6vho1KjzQi2=psR30+CogFd4aXrThr2gsiS4g@mail.gmail.com/

Signed-off-by: Bhupesh <bhupesh@igalia.com>
---
 include/linux/coredump.h       |  3 ++-
 include/trace/events/block.h   |  5 +++++
 include/trace/events/oom.h     |  1 +
 include/trace/events/osnoise.h |  1 +
 include/trace/events/sched.h   | 13 +++++++++++++
 include/trace/events/signal.h  |  1 +
 include/trace/events/task.h    |  2 ++
 7 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/include/linux/coredump.h b/include/linux/coredump.h
index 77e6e195d1d6..058ae3f2bec8 100644
--- a/include/linux/coredump.h
+++ b/include/linux/coredump.h
@@ -53,7 +53,8 @@ extern void do_coredump(const kernel_siginfo_t *siginfo);
 	do {	\
 		char comm[TASK_COMM_LEN];	\
 		/* This will always be NUL terminated. */ \
-		memcpy(comm, current->comm, sizeof(comm)); \
+		memcpy(comm, current->comm, TASK_COMM_LEN); \
+		comm[TASK_COMM_LEN] = '\0'; \
 		printk_ratelimited(Level "coredump: %d(%*pE): " Format "\n",	\
 			task_tgid_vnr(current), (int)strlen(comm), comm, ##__VA_ARGS__);	\
 	} while (0)	\
diff --git a/include/trace/events/block.h b/include/trace/events/block.h
index bd0ea07338eb..94a941ac2034 100644
--- a/include/trace/events/block.h
+++ b/include/trace/events/block.h
@@ -214,6 +214,7 @@ DECLARE_EVENT_CLASS(block_rq,
 		blk_fill_rwbs(__entry->rwbs, rq->cmd_flags);
 		__get_str(cmd)[0] = '\0';
 		memcpy(__entry->comm, current->comm, TASK_COMM_LEN);
+		__entry->comm[TASK_COMM_LEN - 1] = '\0';
 	),
 
 	TP_printk("%d,%d %s %u (%s) %llu + %u %s,%u,%u [%s]",
@@ -352,6 +353,7 @@ DECLARE_EVENT_CLASS(block_bio,
 		__entry->nr_sector	= bio_sectors(bio);
 		blk_fill_rwbs(__entry->rwbs, bio->bi_opf);
 		memcpy(__entry->comm, current->comm, TASK_COMM_LEN);
+		__entry->comm[TASK_COMM_LEN - 1] = '\0';
 	),
 
 	TP_printk("%d,%d %s %llu + %u [%s]",
@@ -439,6 +441,7 @@ TRACE_EVENT(block_plug,
 
 	TP_fast_assign(
 		memcpy(__entry->comm, current->comm, TASK_COMM_LEN);
+		__entry->comm[TASK_COMM_LEN - 1] = '\0';
 	),
 
 	TP_printk("[%s]", __entry->comm)
@@ -458,6 +461,7 @@ DECLARE_EVENT_CLASS(block_unplug,
 	TP_fast_assign(
 		__entry->nr_rq = depth;
 		memcpy(__entry->comm, current->comm, TASK_COMM_LEN);
+		__entry->comm[TASK_COMM_LEN - 1] = '\0';
 	),
 
 	TP_printk("[%s] %d", __entry->comm, __entry->nr_rq)
@@ -509,6 +513,7 @@ TRACE_EVENT(block_split,
 		__entry->new_sector	= new_sector;
 		blk_fill_rwbs(__entry->rwbs, bio->bi_opf);
 		memcpy(__entry->comm, current->comm, TASK_COMM_LEN);
+		__entry->comm[TASK_COMM_LEN - 1] = '\0';
 	),
 
 	TP_printk("%d,%d %s %llu / %llu [%s]",
diff --git a/include/trace/events/oom.h b/include/trace/events/oom.h
index 9f0a5d1482c4..a5641ed4285f 100644
--- a/include/trace/events/oom.h
+++ b/include/trace/events/oom.h
@@ -24,6 +24,7 @@ TRACE_EVENT(oom_score_adj_update,
 	TP_fast_assign(
 		__entry->pid = task->pid;
 		memcpy(__entry->comm, task->comm, TASK_COMM_LEN);
+		__entry->comm[TASK_COMM_LEN - 1] = '\0';
 		__entry->oom_score_adj = task->signal->oom_score_adj;
 	),
 
diff --git a/include/trace/events/osnoise.h b/include/trace/events/osnoise.h
index 3f4273623801..0321b3f8d532 100644
--- a/include/trace/events/osnoise.h
+++ b/include/trace/events/osnoise.h
@@ -117,6 +117,7 @@ TRACE_EVENT(thread_noise,
 
 	TP_fast_assign(
 		memcpy(__entry->comm, t->comm, TASK_COMM_LEN);
+		__entry->comm[TASK_COMM_LEN - 1] = '\0';
 		__entry->pid = t->pid;
 		__entry->start = start;
 		__entry->duration = duration;
diff --git a/include/trace/events/sched.h b/include/trace/events/sched.h
index 8994e97d86c1..3126d44c43ee 100644
--- a/include/trace/events/sched.h
+++ b/include/trace/events/sched.h
@@ -26,6 +26,7 @@ TRACE_EVENT(sched_kthread_stop,
 
 	TP_fast_assign(
 		memcpy(__entry->comm, t->comm, TASK_COMM_LEN);
+		__entry->comm[TASK_COMM_LEN - 1] = '\0';
 		__entry->pid	= t->pid;
 	),
 
@@ -153,6 +154,7 @@ DECLARE_EVENT_CLASS(sched_wakeup_template,
 
 	TP_fast_assign(
 		memcpy(__entry->comm, p->comm, TASK_COMM_LEN);
+		__entry->comm[TASK_COMM_LEN - 1] = '\0';
 		__entry->pid		= p->pid;
 		__entry->prio		= p->prio; /* XXX SCHED_DEADLINE */
 		__entry->target_cpu	= task_cpu(p);
@@ -238,10 +240,12 @@ TRACE_EVENT(sched_switch,
 
 	TP_fast_assign(
 		memcpy(__entry->prev_comm, prev->comm, TASK_COMM_LEN);
+		__entry->comm[TASK_COMM_LEN - 1] = '\0';
 		__entry->prev_pid	= prev->pid;
 		__entry->prev_prio	= prev->prio;
 		__entry->prev_state	= __trace_sched_switch_state(preempt, prev_state, prev);
 		memcpy(__entry->next_comm, next->comm, TASK_COMM_LEN);
+		__entry->next_comm[TASK_COMM_LEN - 1] = '\0';
 		__entry->next_pid	= next->pid;
 		__entry->next_prio	= next->prio;
 		/* XXX SCHED_DEADLINE */
@@ -285,6 +289,7 @@ TRACE_EVENT(sched_migrate_task,
 
 	TP_fast_assign(
 		memcpy(__entry->comm, p->comm, TASK_COMM_LEN);
+		__entry->comm[TASK_COMM_LEN - 1] = '\0';
 		__entry->pid		= p->pid;
 		__entry->prio		= p->prio; /* XXX SCHED_DEADLINE */
 		__entry->orig_cpu	= task_cpu(p);
@@ -310,6 +315,7 @@ DECLARE_EVENT_CLASS(sched_process_template,
 
 	TP_fast_assign(
 		memcpy(__entry->comm, p->comm, TASK_COMM_LEN);
+		__entry->comm[TASK_COMM_LEN - 1] = '\0';
 		__entry->pid		= p->pid;
 		__entry->prio		= p->prio; /* XXX SCHED_DEADLINE */
 	),
@@ -356,6 +362,7 @@ TRACE_EVENT(sched_process_wait,
 
 	TP_fast_assign(
 		memcpy(__entry->comm, current->comm, TASK_COMM_LEN);
+		__entry->comm[TASK_COMM_LEN - 1] = '\0';
 		__entry->pid		= pid_nr(pid);
 		__entry->prio		= current->prio; /* XXX SCHED_DEADLINE */
 	),
@@ -382,8 +389,10 @@ TRACE_EVENT(sched_process_fork,
 
 	TP_fast_assign(
 		memcpy(__entry->parent_comm, parent->comm, TASK_COMM_LEN);
+		__entry->parent_comm[TASK_COMM_LEN - 1] = '\0';
 		__entry->parent_pid	= parent->pid;
 		memcpy(__entry->child_comm, child->comm, TASK_COMM_LEN);
+		__entry->child_comm[TASK_COMM_LEN - 1] = '\0';
 		__entry->child_pid	= child->pid;
 	),
 
@@ -480,6 +489,7 @@ DECLARE_EVENT_CLASS_SCHEDSTAT(sched_stat_template,
 
 	TP_fast_assign(
 		memcpy(__entry->comm, tsk->comm, TASK_COMM_LEN);
+		__entry->comm[TASK_COMM_LEN - 1] = '\0';
 		__entry->pid	= tsk->pid;
 		__entry->delay	= delay;
 	),
@@ -538,6 +548,7 @@ DECLARE_EVENT_CLASS(sched_stat_runtime,
 
 	TP_fast_assign(
 		memcpy(__entry->comm, tsk->comm, TASK_COMM_LEN);
+		__entry->comm[TASK_COMM_LEN - 1] = '\0';
 		__entry->pid		= tsk->pid;
 		__entry->runtime	= runtime;
 	),
@@ -570,6 +581,7 @@ TRACE_EVENT(sched_pi_setprio,
 
 	TP_fast_assign(
 		memcpy(__entry->comm, tsk->comm, TASK_COMM_LEN);
+		__entry->comm[TASK_COMM_LEN - 1] = '\0';
 		__entry->pid		= tsk->pid;
 		__entry->oldprio	= tsk->prio;
 		__entry->newprio	= pi_task ?
@@ -595,6 +607,7 @@ TRACE_EVENT(sched_process_hang,
 
 	TP_fast_assign(
 		memcpy(__entry->comm, tsk->comm, TASK_COMM_LEN);
+		__entry->comm[TASK_COMM_LEN - 1] = '\0';
 		__entry->pid = tsk->pid;
 	),
 
diff --git a/include/trace/events/signal.h b/include/trace/events/signal.h
index 1db7e4b07c01..7f490e553db5 100644
--- a/include/trace/events/signal.h
+++ b/include/trace/events/signal.h
@@ -68,6 +68,7 @@ TRACE_EVENT(signal_generate,
 		__entry->sig	= sig;
 		TP_STORE_SIGINFO(__entry, info);
 		memcpy(__entry->comm, task->comm, TASK_COMM_LEN);
+		__entry->comm[TASK_COMM_LEN - 1] = '\0';
 		__entry->pid	= task->pid;
 		__entry->group	= group;
 		__entry->result	= result;
diff --git a/include/trace/events/task.h b/include/trace/events/task.h
index af535b053033..4ddf21b69372 100644
--- a/include/trace/events/task.h
+++ b/include/trace/events/task.h
@@ -22,6 +22,7 @@ TRACE_EVENT(task_newtask,
 	TP_fast_assign(
 		__entry->pid = task->pid;
 		memcpy(__entry->comm, task->comm, TASK_COMM_LEN);
+		__entry->comm[TASK_COMM_LEN - 1] = '\0';
 		__entry->clone_flags = clone_flags;
 		__entry->oom_score_adj = task->signal->oom_score_adj;
 	),
@@ -45,6 +46,7 @@ TRACE_EVENT(task_rename,
 
 	TP_fast_assign(
 		memcpy(entry->oldcomm, task->comm, TASK_COMM_LEN);
+		entry->oldcomm[TASK_COMM_LEN - 1] = '\0';
 		strscpy(entry->newcomm, comm, TASK_COMM_LEN);
 		__entry->oom_score_adj = task->signal->oom_score_adj;
 	),
-- 
2.38.1

Re: [PATCH v3 2/3] treewide: Switch memcpy() users of 'task->comm' to a more safer implementation

[Petr Mladek]

On Wed 2025-05-07 16:34:43, Bhupesh wrote:
> As Linus mentioned in [1], currently we have several memcpy() use-cases
> which use 'current->comm' to copy the task name over to local copies.
> For an example:
> 
>  ...
>  char comm[TASK_COMM_LEN];
>  memcpy(comm, current->comm, TASK_COMM_LEN);
>  ...
> 
> These should be modified so that we can later implement approaches
> to handle the task->comm's 16-byte length limitation (TASK_COMM_LEN)
> is a more modular way (follow-up patches do the same):
> 
>  ...
>  char comm[TASK_COMM_LEN];
>  memcpy(comm, current->comm, TASK_COMM_LEN);
>  comm[TASK_COMM_LEN - 1] = 0;
>  ...
> 
> The relevant 'memcpy()' users were identified using the following search
> pattern:
>  $ git grep 'memcpy.*->comm\>'
> 
> [1]. https://lore.kernel.org/all/CAHk-=wjAmmHUg6vho1KjzQi2=psR30+CogFd4aXrThr2gsiS4g@mail.gmail.com/
> 
> --- a/include/linux/coredump.h
> +++ b/include/linux/coredump.h
> @@ -53,7 +53,8 @@ extern void do_coredump(const kernel_siginfo_t *siginfo);
>  	do {	\
>  		char comm[TASK_COMM_LEN];	\
>  		/* This will always be NUL terminated. */ \
> -		memcpy(comm, current->comm, sizeof(comm)); \
> +		memcpy(comm, current->comm, TASK_COMM_LEN); \
> +		comm[TASK_COMM_LEN] = '\0'; \

I would expect that we replace this with a helper function/macro
which would do the right thing.

Why is get_task_comm() not used here, please?

>  		printk_ratelimited(Level "coredump: %d(%*pE): " Format "\n",	\

Also the name seems to be used for printing a debug information.
I would expect that we could use the bigger buffer here and print
the "full" name. Is this planed, please?

>  			task_tgid_vnr(current), (int)strlen(comm), comm, ##__VA_ARGS__);	\
>  	} while (0)	\
> diff --git a/include/trace/events/block.h b/include/trace/events/block.h
> index bd0ea07338eb..94a941ac2034 100644
> --- a/include/trace/events/block.h
> +++ b/include/trace/events/block.h
> @@ -214,6 +214,7 @@ DECLARE_EVENT_CLASS(block_rq,
>  		blk_fill_rwbs(__entry->rwbs, rq->cmd_flags);
>  		__get_str(cmd)[0] = '\0';
>  		memcpy(__entry->comm, current->comm, TASK_COMM_LEN);
> +		__entry->comm[TASK_COMM_LEN - 1] = '\0';

Same for all other callers.

That said, I am not sure if the larger buffer is save in all situations.

>  	),

Best Regards,
Petr

Re: [PATCH v3 2/3] treewide: Switch memcpy() users of 'task->comm' to a more safer implementation

[Bhupesh Sharma]

Hi Petr,

On 5/7/25 5:59 PM, Petr Mladek wrote:
> On Wed 2025-05-07 16:34:43, Bhupesh wrote:
>> As Linus mentioned in [1], currently we have several memcpy() use-cases
>> which use 'current->comm' to copy the task name over to local copies.
>> For an example:
>>
>>   ...
>>   char comm[TASK_COMM_LEN];
>>   memcpy(comm, current->comm, TASK_COMM_LEN);
>>   ...
>>
>> These should be modified so that we can later implement approaches
>> to handle the task->comm's 16-byte length limitation (TASK_COMM_LEN)
>> is a more modular way (follow-up patches do the same):
>>
>>   ...
>>   char comm[TASK_COMM_LEN];
>>   memcpy(comm, current->comm, TASK_COMM_LEN);
>>   comm[TASK_COMM_LEN - 1] = 0;
>>   ...
>>
>> The relevant 'memcpy()' users were identified using the following search
>> pattern:
>>   $ git grep 'memcpy.*->comm\>'
>>
>> [1]. https://lore.kernel.org/all/CAHk-=wjAmmHUg6vho1KjzQi2=psR30+CogFd4aXrThr2gsiS4g@mail.gmail.com/
>>
>> --- a/include/linux/coredump.h
>> +++ b/include/linux/coredump.h
>> @@ -53,7 +53,8 @@ extern void do_coredump(const kernel_siginfo_t *siginfo);
>>   	do {	\
>>   		char comm[TASK_COMM_LEN];	\
>>   		/* This will always be NUL terminated. */ \
>> -		memcpy(comm, current->comm, sizeof(comm)); \
>> +		memcpy(comm, current->comm, TASK_COMM_LEN); \
>> +		comm[TASK_COMM_LEN] = '\0'; \
> I would expect that we replace this with a helper function/macro
> which would do the right thing.
>
> Why is get_task_comm() not used here, please?
>
>>   		printk_ratelimited(Level "coredump: %d(%*pE): " Format "\n",	\
> Also the name seems to be used for printing a debug information.
> I would expect that we could use the bigger buffer here and print
> the "full" name. Is this planed, please?
>
>>   			task_tgid_vnr(current), (int)strlen(comm), comm, ##__VA_ARGS__);	\
>>   	} while (0)	\
>> diff --git a/include/trace/events/block.h b/include/trace/events/block.h
>> index bd0ea07338eb..94a941ac2034 100644
>> --- a/include/trace/events/block.h
>> +++ b/include/trace/events/block.h
>> @@ -214,6 +214,7 @@ DECLARE_EVENT_CLASS(block_rq,
>>   		blk_fill_rwbs(__entry->rwbs, rq->cmd_flags);
>>   		__get_str(cmd)[0] = '\0';
>>   		memcpy(__entry->comm, current->comm, TASK_COMM_LEN);
>> +		__entry->comm[TASK_COMM_LEN - 1] = '\0';
> Same for all other callers.
>
> That said, I am not sure if the larger buffer is save in all situations.
>
>>   	),
>

Thanks for the review, I agree on using the helper / wrapper function to 
replace this open-coded memcpy + set last entry as '\0'.

However I see that Steven has already shared a RFC approach (see [1]), 
to use __string() instead of fixed lengths for 'task->comm' for tracing 
events.
I plan to  rebase my v4 on top of his RFC, which might mean that this 
patch would no longer be needed in the v4.

[1]. 
https://lore.kernel.org/linux-trace-kernel/20250507133458.51bafd95@gandalf.local.home/

Regards,
Bhupesh

Re: [PATCH v3 2/3] treewide: Switch memcpy() users of 'task->comm' to a more safer implementation

[Bhupesh Sharma]

On 5/8/25 1:47 PM, Bhupesh Sharma wrote:
> Hi Petr,
>
> On 5/7/25 5:59 PM, Petr Mladek wrote:
>> On Wed 2025-05-07 16:34:43, Bhupesh wrote:
>>> As Linus mentioned in [1], currently we have several memcpy() use-cases
>>> which use 'current->comm' to copy the task name over to local copies.
>>> For an example:
>>>
>>>   ...
>>>   char comm[TASK_COMM_LEN];
>>>   memcpy(comm, current->comm, TASK_COMM_LEN);
>>>   ...
>>>
>>> These should be modified so that we can later implement approaches
>>> to handle the task->comm's 16-byte length limitation (TASK_COMM_LEN)
>>> is a more modular way (follow-up patches do the same):
>>>
>>>   ...
>>>   char comm[TASK_COMM_LEN];
>>>   memcpy(comm, current->comm, TASK_COMM_LEN);
>>>   comm[TASK_COMM_LEN - 1] = 0;
>>>   ...
>>>
>>> The relevant 'memcpy()' users were identified using the following 
>>> search
>>> pattern:
>>>   $ git grep 'memcpy.*->comm\>'
>>>
>>> [1]. 
>>> https://lore.kernel.org/all/CAHk-=wjAmmHUg6vho1KjzQi2=psR30+CogFd4aXrThr2gsiS4g@mail.gmail.com/
>>>
>>> --- a/include/linux/coredump.h
>>> +++ b/include/linux/coredump.h
>>> @@ -53,7 +53,8 @@ extern void do_coredump(const kernel_siginfo_t 
>>> *siginfo);
>>>       do {    \
>>>           char comm[TASK_COMM_LEN];    \
>>>           /* This will always be NUL terminated. */ \
>>> -        memcpy(comm, current->comm, sizeof(comm)); \
>>> +        memcpy(comm, current->comm, TASK_COMM_LEN); \
>>> +        comm[TASK_COMM_LEN] = '\0'; \
>> I would expect that we replace this with a helper function/macro
>> which would do the right thing.
>>
>> Why is get_task_comm() not used here, please?
>>
>>>           printk_ratelimited(Level "coredump: %d(%*pE): " Format 
>>> "\n",    \
>> Also the name seems to be used for printing a debug information.
>> I would expect that we could use the bigger buffer here and print
>> the "full" name. Is this planed, please?
>>
>>>               task_tgid_vnr(current), (int)strlen(comm), comm, 
>>> ##__VA_ARGS__);    \
>>>       } while (0)    \
>>> diff --git a/include/trace/events/block.h 
>>> b/include/trace/events/block.h
>>> index bd0ea07338eb..94a941ac2034 100644
>>> --- a/include/trace/events/block.h
>>> +++ b/include/trace/events/block.h
>>> @@ -214,6 +214,7 @@ DECLARE_EVENT_CLASS(block_rq,
>>>           blk_fill_rwbs(__entry->rwbs, rq->cmd_flags);
>>>           __get_str(cmd)[0] = '\0';
>>>           memcpy(__entry->comm, current->comm, TASK_COMM_LEN);
>>> +        __entry->comm[TASK_COMM_LEN - 1] = '\0';
>> Same for all other callers.
>>
>> That said, I am not sure if the larger buffer is save in all situations.
>>
>>>       ),
>>
>
> Thanks for the review, I agree on using the helper / wrapper function 
> to replace this open-coded memcpy + set last entry as '\0'.
>
> However I see that Steven has already shared a RFC approach (see [1]), 
> to use __string() instead of fixed lengths for 'task->comm' for 
> tracing events.
> I plan to  rebase my v4 on top of his RFC, which might mean that this 
> patch would no longer be needed in the v4.
>
> [1]. 
> https://lore.kernel.org/linux-trace-kernel/20250507133458.51bafd95@gandalf.local.home/
>

Sorry, pressed the send button too quickly :D

I instead meant - "I plan to  rebase my v4 on top of Steven's RFC, which 
might mean that this patch would no longer need to address the trace 
events, but would still need to handle other places where tsk->comm 
directly in memcpy() and replace it with 'get_task_comm()' instead".

Thanks.

Re: [PATCH v3 2/3] treewide: Switch memcpy() users of 'task->comm' to a more safer implementation

[Steven Rostedt]

On Thu, 8 May 2025 13:52:01 +0530
Bhupesh Sharma <bhsharma@igalia.com> wrote:

> > [1]. 
> > https://lore.kernel.org/linux-trace-kernel/20250507133458.51bafd95@gandalf.local.home/
> >  
> 
> Sorry, pressed the send button too quickly :D
> 
> I instead meant - "I plan to  rebase my v4 on top of Steven's RFC, which 
> might mean that this patch would no longer need to address the trace 
> events, but would still need to handle other places where tsk->comm 
> directly in memcpy() and replace it with 'get_task_comm()' instead".

Note I didn't switch all the events, just most of the sched events.

This may affect user space that parses the raw events and may expect a hard
coded string instead of a dynamic one (as the sched_switch and sched_waking
events do).

We will need to investigate before we make these changes, which is why I
posted it as a RFC.

-- Steve

Re: [PATCH v3 2/3] treewide: Switch memcpy() users of 'task->comm' to a more safer implementation

[kernel test robot]

Hi Bhupesh,

kernel test robot noticed the following build errors:

[auto build test ERROR on trace/for-next]
[also build test ERROR on tip/sched/core akpm-mm/mm-everything linus/master v6.15-rc5 next-20250508]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Bhupesh/exec-Remove-obsolete-comments/20250507-190740
base:   https://git.kernel.org/pub/scm/linux/kernel/git/trace/linux-trace for-next
patch link:    https://lore.kernel.org/r/20250507110444.963779-3-bhupesh%40igalia.com
patch subject: [PATCH v3 2/3] treewide: Switch memcpy() users of 'task->comm' to a more safer implementation
config: x86_64-defconfig (https://download.01.org/0day-ci/archive/20250508/202505082038.A5ejhbR4-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-12) 11.3.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250508/202505082038.A5ejhbR4-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202505082038.A5ejhbR4-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from include/trace/define_trace.h:119,
                    from include/trace/events/sched.h:856,
                    from kernel/sched/core.c:84:
   include/trace/events/sched.h: In function 'do_trace_event_raw_event_sched_switch':
>> include/trace/events/sched.h:245:24: error: 'struct trace_event_raw_sched_switch' has no member named 'comm'
     245 |                 __entry->comm[TASK_COMM_LEN - 1] = '\0';
         |                        ^~
   include/trace/trace_events.h:427:11: note: in definition of macro '__DECLARE_EVENT_CLASS'
     427 |         { assign; }                                                     \
         |           ^~~~~~
   include/trace/trace_events.h:435:23: note: in expansion of macro 'PARAMS'
     435 |                       PARAMS(assign), PARAMS(print))                    \
         |                       ^~~~~~
   include/trace/trace_events.h:40:9: note: in expansion of macro 'DECLARE_EVENT_CLASS'
      40 |         DECLARE_EVENT_CLASS(name,                              \
         |         ^~~~~~~~~~~~~~~~~~~
   include/trace/trace_events.h:44:30: note: in expansion of macro 'PARAMS'
      44 |                              PARAMS(assign),                   \
         |                              ^~~~~~
   include/trace/events/sched.h:224:1: note: in expansion of macro 'TRACE_EVENT'
     224 | TRACE_EVENT(sched_switch,
         | ^~~~~~~~~~~
   include/trace/events/sched.h:243:9: note: in expansion of macro 'TP_fast_assign'
     243 |         TP_fast_assign(
         |         ^~~~~~~~~~~~~~
   In file included from include/trace/define_trace.h:120,
                    from include/trace/events/sched.h:856,
                    from kernel/sched/core.c:84:
   include/trace/events/sched.h: In function 'do_perf_trace_sched_switch':
>> include/trace/events/sched.h:245:24: error: 'struct trace_event_raw_sched_switch' has no member named 'comm'
     245 |                 __entry->comm[TASK_COMM_LEN - 1] = '\0';
         |                        ^~
   include/trace/perf.h:51:11: note: in definition of macro '__DECLARE_EVENT_CLASS'
      51 |         { assign; }                                                     \
         |           ^~~~~~
   include/trace/perf.h:67:23: note: in expansion of macro 'PARAMS'
      67 |                       PARAMS(assign), PARAMS(print))                    \
         |                       ^~~~~~
   include/trace/trace_events.h:40:9: note: in expansion of macro 'DECLARE_EVENT_CLASS'
      40 |         DECLARE_EVENT_CLASS(name,                              \
         |         ^~~~~~~~~~~~~~~~~~~
   include/trace/trace_events.h:44:30: note: in expansion of macro 'PARAMS'
      44 |                              PARAMS(assign),                   \
         |                              ^~~~~~
   include/trace/events/sched.h:224:1: note: in expansion of macro 'TRACE_EVENT'
     224 | TRACE_EVENT(sched_switch,
         | ^~~~~~~~~~~
   include/trace/events/sched.h:243:9: note: in expansion of macro 'TP_fast_assign'
     243 |         TP_fast_assign(
         |         ^~~~~~~~~~~~~~


vim +245 include/trace/events/sched.h

   225	
   226		TP_PROTO(bool preempt,
   227			 struct task_struct *prev,
   228			 struct task_struct *next,
   229			 unsigned int prev_state),
   230	
   231		TP_ARGS(preempt, prev, next, prev_state),
   232	
   233		TP_STRUCT__entry(
   234			__array(	char,	prev_comm,	TASK_COMM_LEN	)
   235			__field(	pid_t,	prev_pid			)
   236			__field(	int,	prev_prio			)
   237			__field(	long,	prev_state			)
   238			__array(	char,	next_comm,	TASK_COMM_LEN	)
   239			__field(	pid_t,	next_pid			)
   240			__field(	int,	next_prio			)
   241		),
   242	
   243		TP_fast_assign(
   244			memcpy(__entry->prev_comm, prev->comm, TASK_COMM_LEN);
 > 245			__entry->comm[TASK_COMM_LEN - 1] = '\0';
   246			__entry->prev_pid	= prev->pid;
   247			__entry->prev_prio	= prev->prio;
   248			__entry->prev_state	= __trace_sched_switch_state(preempt, prev_state, prev);
   249			memcpy(__entry->next_comm, next->comm, TASK_COMM_LEN);
   250			__entry->next_comm[TASK_COMM_LEN - 1] = '\0';
   251			__entry->next_pid	= next->pid;
   252			__entry->next_prio	= next->prio;
   253			/* XXX SCHED_DEADLINE */
   254		),
   255	
   256		TP_printk("prev_comm=%s prev_pid=%d prev_prio=%d prev_state=%s%s ==> next_comm=%s next_pid=%d next_prio=%d",
   257			__entry->prev_comm, __entry->prev_pid, __entry->prev_prio,
   258	
   259			(__entry->prev_state & (TASK_REPORT_MAX - 1)) ?
   260			  __print_flags(__entry->prev_state & (TASK_REPORT_MAX - 1), "|",
   261					{ TASK_INTERRUPTIBLE, "S" },
   262					{ TASK_UNINTERRUPTIBLE, "D" },
   263					{ __TASK_STOPPED, "T" },
   264					{ __TASK_TRACED, "t" },
   265					{ EXIT_DEAD, "X" },
   266					{ EXIT_ZOMBIE, "Z" },
   267					{ TASK_PARKED, "P" },
   268					{ TASK_DEAD, "I" }) :
   269			  "R",
   270	
   271			__entry->prev_state & TASK_REPORT_MAX ? "+" : "",
   272			__entry->next_comm, __entry->next_pid, __entry->next_prio)
   273	);
   274	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH v3 3/3] exec: Add support for 64 byte 'tsk->real_comm'

[Bhupesh]

Historically due to the 16-byte length of TASK_COMM_LEN, the
users of 'tsk->comm' are restricted to use a fixed-size target
buffer also of TASK_COMM_LEN for 'memcpy()' like use-cases.

To fix the same, Linus suggested in [1] that we can add the
following union inside 'task_struct':
       union {
               char    comm[TASK_COMM_LEN];
               char    real_comm[REAL_TASK_COMM_LEN];
       };

and then modify '__set_task_comm()' to pass 'tsk->real_comm'
to the existing users.

This would mean that:
(1) The old common pattern of just printing with '%s' and tsk->comm
    would just continue to work (as it is):

        pr_alert("BUG: Bad page state in process %s  pfn:%05lx\n",
                current->comm, page_to_pfn(page));

(2) And, the memcpy() users of 'tsk->comm' would need to be made more
    stable by ensuring that the destination buffer always has a closing
    NUL character (done already in the preceding patch in this series).

So, eventually:
- users who want the existing 'TASK_COMM_LEN' behavior will get it
  (existing ABIs would continue to work),
- users who just print out 'tsk->comm' as a string will get the longer
  new "real comm",
- users who do 'sizeof(->comm)' will continue to get the old value
  because of the union.

[1]. https://lore.kernel.org/all/CAHk-=wjAmmHUg6vho1KjzQi2=psR30+CogFd4aXrThr2gsiS4g@mail.gmail.com

Signed-off-by: Bhupesh <bhupesh@igalia.com>
---
 fs/exec.c             | 6 +++---
 include/linux/sched.h | 8 ++++++--
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index 8e4ea5f1e64c..2b2f2dacc013 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1190,11 +1190,11 @@ static int unshare_sighand(struct task_struct *me)
  */
 void __set_task_comm(struct task_struct *tsk, const char *buf, bool exec)
 {
-	size_t len = min(strlen(buf), sizeof(tsk->comm) - 1);
+	size_t len = min(strlen(buf), sizeof(tsk->real_comm) - 1);
 
 	trace_task_rename(tsk, buf);
-	memcpy(tsk->comm, buf, len);
-	memset(&tsk->comm[len], 0, sizeof(tsk->comm) - len);
+	memcpy(tsk->real_comm, buf, len);
+	memset(&tsk->real_comm[len], 0, sizeof(tsk->real_comm) - len);
 	perf_event_comm(tsk, exec);
 }
 
diff --git a/include/linux/sched.h b/include/linux/sched.h
index cb219c6db179..2744d90badf1 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -317,6 +317,7 @@ struct user_event_mm;
  */
 enum {
 	TASK_COMM_LEN = 16,
+	REAL_TASK_COMM_LEN = 64,
 };
 
 extern void sched_tick(void);
@@ -1162,7 +1163,10 @@ struct task_struct {
 	 *   - logic inside set_task_comm() will ensure it is always NUL-terminated and
 	 *     zero-padded
 	 */
-	char				comm[TASK_COMM_LEN];
+	union {
+		char			comm[TASK_COMM_LEN];
+		char			real_comm[REAL_TASK_COMM_LEN];
+	};
 
 	struct nameidata		*nameidata;
 
@@ -2005,7 +2009,7 @@ extern void __set_task_comm(struct task_struct *tsk, const char *from, bool exec
  */
 #define get_task_comm(buf, tsk) ({			\
 	BUILD_BUG_ON(sizeof(buf) < TASK_COMM_LEN);	\
-	strscpy_pad(buf, (tsk)->comm);			\
+	strscpy_pad(buf, (tsk)->real_comm);		\
 	buf;						\
 })
 
-- 
2.38.1

Re: [PATCH v3 3/3] exec: Add support for 64 byte 'tsk->real_comm'

[Petr Mladek]

On Wed 2025-05-07 16:34:44, Bhupesh wrote:
> Historically due to the 16-byte length of TASK_COMM_LEN, the
> users of 'tsk->comm' are restricted to use a fixed-size target
> buffer also of TASK_COMM_LEN for 'memcpy()' like use-cases.
> 
> To fix the same, Linus suggested in [1] that we can add the
> following union inside 'task_struct':
>        union {
>                char    comm[TASK_COMM_LEN];
>                char    real_comm[REAL_TASK_COMM_LEN];
>        };

Nit: IMHO, the prefix "real_" is misleading. The buffer size is still
      limited and the name might be shrinked. I would suggest
      something like:

	char    comm_ext[TASK_COMM_EXT_LEN];
or
	char    comm_64[TASK_COMM_64_LEN]

> and then modify '__set_task_comm()' to pass 'tsk->real_comm'
> to the existing users.

Best Regards,
Petr

Re: [PATCH v3 3/3] exec: Add support for 64 byte 'tsk->real_comm'

[Steven Rostedt]

On Wed, 7 May 2025 14:54:36 +0200
Petr Mladek <pmladek@suse.com> wrote:

> > To fix the same, Linus suggested in [1] that we can add the
> > following union inside 'task_struct':
> >        union {
> >                char    comm[TASK_COMM_LEN];
> >                char    real_comm[REAL_TASK_COMM_LEN];
> >        };  
> 
> Nit: IMHO, the prefix "real_" is misleading. The buffer size is still
>       limited and the name might be shrinked. I would suggest

Agreed.

>       something like:
> 
> 	char    comm_ext[TASK_COMM_EXT_LEN];
> or
> 	char    comm_64[TASK_COMM_64_LEN]

I prefer "comm_ext" as I don't think we want to hard code the actual size.
Who knows, in the future we may extend it again!

-- Steve

Re: [PATCH v3 3/3] exec: Add support for 64 byte 'tsk->real_comm'

[Bhupesh Sharma]

On 5/7/25 10:53 PM, Steven Rostedt wrote:
> On Wed, 7 May 2025 14:54:36 +0200
> Petr Mladek <pmladek@suse.com> wrote:
>
>>> To fix the same, Linus suggested in [1] that we can add the
>>> following union inside 'task_struct':
>>>         union {
>>>                 char    comm[TASK_COMM_LEN];
>>>                 char    real_comm[REAL_TASK_COMM_LEN];
>>>         };
>> Nit: IMHO, the prefix "real_" is misleading. The buffer size is still
>>        limited and the name might be shrinked. I would suggest
> Agreed.
>
>>        something like:
>>
>> 	char    comm_ext[TASK_COMM_EXT_LEN];
>> or
>> 	char    comm_64[TASK_COMM_64_LEN]
> I prefer "comm_ext" as I don't think we want to hard code the actual size.
> Who knows, in the future we may extend it again!
>

Ok, let me use 'comm_64' instead in v4.

Thanks for the review.

Regards,
Bhupesh

Re: [PATCH v3 3/3] exec: Add support for 64 byte 'tsk->real_comm'

[Steven Rostedt]

On Thu, 8 May 2025 13:38:11 +0530
Bhupesh Sharma <bhsharma@igalia.com> wrote:

> >>        something like:
> >>
> >> 	char    comm_ext[TASK_COMM_EXT_LEN];
> >> or
> >> 	char    comm_64[TASK_COMM_64_LEN]  
> > I prefer "comm_ext" as I don't think we want to hard code the actual size.
> > Who knows, in the future we may extend it again!
> >  
> 
> Ok, let me use 'comm_64' instead in v4.

I think you missed what I said. I prefer the comm_ext over comm_64 because
I don't think it's a good idea to hardcode the extended size in the name.
That will make it very difficult in the future if we want to make it even
larger.

-- Steve