mbox series

[RFC,0/2] Dynamically allocate memory to store task's full name

Message ID 20250314052715.610377-1-bhupesh@igalia.com (mailing list archive)
Headers show
Series Dynamically allocate memory to store task's full name | expand

Message

Bhupesh March 14, 2025, 5:27 a.m. UTC
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.

For example, currently running 'ps', the task->comm value of a long
task name is truncated due to the limitation of TASK_COMM_LEN.
    create_very_lon

This leads to the names passed from userland via pthread_setname_np()
being truncated.

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

For example for debug applications invoking 'pthread_getname_np()'
to debug task names.

This RFC aims to start a conversation and improve the initial RFC
patchset to avoid such buffer overflows by introducing a new
dynamically allocated pointer to store task's full name, which
shouldn't introduce too much overhead as it is in the non-critical
path.

After this change, the full name of these (otherwise truncated) tasks
will be shown in 'ps'. For example:
    create_very_long_name_user_space_script.sh

Bhupesh (2):
  exec: Dynamically allocate memory to store task's full name
  fs/proc: Pass 'task->full_name' via 'proc_task_name()'

 fs/exec.c             | 21 ++++++++++++++++++---
 fs/proc/array.c       |  2 +-
 include/linux/sched.h |  9 +++++++++
 3 files changed, 28 insertions(+), 4 deletions(-)

Comments

Kees Cook March 14, 2025, 9:25 p.m. UTC | #1
On Fri, Mar 14, 2025 at 10:57:13AM +0530, Bhupesh wrote:
> 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.
> 
> For example, currently running 'ps', the task->comm value of a long
> task name is truncated due to the limitation of TASK_COMM_LEN.
>     create_very_lon
> 
> This leads to the names passed from userland via pthread_setname_np()
> being truncated.

So there have been long discussions about "comm", and it mainly boils
down to "leave it alone". For the /proc-scraping tools like "ps" and
"top", they check both "comm" and "cmdline", depending on mode. The more
useful (and already untruncated) stuff is in "cmdline", so I suspect it
may make more sense to have pthread_setname_np() interact with that
instead. Also TASK_COMM_LEN is basically considered userspace ABI at
this point and we can't sanely change its length without breaking the
world.

Best to use /proc/$pid/task/$tid/cmdline IMO...

-Kees

> will be shown in 'ps'. For example:
>     create_very_long_name_user_space_script.sh
> 
> Bhupesh (2):
>   exec: Dynamically allocate memory to store task's full name
>   fs/proc: Pass 'task->full_name' via 'proc_task_name()'
> 
>  fs/exec.c             | 21 ++++++++++++++++++---
>  fs/proc/array.c       |  2 +-
>  include/linux/sched.h |  9 +++++++++
>  3 files changed, 28 insertions(+), 4 deletions(-)
> 
> -- 
> 2.38.1
>
Andres Rodriguez March 15, 2025, 7:43 a.m. UTC | #2
On 3/14/25 14:25, Kees Cook wrote:
> On Fri, Mar 14, 2025 at 10:57:13AM +0530, Bhupesh wrote:
>> 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.
>>
>> For example, currently running 'ps', the task->comm value of a long
>> task name is truncated due to the limitation of TASK_COMM_LEN.
>>      create_very_lon
>>
>> This leads to the names passed from userland via pthread_setname_np()
>> being truncated.
> 
> So there have been long discussions about "comm", and it mainly boils
> down to "leave it alone". For the /proc-scraping tools like "ps" and
> "top", they check both "comm" and "cmdline", depending on mode. The more
> useful (and already untruncated) stuff is in "cmdline", so I suspect it
> may make more sense to have pthread_setname_np() interact with that
> instead. Also TASK_COMM_LEN is basically considered userspace ABI at
> this point and we can't sanely change its length without breaking the
> world.
> 

Completely agree that comm is best left untouched. TASK_COMM_LEN is 
embedded into the kernel and the pthread ABI changes here should be avoided.

> Best to use /proc/$pid/task/$tid/cmdline IMO...

Your recommendation works great for programs like ps and top, which are
the examples proposed in the cover letter. However, I think the opening 
email didn't point out use cases where the name is modified at runtime. 
In those cases cmdline would be an unsuitable solution as it should 
remain immutable across the process lifetime. An example of this use 
case would be to set a thread's name for debugging purposes and then 
trying to query it via gdb or perf.

I wrote a quick and dirty example to illustrate what I mean:
https://github.com/lostgoat/tasknames

I think an alternative approach could be to have a separate entry in 
procfs to store a tasks debug name (and leave comm completely 
untouched), e.g. /proc/$pid/task/$tid/debug_name. This would allow 
userspace apps to be updated with the following logic:

get_task_debug_name() {
     if ( !is_empty( debug_name ) )
         return debug_name;
     return comm;
}

"Legacy" userspace apps would remain ABI compatible as they would just 
fall back to comm. And apps that want to opt in to the new behaviour can 
be updated one at a time. Which would be work intensive, but even just 
updating gdb and perf would be super helpful.

-Andres

> 
> -Kees
> 
>> will be shown in 'ps'. For example:
>>      create_very_long_name_user_space_script.sh
>>
>> Bhupesh (2):
>>    exec: Dynamically allocate memory to store task's full name
>>    fs/proc: Pass 'task->full_name' via 'proc_task_name()'
>>
>>   fs/exec.c             | 21 ++++++++++++++++++---
>>   fs/proc/array.c       |  2 +-
>>   include/linux/sched.h |  9 +++++++++
>>   3 files changed, 28 insertions(+), 4 deletions(-)
>>
>> -- 
>> 2.38.1
>>
>