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

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

[Bhupesh]

Changes since v3:
================
- v3 can be seen here: https://lore.kernel.org/lkml/20250507110444.963779-1-bhupesh@igalia.com/
- As suggested by Petr and Steven, used 'comm_ext' name instead of
  'real_comm'. Correspondingly the macro name is changed to 'TASK_COMM_EXT_LEN'
  for the 64-byte extended comm.
- Rebased this patchset on linux-next/master, which contain the following patch from
  Steven now:
       155fd6c3e2f0 ("tracing/sched: Use __string() instead of fixed lengths for task->comm")
- Accordingly, v4 drops the changes done for 'trace/sched' events in v3,
  but retains the 'safe' memcpy' changes for other kernel trace users.

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 patchset does not touch 'TASK_COMM_LEN' at all, i.e.
'TASK_COMM_LEN' and the 16-byte design remains untouched.

Via this patchset, as Linus suggested, we can add the
following union inside 'task_struct':
       union {
               char    comm[TASK_COMM_LEN];
               char    comm_ext[TASK_COMM_EXT_LEN];
       };

and then modify '__set_task_comm()' to pass 'tsk->comm_ext'
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 "extended 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->comm_ext'

 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/signal.h  |  1 +
 include/trace/events/task.h    |  2 ++
 8 files changed, 23 insertions(+), 10 deletions(-)

-- 
2.38.1

[PATCH v4 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 8c60a42f9d00..704222114dcc 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1162,10 +1162,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 v4 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/signal.h  | 1 +
 include/trace/events/task.h    | 2 ++
 6 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/include/linux/coredump.h b/include/linux/coredump.h
index 76e41805b92d..468abc308c24 100644
--- a/include/linux/coredump.h
+++ b/include/linux/coredump.h
@@ -54,7 +54,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 ad36e73b8579..11aa0b58176d 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]",
@@ -424,6 +426,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)
@@ -443,6 +446,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)
@@ -494,6 +498,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/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

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

[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    comm_ext[TASK_COMM_EXT_LEN];
       };

and then modify '__set_task_comm()' to pass 'tsk->comm_ext'
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 "extended 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 1f5fdd2e096e..3b39fbfc8fe4 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1077,11 +1077,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->comm_ext) - 1);
 
 	trace_task_rename(tsk, buf);
-	memcpy(tsk->comm, buf, len);
-	memset(&tsk->comm[len], 0, sizeof(tsk->comm) - len);
+	memcpy(tsk->comm_ext, buf, len);
+	memset(&tsk->comm_ext[len], 0, sizeof(tsk->comm_ext) - len);
 	perf_event_comm(tsk, exec);
 }
 
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 704222114dcc..2605207170b4 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -316,6 +316,7 @@ struct user_event_mm;
  */
 enum {
 	TASK_COMM_LEN = 16,
+	TASK_COMM_EXT_LEN = 64,
 };
 
 extern void sched_tick(void);
@@ -1165,7 +1166,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			comm_ext[TASK_COMM_EXT_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)->comm_ext);		\
 	buf;						\
 })
 
-- 
2.38.1

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

[Yafang Shao]

On Wed, May 21, 2025 at 2:24 PM Bhupesh <bhupesh@igalia.com> 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\>'

Hello Bhupesh,

Several BPF programs currently read task->comm directly, as seen in:

// tools/testing/selftests/bpf/progs/test_skb_helpers.c [0]
bpf_probe_read_kernel_str(&comm, sizeof(comm), &task->comm);

This approach may cause issues after the follow-up patch.
I believe we should replace it with the safer bpf_get_current_comm()
or explicitly null-terminate it with "comm[sizeof(comm) - 1] = '\0'".
Out-of-tree BPF programs like BCC[1] or bpftrace[2] relying on direct
task->comm access may also break and require updates.

[0]. https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/tree/tools/testing/selftests/bpf/progs/test_skb_helpers.c#n26
[1]. https://github.com/iovisor/bcc
[2]. https://github.com/bpftrace/bpftrace

