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@xxxxxxxxxxxxxx/
--- 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@xxxxxxxxxxxxxxxxxx/
Regards,
Bhupesh
