[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

[kernel test robot]

Hi Bhupesh,

kernel test robot noticed the following build warnings:

[auto build test WARNING on trace/for-next]
[also build test WARNING on tip/sched/core akpm-mm/mm-everything linus/master v6.15-rc7 next-20250521]
[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/20250521-142443
base:   https://git.kernel.org/pub/scm/linux/kernel/git/trace/linux-trace for-next
patch link:    https://lore.kernel.org/r/20250521062337.53262-3-bhupesh%40igalia.com
patch subject: [PATCH v4 2/3] treewide: Switch memcpy() users of 'task->comm' to a more safer implementation
config: arc-randconfig-002-20250522 (https://download.01.org/0day-ci/archive/20250522/202505220326.5yDQHjnt-lkp@intel.com/config)
compiler: arc-linux-gcc (GCC) 8.5.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250522/202505220326.5yDQHjnt-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/202505220326.5yDQHjnt-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from fs/coredump.c:20:
   fs/coredump.c: In function 'do_coredump':
>> include/linux/coredump.h:57:7: warning: array subscript 16 is above array bounds of 'char[16]' [-Warray-bounds]
      comm[TASK_COMM_LEN] = '\0'; \
      ~~~~^~~~~~~~~~~~~~~
   include/linux/coredump.h:63:43: note: in expansion of macro '__COREDUMP_PRINTK'
    #define coredump_report_failure(fmt, ...) __COREDUMP_PRINTK(KERN_WARNING, fmt, ##__VA_ARGS__)
                                              ^~~~~~~~~~~~~~~~~
   fs/coredump.c:655:4: note: in expansion of macro 'coredump_report_failure'
       coredump_report_failure(
       ^~~~~~~~~~~~~~~~~~~~~~~
>> include/linux/coredump.h:57:7: warning: array subscript 16 is above array bounds of 'char[16]' [-Warray-bounds]
      comm[TASK_COMM_LEN] = '\0'; \
      ~~~~^~~~~~~~~~~~~~~
   include/linux/coredump.h:63:43: note: in expansion of macro '__COREDUMP_PRINTK'
    #define coredump_report_failure(fmt, ...) __COREDUMP_PRINTK(KERN_WARNING, fmt, ##__VA_ARGS__)
                                              ^~~~~~~~~~~~~~~~~
   fs/coredump.c:730:4: note: in expansion of macro 'coredump_report_failure'
       coredump_report_failure("Core dump to %s aborted: "
       ^~~~~~~~~~~~~~~~~~~~~~~
>> include/linux/coredump.h:57:7: warning: array subscript 16 is above array bounds of 'char[16]' [-Warray-bounds]
      comm[TASK_COMM_LEN] = '\0'; \
      ~~~~^~~~~~~~~~~~~~~
   include/linux/coredump.h:63:43: note: in expansion of macro '__COREDUMP_PRINTK'
    #define coredump_report_failure(fmt, ...) __COREDUMP_PRINTK(KERN_WARNING, fmt, ##__VA_ARGS__)
                                              ^~~~~~~~~~~~~~~~~
   fs/coredump.c:725:4: note: in expansion of macro 'coredump_report_failure'
       coredump_report_failure("Core dump to %s aborted: "
       ^~~~~~~~~~~~~~~~~~~~~~~
>> include/linux/coredump.h:57:7: warning: array subscript 16 is above array bounds of 'char[16]' [-Warray-bounds]
      comm[TASK_COMM_LEN] = '\0'; \
      ~~~~^~~~~~~~~~~~~~~
   include/linux/coredump.h:63:43: note: in expansion of macro '__COREDUMP_PRINTK'
    #define coredump_report_failure(fmt, ...) __COREDUMP_PRINTK(KERN_WARNING, fmt, ##__VA_ARGS__)
                                              ^~~~~~~~~~~~~~~~~
   fs/coredump.c:618:4: note: in expansion of macro 'coredump_report_failure'
       coredump_report_failure("over core_pipe_limit, skipping core dump");
       ^~~~~~~~~~~~~~~~~~~~~~~
>> include/linux/coredump.h:57:7: warning: array subscript 16 is above array bounds of 'char[16]' [-Warray-bounds]
      comm[TASK_COMM_LEN] = '\0'; \
      ~~~~^~~~~~~~~~~~~~~
   include/linux/coredump.h:63:43: note: in expansion of macro '__COREDUMP_PRINTK'
    #define coredump_report_failure(fmt, ...) __COREDUMP_PRINTK(KERN_WARNING, fmt, ##__VA_ARGS__)
                                              ^~~~~~~~~~~~~~~~~
   fs/coredump.c:642:4: note: in expansion of macro 'coredump_report_failure'
       coredump_report_failure("|%s pipe failed", cn.corename);
       ^~~~~~~~~~~~~~~~~~~~~~~
>> include/linux/coredump.h:57:7: warning: array subscript 16 is above array bounds of 'char[16]' [-Warray-bounds]
      comm[TASK_COMM_LEN] = '\0'; \
      ~~~~^~~~~~~~~~~~~~~
   include/linux/coredump.h:63:43: note: in expansion of macro '__COREDUMP_PRINTK'
    #define coredump_report_failure(fmt, ...) __COREDUMP_PRINTK(KERN_WARNING, fmt, ##__VA_ARGS__)
                                              ^~~~~~~~~~~~~~~~~
   fs/coredump.c:625:4: note: in expansion of macro 'coredump_report_failure'
       coredump_report_failure("%s failed to allocate memory", __func__);
       ^~~~~~~~~~~~~~~~~~~~~~~
>> include/linux/coredump.h:57:7: warning: array subscript 16 is above array bounds of 'char[16]' [-Warray-bounds]
      comm[TASK_COMM_LEN] = '\0'; \
      ~~~~^~~~~~~~~~~~~~~
   include/linux/coredump.h:63:43: note: in expansion of macro '__COREDUMP_PRINTK'
    #define coredump_report_failure(fmt, ...) __COREDUMP_PRINTK(KERN_WARNING, fmt, ##__VA_ARGS__)
                                              ^~~~~~~~~~~~~~~~~
   fs/coredump.c:611:4: note: in expansion of macro 'coredump_report_failure'
       coredump_report_failure("RLIMIT_CORE is set to 1, aborting core");
       ^~~~~~~~~~~~~~~~~~~~~~~
>> include/linux/coredump.h:57:7: warning: array subscript 16 is above array bounds of 'char[16]' [-Warray-bounds]
      comm[TASK_COMM_LEN] = '\0'; \
      ~~~~^~~~~~~~~~~~~~~
   include/linux/coredump.h:63:43: note: in expansion of macro '__COREDUMP_PRINTK'
    #define coredump_report_failure(fmt, ...) __COREDUMP_PRINTK(KERN_WARNING, fmt, ##__VA_ARGS__)
                                              ^~~~~~~~~~~~~~~~~
   fs/coredump.c:591:4: note: in expansion of macro 'coredump_report_failure'
       coredump_report_failure("format_corename failed, aborting core");
       ^~~~~~~~~~~~~~~~~~~~~~~
>> include/linux/coredump.h:57:7: warning: array subscript 16 is above array bounds of 'char[16]' [-Warray-bounds]
      comm[TASK_COMM_LEN] = '\0'; \
      ~~~~^~~~~~~~~~~~~~~
   include/linux/coredump.h:63:43: note: in expansion of macro '__COREDUMP_PRINTK'
    #define coredump_report_failure(fmt, ...) __COREDUMP_PRINTK(KERN_WARNING, fmt, ##__VA_ARGS__)
                                              ^~~~~~~~~~~~~~~~~
   fs/coredump.c:752:4: note: in expansion of macro 'coredump_report_failure'
       coredump_report_failure("Core dump to |%s disabled", cn.corename);
       ^~~~~~~~~~~~~~~~~~~~~~~
   fs/coredump.c: In function 'validate_coredump_safety':
>> include/linux/coredump.h:57:7: warning: array subscript 16 is above array bounds of 'char[16]' [-Warray-bounds]
      comm[TASK_COMM_LEN] = '\0'; \
      ~~~~^~~~~~~~~~~~~~~
   include/linux/coredump.h:63:43: note: in expansion of macro '__COREDUMP_PRINTK'
    #define coredump_report_failure(fmt, ...) __COREDUMP_PRINTK(KERN_WARNING, fmt, ##__VA_ARGS__)
                                              ^~~~~~~~~~~~~~~~~
   fs/coredump.c:1006:3: note: in expansion of macro 'coredump_report_failure'
      coredump_report_failure("Unsafe core_pattern used with fs.suid_dumpable=2: "
      ^~~~~~~~~~~~~~~~~~~~~~~


vim +57 include/linux/coredump.h

    46	
    47	/*
    48	 * Logging for the coredump code, ratelimited.
    49	 * The TGID and comm fields are added to the message.
    50	 */
    51	
    52	#define __COREDUMP_PRINTK(Level, Format, ...) \
    53		do {	\
    54			char comm[TASK_COMM_LEN];	\
    55			/* This will always be NUL terminated. */ \
    56			memcpy(comm, current->comm, TASK_COMM_LEN); \
  > 57			comm[TASK_COMM_LEN] = '\0'; \
    58			printk_ratelimited(Level "coredump: %d(%*pE): " Format "\n",	\
    59				task_tgid_vnr(current), (int)strlen(comm), comm, ##__VA_ARGS__);	\
    60		} while (0)	\
    61	

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

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 2/3] treewide: Switch memcpy() users of 'task->comm' to a more safer implementation

[Bhupesh Sharma]

Hi,

On 5/22/25 1:32 AM, kernel test robot wrote:
> Hi Bhupesh,
>
> kernel test robot noticed the following build warnings:
>
> [auto build test WARNING on trace/for-next]
> [also build test WARNING on tip/sched/core akpm-mm/mm-everything linus/master v6.15-rc7 next-20250521]
> [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/20250521-142443
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/trace/linux-trace for-next
> patch link:    https://lore.kernel.org/r/20250521062337.53262-3-bhupesh%40igalia.com
> patch subject: [PATCH v4 2/3] treewide: Switch memcpy() users of 'task->comm' to a more safer implementation
> config: arc-randconfig-002-20250522 (https://download.01.org/0day-ci/archive/20250522/202505220326.5yDQHjnt-lkp@intel.com/config)
> compiler: arc-linux-gcc (GCC) 8.5.0
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250522/202505220326.5yDQHjnt-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/202505220326.5yDQHjnt-lkp@intel.com/
>
> All warnings (new ones prefixed by >>):
>
>     In file included from fs/coredump.c:20:
>     fs/coredump.c: In function 'do_coredump':
>>> include/linux/coredump.h:57:7: warning: array subscript 16 is above array bounds of 'char[16]' [-Warray-bounds]
>        comm[TASK_COMM_LEN] = '\0'; \
>        ~~~~^~~~~~~~~~~~~~~
>     include/linux/coredump.h:63:43: note: in expansion of macro '__COREDUMP_PRINTK'
>      #define coredump_report_failure(fmt, ...) __COREDUMP_PRINTK(KERN_WARNING, fmt, ##__VA_ARGS__)
>                                                ^~~~~~~~~~~~~~~~~
>     fs/coredump.c:655:4: note: in expansion of macro 'coredump_report_failure'
>         coredump_report_failure(
>         ^~~~~~~~~~~~~~~~~~~~~~~
>>> include/linux/coredump.h:57:7: warning: array subscript 16 is above array bounds of 'char[16]' [-Warray-bounds]
>        comm[TASK_COMM_LEN] = '\0'; \
>        ~~~~^~~~~~~~~~~~~~~
>     include/linux/coredump.h:63:43: note: in expansion of macro '__COREDUMP_PRINTK'
>      #define coredump_report_failure(fmt, ...) __COREDUMP_PRINTK(KERN_WARNING, fmt, ##__VA_ARGS__)
>                                                ^~~~~~~~~~~~~~~~~
>     fs/coredump.c:730:4: note: in expansion of macro 'coredump_report_failure'
>         coredump_report_failure("Core dump to %s aborted: "
>         ^~~~~~~~~~~~~~~~~~~~~~~
>>> include/linux/coredump.h:57:7: warning: array subscript 16 is above array bounds of 'char[16]' [-Warray-bounds]
>        comm[TASK_COMM_LEN] = '\0'; \
>        ~~~~^~~~~~~~~~~~~~~
>     include/linux/coredump.h:63:43: note: in expansion of macro '__COREDUMP_PRINTK'
>      #define coredump_report_failure(fmt, ...) __COREDUMP_PRINTK(KERN_WARNING, fmt, ##__VA_ARGS__)
>                                                ^~~~~~~~~~~~~~~~~
>     fs/coredump.c:725:4: note: in expansion of macro 'coredump_report_failure'
>         coredump_report_failure("Core dump to %s aborted: "
>         ^~~~~~~~~~~~~~~~~~~~~~~
>>> include/linux/coredump.h:57:7: warning: array subscript 16 is above array bounds of 'char[16]' [-Warray-bounds]
>        comm[TASK_COMM_LEN] = '\0'; \
>        ~~~~^~~~~~~~~~~~~~~
>     include/linux/coredump.h:63:43: note: in expansion of macro '__COREDUMP_PRINTK'
>      #define coredump_report_failure(fmt, ...) __COREDUMP_PRINTK(KERN_WARNING, fmt, ##__VA_ARGS__)
>                                                ^~~~~~~~~~~~~~~~~
>     fs/coredump.c:618:4: note: in expansion of macro 'coredump_report_failure'
>         coredump_report_failure("over core_pipe_limit, skipping core dump");
>         ^~~~~~~~~~~~~~~~~~~~~~~
>>> include/linux/coredump.h:57:7: warning: array subscript 16 is above array bounds of 'char[16]' [-Warray-bounds]
>        comm[TASK_COMM_LEN] = '\0'; \
>        ~~~~^~~~~~~~~~~~~~~
>     include/linux/coredump.h:63:43: note: in expansion of macro '__COREDUMP_PRINTK'
>      #define coredump_report_failure(fmt, ...) __COREDUMP_PRINTK(KERN_WARNING, fmt, ##__VA_ARGS__)
>                                                ^~~~~~~~~~~~~~~~~
>     fs/coredump.c:642:4: note: in expansion of macro 'coredump_report_failure'
>         coredump_report_failure("|%s pipe failed", cn.corename);
>         ^~~~~~~~~~~~~~~~~~~~~~~
>>> include/linux/coredump.h:57:7: warning: array subscript 16 is above array bounds of 'char[16]' [-Warray-bounds]
>        comm[TASK_COMM_LEN] = '\0'; \
>        ~~~~^~~~~~~~~~~~~~~
>     include/linux/coredump.h:63:43: note: in expansion of macro '__COREDUMP_PRINTK'
>      #define coredump_report_failure(fmt, ...) __COREDUMP_PRINTK(KERN_WARNING, fmt, ##__VA_ARGS__)
>                                                ^~~~~~~~~~~~~~~~~
>     fs/coredump.c:625:4: note: in expansion of macro 'coredump_report_failure'
>         coredump_report_failure("%s failed to allocate memory", __func__);
>         ^~~~~~~~~~~~~~~~~~~~~~~
>>> include/linux/coredump.h:57:7: warning: array subscript 16 is above array bounds of 'char[16]' [-Warray-bounds]
>        comm[TASK_COMM_LEN] = '\0'; \
>        ~~~~^~~~~~~~~~~~~~~
>     include/linux/coredump.h:63:43: note: in expansion of macro '__COREDUMP_PRINTK'
>      #define coredump_report_failure(fmt, ...) __COREDUMP_PRINTK(KERN_WARNING, fmt, ##__VA_ARGS__)
>                                                ^~~~~~~~~~~~~~~~~
>     fs/coredump.c:611:4: note: in expansion of macro 'coredump_report_failure'
>         coredump_report_failure("RLIMIT_CORE is set to 1, aborting core");
>         ^~~~~~~~~~~~~~~~~~~~~~~
>>> include/linux/coredump.h:57:7: warning: array subscript 16 is above array bounds of 'char[16]' [-Warray-bounds]
>        comm[TASK_COMM_LEN] = '\0'; \
>        ~~~~^~~~~~~~~~~~~~~
>     include/linux/coredump.h:63:43: note: in expansion of macro '__COREDUMP_PRINTK'
>      #define coredump_report_failure(fmt, ...) __COREDUMP_PRINTK(KERN_WARNING, fmt, ##__VA_ARGS__)
>                                                ^~~~~~~~~~~~~~~~~
>     fs/coredump.c:591:4: note: in expansion of macro 'coredump_report_failure'
>         coredump_report_failure("format_corename failed, aborting core");
>         ^~~~~~~~~~~~~~~~~~~~~~~
>>> include/linux/coredump.h:57:7: warning: array subscript 16 is above array bounds of 'char[16]' [-Warray-bounds]
>        comm[TASK_COMM_LEN] = '\0'; \
>        ~~~~^~~~~~~~~~~~~~~
>     include/linux/coredump.h:63:43: note: in expansion of macro '__COREDUMP_PRINTK'
>      #define coredump_report_failure(fmt, ...) __COREDUMP_PRINTK(KERN_WARNING, fmt, ##__VA_ARGS__)
>                                                ^~~~~~~~~~~~~~~~~
>     fs/coredump.c:752:4: note: in expansion of macro 'coredump_report_failure'
>         coredump_report_failure("Core dump to |%s disabled", cn.corename);
>         ^~~~~~~~~~~~~~~~~~~~~~~
>     fs/coredump.c: In function 'validate_coredump_safety':
>>> include/linux/coredump.h:57:7: warning: array subscript 16 is above array bounds of 'char[16]' [-Warray-bounds]
>        comm[TASK_COMM_LEN] = '\0'; \
>        ~~~~^~~~~~~~~~~~~~~
>     include/linux/coredump.h:63:43: note: in expansion of macro '__COREDUMP_PRINTK'
>      #define coredump_report_failure(fmt, ...) __COREDUMP_PRINTK(KERN_WARNING, fmt, ##__VA_ARGS__)
>                                                ^~~~~~~~~~~~~~~~~
>     fs/coredump.c:1006:3: note: in expansion of macro 'coredump_report_failure'
>        coredump_report_failure("Unsafe core_pattern used with fs.suid_dumpable=2: "
>        ^~~~~~~~~~~~~~~~~~~~~~~
>
>
> vim +57 include/linux/coredump.h
>
>      46	
>      47	/*
>      48	 * Logging for the coredump code, ratelimited.
>      49	 * The TGID and comm fields are added to the message.
>      50	 */
>      51	
>      52	#define __COREDUMP_PRINTK(Level, Format, ...) \
>      53		do {	\
>      54			char comm[TASK_COMM_LEN];	\
>      55			/* This will always be NUL terminated. */ \
>      56			memcpy(comm, current->comm, TASK_COMM_LEN); \
>    > 57			comm[TASK_COMM_LEN] = '\0'; \
>      58			printk_ratelimited(Level "coredump: %d(%*pE): " Format "\n",	\
>      59				task_tgid_vnr(current), (int)strlen(comm), comm, ##__VA_ARGS__);	\
>      60		} while (0)	\
>      61	
>

Thanks, I will fix these in v5.

Regards,
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 2/3] treewide: Switch memcpy() users of 'task->comm' to a more safer implementation

[Dan Carpenter]

Hi Bhupesh,

kernel test robot noticed the following build warnings:

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/20250521-142443
base:   https://git.kernel.org/pub/scm/linux/kernel/git/trace/linux-trace for-next
patch link:    https://lore.kernel.org/r/20250521062337.53262-3-bhupesh%40igalia.com
patch subject: [PATCH v4 2/3] treewide: Switch memcpy() users of 'task->comm' to a more safer implementation
config: powerpc64-randconfig-r071-20250522 (https://download.01.org/0day-ci/archive/20250522/202505221104.qV4Iy0rA-lkp@intel.com/config)
compiler: clang version 21.0.0git (https://github.com/llvm/llvm-project f819f46284f2a79790038e1f6649172789734ae8)

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>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202505221104.qV4Iy0rA-lkp@intel.com/

New smatch warnings:
fs/coredump.c:591 do_coredump() error: buffer overflow 'comm' 16 <= 16
fs/coredump.c:1006 validate_coredump_safety() error: buffer overflow 'comm' 16 <= 16

vim +/comm +591 fs/coredump.c

a78282e2c94f4c Linus Torvalds               2024-09-26  524  void do_coredump(const kernel_siginfo_t *siginfo)
10c28d937e2cca Alex Kelly                   2012-09-26  525  {
10c28d937e2cca Alex Kelly                   2012-09-26  526  	struct core_state core_state;
10c28d937e2cca Alex Kelly                   2012-09-26  527  	struct core_name cn;
10c28d937e2cca Alex Kelly                   2012-09-26  528  	struct mm_struct *mm = current->mm;
10c28d937e2cca Alex Kelly                   2012-09-26  529  	struct linux_binfmt * binfmt;
10c28d937e2cca Alex Kelly                   2012-09-26  530  	const struct cred *old_cred;
10c28d937e2cca Alex Kelly                   2012-09-26  531  	struct cred *cred;
a78282e2c94f4c Linus Torvalds               2024-09-26  532  	int retval = 0;
10c28d937e2cca Alex Kelly                   2012-09-26  533  	int ispipe;
315c69261dd3fa Paul Wise                    2019-08-02  534  	size_t *argv = NULL;
315c69261dd3fa Paul Wise                    2019-08-02  535  	int argc = 0;
fbb1816942c044 Jann Horn                    2015-09-09  536  	/* require nonrelative corefile path and be extra careful */
fbb1816942c044 Jann Horn                    2015-09-09  537  	bool need_suid_safe = false;
acdedd99b0f3bf Oleg Nesterov                2013-04-30  538  	bool core_dumped = false;
10c28d937e2cca Alex Kelly                   2012-09-26  539  	static atomic_t core_dump_count = ATOMIC_INIT(0);
10c28d937e2cca Alex Kelly                   2012-09-26  540  	struct coredump_params cprm = {
5ab1c309b34488 Denys Vlasenko               2012-10-04  541  		.siginfo = siginfo,
10c28d937e2cca Alex Kelly                   2012-09-26  542  		.limit = rlimit(RLIMIT_CORE),
10c28d937e2cca Alex Kelly                   2012-09-26  543  		/*
10c28d937e2cca Alex Kelly                   2012-09-26  544  		 * We must use the same mm->flags while dumping core to avoid
10c28d937e2cca Alex Kelly                   2012-09-26  545  		 * inconsistency of bit flags, since this flag is not protected
10c28d937e2cca Alex Kelly                   2012-09-26  546  		 * by any locks.
10c28d937e2cca Alex Kelly                   2012-09-26  547  		 */
10c28d937e2cca Alex Kelly                   2012-09-26  548  		.mm_flags = mm->flags,
95c5436a488384 Eric W. Biederman            2022-03-08  549  		.vma_meta = NULL,
8603b6f58637ce Oleksandr Natalenko          2022-09-03  550  		.cpu = raw_smp_processor_id(),
10c28d937e2cca Alex Kelly                   2012-09-26  551  	};
10c28d937e2cca Alex Kelly                   2012-09-26  552  
5ab1c309b34488 Denys Vlasenko               2012-10-04  553  	audit_core_dumps(siginfo->si_signo);
10c28d937e2cca Alex Kelly                   2012-09-26  554  
10c28d937e2cca Alex Kelly                   2012-09-26  555  	binfmt = mm->binfmt;
a78282e2c94f4c Linus Torvalds               2024-09-26  556  	if (!binfmt || !binfmt->core_dump)
10c28d937e2cca Alex Kelly                   2012-09-26  557  		goto fail;
a78282e2c94f4c Linus Torvalds               2024-09-26  558  	if (!__get_dumpable(cprm.mm_flags))
10c28d937e2cca Alex Kelly                   2012-09-26  559  		goto fail;
10c28d937e2cca Alex Kelly                   2012-09-26  560  
10c28d937e2cca Alex Kelly                   2012-09-26  561  	cred = prepare_creds();
a78282e2c94f4c Linus Torvalds               2024-09-26  562  	if (!cred)
10c28d937e2cca Alex Kelly                   2012-09-26  563  		goto fail;
10c28d937e2cca Alex Kelly                   2012-09-26  564  	/*
10c28d937e2cca Alex Kelly                   2012-09-26  565  	 * We cannot trust fsuid as being the "true" uid of the process
10c28d937e2cca Alex Kelly                   2012-09-26  566  	 * nor do we know its entire history. We only know it was tainted
10c28d937e2cca Alex Kelly                   2012-09-26  567  	 * so we dump it as root in mode 2, and only into a controlled
10c28d937e2cca Alex Kelly                   2012-09-26  568  	 * environment (pipe handler or fully qualified path).
10c28d937e2cca Alex Kelly                   2012-09-26  569  	 */
e579d2c259be42 Kees Cook                    2013-02-27  570  	if (__get_dumpable(cprm.mm_flags) == SUID_DUMP_ROOT) {
10c28d937e2cca Alex Kelly                   2012-09-26  571  		/* Setuid core dump mode */
10c28d937e2cca Alex Kelly                   2012-09-26  572  		cred->fsuid = GLOBAL_ROOT_UID;	/* Dump root private */
fbb1816942c044 Jann Horn                    2015-09-09  573  		need_suid_safe = true;
10c28d937e2cca Alex Kelly                   2012-09-26  574  	}
10c28d937e2cca Alex Kelly                   2012-09-26  575  
5ab1c309b34488 Denys Vlasenko               2012-10-04  576  	retval = coredump_wait(siginfo->si_signo, &core_state);
10c28d937e2cca Alex Kelly                   2012-09-26  577  	if (retval < 0)
10c28d937e2cca Alex Kelly                   2012-09-26  578  		goto fail_creds;
10c28d937e2cca Alex Kelly                   2012-09-26  579  
10c28d937e2cca Alex Kelly                   2012-09-26  580  	old_cred = override_creds(cred);
10c28d937e2cca Alex Kelly                   2012-09-26  581  
315c69261dd3fa Paul Wise                    2019-08-02  582  	ispipe = format_corename(&cn, &cprm, &argv, &argc);
10c28d937e2cca Alex Kelly                   2012-09-26  583  
10c28d937e2cca Alex Kelly                   2012-09-26  584  	if (ispipe) {
315c69261dd3fa Paul Wise                    2019-08-02  585  		int argi;
10c28d937e2cca Alex Kelly                   2012-09-26  586  		int dump_count;
10c28d937e2cca Alex Kelly                   2012-09-26  587  		char **helper_argv;
907ed1328d2a74 Lucas De Marchi              2013-04-30  588  		struct subprocess_info *sub_info;
10c28d937e2cca Alex Kelly                   2012-09-26  589  
10c28d937e2cca Alex Kelly                   2012-09-26  590  		if (ispipe < 0) {
c114e9948c2b6a Roman Kisel                  2024-07-18 @591  			coredump_report_failure("format_corename failed, aborting core");
e7fd1549aeb83e Oleg Nesterov                2013-07-03  592  			goto fail_unlock;

>               /* This will always be NUL terminated. */ \
> -             memcpy(comm, current->comm, sizeof(comm)); \
> +             memcpy(comm, current->comm, TASK_COMM_LEN); \
> +             comm[TASK_COMM_LEN] = '\0'; \
                     ^^^^^^^^^^^^^^
This was supposed to be "TASK_COMM_LEN - 1".  Also the comment says
it's not required...

10c28d937e2cca Alex Kelly                   2012-09-26  593  		}
10c28d937e2cca Alex Kelly                   2012-09-26  594  
10c28d937e2cca Alex Kelly                   2012-09-26  595  		if (cprm.limit == 1) {
10c28d937e2cca Alex Kelly                   2012-09-26  596  			/* See umh_pipe_setup() which sets RLIMIT_CORE = 1.

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

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.