-- 
Regards
Yafang

Re: [PATCH v4 1/3] exec: Remove obsolete comments

[Yafang Shao]

On Wed, May 21, 2025 at 2:24 PM Bhupesh <bhupesh@igalia.com> wrote:
>
> 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>

Acked-by: Yafang Shao <laoar.shao@gmail.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 8c60a42f9d00..704222114dcc 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -1162,10 +1162,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
>


-- 
Regards
Yafang

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

[Yafang Shao]

On Thu, May 22, 2025 at 2:15 PM Yafang Shao <laoar.shao@gmail.com> wrote:
>
> On Wed, May 21, 2025 at 2:24 PM Bhupesh <bhupesh@igalia.com> 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\>'
>
> Hello Bhupesh,
>
> Several BPF programs currently read task->comm directly, as seen in:
>
> // tools/testing/selftests/bpf/progs/test_skb_helpers.c [0]
> bpf_probe_read_kernel_str(&comm, sizeof(comm), &task->comm);
>
> This approach may cause issues after the follow-up patch.
> I believe we should replace it with the safer bpf_get_current_comm()
> or explicitly null-terminate it with "comm[sizeof(comm) - 1] = '\0'".
> Out-of-tree BPF programs like BCC[1] or bpftrace[2] relying on direct
> task->comm access may also break and require updates.
>
> [0]. https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/tree/tools/testing/selftests/bpf/progs/test_skb_helpers.c#n26
> [1]. https://github.com/iovisor/bcc
> [2]. https://github.com/bpftrace/bpftrace

Hmm, upon checking, I confirmed that bpf_probe_read_kernel_str()
already ensures the destination string is null-terminated. Therefore,
this change is unnecessary. Please disregard my previous comment.

-- 
Regards
Yafang

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

[Bhupesh Sharma]

Hi Yafang,

Many thanks for the review.

On 5/22/25 11:57 AM, Yafang Shao wrote:
> On Thu, May 22, 2025 at 2:15 PM Yafang Shao <laoar.shao@gmail.com> wrote:
>> On Wed, May 21, 2025 at 2:24 PM Bhupesh <bhupesh@igalia.com> 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\>'
>> Hello Bhupesh,
>>
>> Several BPF programs currently read task->comm directly, as seen in:
>>
>> // tools/testing/selftests/bpf/progs/test_skb_helpers.c [0]
>> bpf_probe_read_kernel_str(&comm, sizeof(comm), &task->comm);
>>
>> This approach may cause issues after the follow-up patch.
>> I believe we should replace it with the safer bpf_get_current_comm()
>> or explicitly null-terminate it with "comm[sizeof(comm) - 1] = '\0'".
>> Out-of-tree BPF programs like BCC[1] or bpftrace[2] relying on direct
>> task->comm access may also break and require updates.
>>
>> [0]. https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/tree/tools/testing/selftests/bpf/progs/test_skb_helpers.c#n26
>> [1]. https://github.com/iovisor/bcc
>> [2]. https://github.com/bpftrace/bpftrace
> Hmm, upon checking, I confirmed that bpf_probe_read_kernel_str()
> already ensures the destination string is null-terminated. Therefore,
> this change is unnecessary. Please disregard my previous comment.
>

Sure. Yes, bpf_probe_read_kernel_str() handles these cases.

Thanks,
Bhupesh

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

[Kees Cook]

On Wed, May 21, 2025 at 11:53:37AM +0530, 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    comm_ext[TASK_COMM_EXT_LEN];
>        };

I remain unconvinced that this is at all safe. With the existing
memcpy() and so many places using %s and task->comm, this feels very
very risky to me.

Can we just make it separate, instead of a union? Then we don't have to
touch comm at all.

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

We can use set_task_comm() to set both still...

-- 
Kees Cook

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

[Bhupesh Sharma]

Hi Kees,

Thanks for the review.

