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

Bhupesh Sharma
2025-06-30T07:58:53Z

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.