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

[Bhupesh Sharma <bhsharma@igalia.com>]

On 5/8/25 1:47 PM, Bhupesh Sharma wrote:
> Hi Petr,
>
> On 5/7/25 5:59 PM, Petr Mladek wrote:
>> On Wed 2025-05-07 16:34:43, Bhupesh 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\>'
>>>
>>> [1]. 
>>> https://lore.kernel.org/all/CAHk-=wjAmmHUg6vho1KjzQi2=psR30+CogFd4aXrThr2gsiS4g@mail.gmail.com/
>>>
>>> --- a/include/linux/coredump.h
>>> +++ b/include/linux/coredump.h
>>> @@ -53,7 +53,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'; \
>> I would expect that we replace this with a helper function/macro
>> which would do the right thing.
>>
>> Why is get_task_comm() not used here, please?
>>
>>>           printk_ratelimited(Level "coredump: %d(%*pE): " Format 
>>> "\n",    \
>> Also the name seems to be used for printing a debug information.
>> I would expect that we could use the bigger buffer here and print
>> the "full" name. Is this planed, please?
>>
>>>               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 bd0ea07338eb..94a941ac2034 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';
>> Same for all other callers.
>>
>> That said, I am not sure if the larger buffer is save in all situations.
>>
>>>       ),
>>
>
> Thanks for the review, I agree on using the helper / wrapper function 
> to replace this open-coded memcpy + set last entry as '\0'.
>
> However I see that Steven has already shared a RFC approach (see [1]), 
> to use __string() instead of fixed lengths for 'task->comm' for 
> tracing events.
> I plan to  rebase my v4 on top of his RFC, which might mean that this 
> patch would no longer be needed in the v4.
>
> [1]. 
> https://lore.kernel.org/linux-trace-kernel/20250507133458.51bafd95@gandalf.local.home/
>

Sorry, pressed the send button too quickly :D

I instead meant - "I plan to  rebase my v4 on top of Steven's RFC, which 
might mean that this patch would no longer need to address the trace 
events, but would still need to handle other places where tsk->comm 
directly in memcpy() and replace it with 'get_task_comm()' instead".

Thanks.