On 5/23/25 9:18 AM, Kees Cook wrote:
> On Wed, May 21, 2025 at 11:53:37AM +0530, 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    comm_ext[TASK_COMM_EXT_LEN];
>>         };
> I remain unconvinced that this is at all safe. With the existing
> memcpy() and so many places using %s and task->comm, this feels very
> very risky to me.
>
> Can we just make it separate, instead of a union? Then we don't have to
> touch comm at all.

I understand your apprehensions, but I think we have covered _almost_ 
all the existing use-cases as of now:

1. memcpy() users: Handled by [PATCH 2/3] of this series, where we 
identify existing users using the following search
     pattern:
        $ git grep 'memcpy.*->comm\>'

2. %s usage: I checked this at multiple places and can confirm that %s 
usage to print out 'tsk->comm' (as a string), get the longer
     new "extended comm".

3. users who do 'sizeof(->comm)' will continue to get the old value 
because of the union.

The problem with having two separate comms: tsk->comm and tsk->ext_comm, 
instead of a union is two fold:
(a). If we keep two separate statically allocated comms: tsk->comm and 
tsk->ext_comm in struct task_struct, we need to basically keep 
supporting backward compatibility / ABI via tsk->comm and ask new 
user-land users to move to tsk->ext_comm.

(b). If we keep one statically allocated comm: tsk->comm and one dynamically allocated tsk->ext_comm in struct task_struct, then we have the problem of allocating the tsk->ext_comm which _may_ be in the exec()  hot path.

I think the discussion between Linus and Yafang (see [1]), was more towards avoiding the approach in 3(a).

Also we discussed the 3(b) approach, during the review of v2 of this series, where there was a apprehensions around: adding another field to store the task name and allocating tsk->ext_comm dynamically in the exec() hot path (see [2]).

[1]. https://lore.kernel.org/all/CAHk-=wjAmmHUg6vho1KjzQi2=psR30+CogFd4aXrThr2gsiS4g@mail.gmail.com/
[2]. https://lore.kernel.org/lkml/CALOAHbB51b-reG6+ypr43sBJ-QpQhF39r5WPjuEp5rgabgRmoA@mail.gmail.com/

Please let me know your views.

Thanks,
Bhupesh

>> and then modify '__set_task_comm()' to pass 'tsk->comm_ext'
>> to the existing users.
> We can use set_task_comm() to set both still...
>

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

[Kees Cook]

On Fri, May 23, 2025 at 06:01:41PM +0530, Bhupesh Sharma wrote:
> 2. %s usage: I checked this at multiple places and can confirm that %s usage
> to print out 'tsk->comm' (as a string), get the longer
>     new "extended comm".

As an example of why I don't like this union is that this is now lying
to the compiler. e.g. a %s of an object with a known size (sizeof(comm))
may now run off the end of comm without finding a %NUL character... this
is "safe" in the sense that the "extended comm" is %NUL terminated, but
it makes the string length ambiguous for the compiler (and any
associated security hardening).

> 3. users who do 'sizeof(->comm)' will continue to get the old value because
> of the union.

Right -- this is exactly where I think it can get very very wrong,
leaving things unterminated.

> The problem with having two separate comms: tsk->comm and tsk->ext_comm,
> instead of a union is two fold:
> (a). If we keep two separate statically allocated comms: tsk->comm and
> tsk->ext_comm in struct task_struct, we need to basically keep supporting
> backward compatibility / ABI via tsk->comm and ask new user-land users to
> move to tsk->ext_comm.
> 
> (b). If we keep one statically allocated comm: tsk->comm and one dynamically allocated tsk->ext_comm in struct task_struct, then we have the problem of allocating the tsk->ext_comm which _may_ be in the exec()  hot path.
> 
> I think the discussion between Linus and Yafang (see [1]), was more towards avoiding the approach in 3(a).
> 
> Also we discussed the 3(b) approach, during the review of v2 of this series, where there was a apprehensions around: adding another field to store the task name and allocating tsk->ext_comm dynamically in the exec() hot path (see [2]).

