Re: [PATCH v2 1/3] exec: Dynamically allocate memory to store task's full name

Bhupesh Sharma
2025-04-04T06:49:10Z

Hi Kees,

On 4/3/25 10:00 PM, Kees Cook wrote:
> On Mon, Mar 31, 2025 at 05:48:18PM +0530, Bhupesh wrote:
>> Provide a parallel implementation for get_task_comm() called
>> get_task_full_name() which allows the dynamically allocated
>> and filled-in task's full name to be passed to interested
>> users such as 'gdb'.
>>
>> Currently while running 'gdb', the 'task->comm' value of a long
>> task name is truncated due to the limitation of TASK_COMM_LEN.
>>
>> For example using gdb to debug a simple app currently which generate
>> threads with long task names:
>> # gdb ./threadnames -ex "run info thread" -ex "detach" -ex "quit" > log
>> # cat log
>>
>> NameThatIsTooLo
>>
>> This patch does not touch 'TASK_COMM_LEN' at all, i.e.
>> 'TASK_COMM_LEN' and the 16-byte design remains untouched. Which means
>> that all the legacy / existing ABI, continue to work as before using
>> '/proc/$pid/task/$tid/comm'.
>>
>> This patch only adds a parallel, dynamically-allocated
>> 'task->full_name' which can be used by interested users
>> via '/proc/$pid/task/$tid/full_name'.
>>
>> After this change, gdb is able to show full name of the task:
>> # gdb ./threadnames -ex "run info thread" -ex "detach" -ex "quit" > log
>> # cat log
>>
>> NameThatIsTooLongForComm[4662]
>>
>> Signed-off-by: Bhupesh <bhupesh@igalia.com>
>> ---
>> fs/exec.c | 21 ++++++++++++++++++---
>> include/linux/sched.h | 9 +++++++++
>> 2 files changed, 27 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/exec.c b/fs/exec.c
>> index f45859ad13ac..4219d77a519c 100644
>> --- a/fs/exec.c
>> +++ b/fs/exec.c
>> @@ -1208,6 +1208,9 @@ int begin_new_exec(struct linux_binprm * bprm)
>> {
>> struct task_struct *me = current;
>> int retval;
>> + va_list args;
>> + char *name;
>> + const char *fmt;
>>
>> /* Once we are committed compute the creds */
>> retval = bprm_creds_from_file(bprm);
>> @@ -1348,11 +1351,22 @@ int begin_new_exec(struct linux_binprm * bprm)
>> * detecting a concurrent rename and just want a terminated name.
>> */
>> rcu_read_lock();
>> - __set_task_comm(me, smp_load_acquire(&bprm->file->f_path.dentry->d_name.name),
>> - true);
>> + fmt = smp_load_acquire(&bprm->file->f_path.dentry->d_name.name);
>> + name = kvasprintf(GFP_KERNEL, fmt, args);
>> + if (!name)
>> + return -ENOMEM;
>> +
>> + me->full_name = name;
>> + __set_task_comm(me, fmt, true);
> I don't want to add new allocations to the default exec path unless we
> absolutely must.
>
> In the original proposal this was about setting thread names (after
> exec), and I think that'll be fine.
>

Ok.

>> rcu_read_unlock();
>> } else {
>> - __set_task_comm(me, kbasename(bprm->filename), true);
>> + fmt = kbasename(bprm->filename);
>> + name = kvasprintf(GFP_KERNEL, fmt, args);
>> + if (!name)
>> + return -ENOMEM;
>> +
>> + me->full_name = name;
>> + __set_task_comm(me, fmt, true);
>> }
> I think we can just set me->full_name = me->comm by default.

Sure.

>>
>> /* An exec changes our domain. We are no longer part of the thread
>> @@ -1399,6 +1413,7 @@ int begin_new_exec(struct linux_binprm * bprm)
>> return 0;
>>
>> out_unlock:
>> + kfree(me->full_name);
>> up_write(&me->signal->exec_update_lock);
>> if (!bprm->cred)
>> mutex_unlock(&me->signal->cred_guard_mutex);
>> diff --git a/include/linux/sched.h b/include/linux/sched.h
>> index 56ddeb37b5cd..053b52606652 100644
>> --- a/include/linux/sched.h
>> +++ b/include/linux/sched.h
>> @@ -1166,6 +1166,9 @@ struct task_struct {
>> */
>> char comm[TASK_COMM_LEN];
>>
>> + /* To store the full name if task comm is truncated. */
>> + char *full_name;
>> +
>> struct nameidata *nameidata;
>>
>> #ifdef CONFIG_SYSVIPC
>> @@ -2007,6 +2010,12 @@ extern void __set_task_comm(struct task_struct *tsk, const char *from, bool exec
>> buf; \
>> })
>>
>> +#define get_task_full_name(buf, buf_size, tsk) ({ \
>> + BUILD_BUG_ON(sizeof(buf) < TASK_COMM_LEN); \
>> + strscpy_pad(buf, (tsk)->full_name, buf_size); \
>> + buf; \
>> +})
> I think it should be possible to just switch get_task_comm() to use
> (tsk)->full_name.
>

In another review for this series, Yafang mentioned the following
cleanup + approach suggested by Linus (see [0]).
Also I have summarized my understanding on the basis of the suggestions
Linus shared and the accompanying background threads (please see [1]).

Kindly share your views on the same, so that I can change the
implementation in v3 series accordingly.

[0].https://lore.kernel.org/all/CAHk-=wjAmmHUg6vho1KjzQi2=psR30+CogFd4aXrThr2gsiS4g@mail.gmail.com/
[1]. https://lore.kernel.org/all/20250331121820.455916-1-bhupesh@igalia.com/T/#m7c163829440f2c98e64b475b7cdbc35ba4d613ca

Thanks,
Bhupesh