Right -- I agree we need them statically allocated. But I think a union
is going to be really error-prone.

How about this: rename task->comm to something else (task->comm_str?),
increase its size and then add ABI-keeping wrappers for everything that
_must_ have the old length.

Doing this guarantees we won't miss anything (since "comm" got renamed),
and during the refactoring all the places where the old length is required
will be glaringly obvious. (i.e. it will be harder to make mistakes
about leaving things unterminated.)

-- 
Kees Cook

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

[Bhupesh Sharma]

Hi Kees,

On 5/24/25 2:25 AM, Kees Cook wrote:
> On Fri, May 23, 2025 at 06:01:41PM +0530, Bhupesh Sharma wrote:
>> 2. %s usage: I checked this at multiple places and can confirm that %s usage
>> to print out 'tsk->comm' (as a string), get the longer
>>      new "extended comm".
> As an example of why I don't like this union is that this is now lying
> to the compiler. e.g. a %s of an object with a known size (sizeof(comm))
> may now run off the end of comm without finding a %NUL character... this
> is "safe" in the sense that the "extended comm" is %NUL terminated, but
> it makes the string length ambiguous for the compiler (and any
> associated security hardening).

Right.

>
>> 3. users who do 'sizeof(->comm)' will continue to get the old value because
>> of the union.
> Right -- this is exactly where I think it can get very very wrong,
> leaving things unterminated.
>
>> The problem with having two separate comms: tsk->comm and tsk->ext_comm,
>> instead of a union is two fold:
>> (a). If we keep two separate statically allocated comms: tsk->comm and
>> tsk->ext_comm in struct task_struct, we need to basically keep supporting
>> backward compatibility / ABI via tsk->comm and ask new user-land users to
>> move to tsk->ext_comm.
>>
>> (b). If we keep one statically allocated comm: tsk->comm and one dynamically allocated tsk->ext_comm in struct task_struct, then we have the problem of allocating the tsk->ext_comm which _may_ be in the exec()  hot path.
>>
>> I think the discussion between Linus and Yafang (see [1]), was more towards avoiding the approach in 3(a).
>>
>> Also we discussed the 3(b) approach, during the review of v2 of this series, where there was a apprehensions around: adding another field to store the task name and allocating tsk->ext_comm dynamically in the exec() hot path (see [2]).
> Right -- I agree we need them statically allocated. But I think a union
> is going to be really error-prone.
>
> How about this: rename task->comm to something else (task->comm_str?),
> increase its size and then add ABI-keeping wrappers for everything that
> _must_ have the old length.
>
> Doing this guarantees we won't miss anything (since "comm" got renamed),
> and during the refactoring all the places where the old length is required
> will be glaringly obvious. (i.e. it will be harder to make mistakes
> about leaving things unterminated.)
>

Ok, I got your point. Let me explore then how best a ABI-keeping wrapper 
can be introduced.
I am thinking of something like:

abi_wrapper_get_task_comm {

     if (requested_comm_length <= 16)
         return 16byte comm with NUL terminator; // old comm (16-bytes)
     else
         return 64byte comm with NUL terminator; // extended comm (64-bytes)
     ....
}

Please let me know if this looks better. Accordingly I will start with 
v5 changes.

Thanks,
Bhupesh

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

[Bhupesh Sharma]

On 5/26/25 4:43 PM, Bhupesh Sharma wrote:
> Hi Kees,
>
> On 5/24/25 2:25 AM, Kees Cook wrote:
>> On Fri, May 23, 2025 at 06:01:41PM +0530, Bhupesh Sharma wrote:
>>> 2. %s usage: I checked this at multiple places and can confirm that 
>>> %s usage
>>> to print out 'tsk->comm' (as a string), get the longer
>>>      new "extended comm".
>> As an example of why I don't like this union is that this is now lying
>> to the compiler. e.g. a %s of an object with a known size (sizeof(comm))
>> may now run off the end of comm without finding a %NUL character... this
>> is "safe" in the sense that the "extended comm" is %NUL terminated, but
>> it makes the string length ambiguous for the compiler (and any
>> associated security hardening).
>
> Right.
>
>>
>>> 3. users who do 'sizeof(->comm)' will continue to get the old value 
>>> because
>>> of the union.
>> Right -- this is exactly where I think it can get very very wrong,
>> leaving things unterminated.
>>
>>> The problem with having two separate comms: tsk->comm and 
>>> tsk->ext_comm,
>>> instead of a union is two fold:
>>> (a). If we keep two separate statically allocated comms: tsk->comm and
>>> tsk->ext_comm in struct task_struct, we need to basically keep 
>>> supporting
>>> backward compatibility / ABI via tsk->comm and ask new user-land 
>>> users to
>>> move to tsk->ext_comm.
>>>
>>> (b). If we keep one statically allocated comm: tsk->comm and one 
>>> dynamically allocated tsk->ext_comm in struct task_struct, then we 
>>> have the problem of allocating the tsk->ext_comm which _may_ be in 
>>> the exec()  hot path.
>>>
>>> I think the discussion between Linus and Yafang (see [1]), was more 
>>> towards avoiding the approach in 3(a).
>>>
>>> Also we discussed the 3(b) approach, during the review of v2 of this 
>>> series, where there was a apprehensions around: adding another field 
>>> to store the task name and allocating tsk->ext_comm dynamically in 
>>> the exec() hot path (see [2]).
>> Right -- I agree we need them statically allocated. But I think a union
>> is going to be really error-prone.
>>
>> How about this: rename task->comm to something else (task->comm_str?),
>> increase its size and then add ABI-keeping wrappers for everything that
>> _must_ have the old length.
>>
>> Doing this guarantees we won't miss anything (since "comm" got renamed),
>> and during the refactoring all the places where the old length is 
>> required
>> will be glaringly obvious. (i.e. it will be harder to make mistakes
>> about leaving things unterminated.)
>>
>
> Ok, I got your point. Let me explore then how best a ABI-keeping 
> wrapper can be introduced.
> I am thinking of something like:
>
> abi_wrapper_get_task_comm {
>
>     if (requested_comm_length <= 16)
>         return 16byte comm with NUL terminator; // old comm (16-bytes)
>     else
>         return 64byte comm with NUL terminator; // extended comm 
> (64-bytes)
>     ....
> }
>
> Please let me know if this looks better. Accordingly I will start with 
> v5 changes.

Hi Everyone, sorry for the delay but I wanted the revive this discussion 
after the -rc1 and my PTO.

I am looking for suggestions on how to implement v5 for this series. 
Here is some background of the version (and related discussions so far):

In the v4, the implementation for tsk->comm handling (for supporting 
long 64byte task names) looked at handling the possible use-cases as 
follows:

1. memcpy() users: Handled by [PATCH 2/3] of this series, where we 
identify existing users using the following search
     pattern:
        $ git grep 'memcpy.*->comm\>'

2. %s usage: I checked this at multiple places and can confirm that %s 
usage to print out 'tsk->comm' (as a string), get the longer
     new "extended comm".

3. users who do 'sizeof(->comm)' will continue to get the old value 
because of the union.

The above points were taken to address the points discussed earlier 
between Linus and Yafang (see [1])

As Kees, suggested in the v4 review (see [2]):
1. Let's rename task->comm to something else (task->comm_str?) and 
increase its size, and

2. Then add ABI-keeping wrappers for everything that  _must_ have the 
old length.

I am thinking of implementing it with something like:

abi_wrapper_get_task_comm {

     if (requested_comm_length <= 16)
         return 16byte comm with NUL terminator; // old comm (16-bytes)
     else
         return 64byte comm with NUL terminator; // extended comm 
(64-bytes)
     ....
}

Kindly let me know your views on the above approach(es).

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

Thanks.