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/
Signed-off-by: Bhupesh <bhupesh@igalia.com>
---
include/linux/coredump.h | 3 ++-
include/trace/events/block.h | 5 +++++
include/trace/events/oom.h | 1 +
include/trace/events/osnoise.h | 1 +
include/trace/events/sched.h | 13 +++++++++++++
include/trace/events/signal.h | 1 +
include/trace/events/task.h | 2 ++
7 files changed, 25 insertions(+), 1 deletion(-)
diff --git a/include/linux/coredump.h b/include/linux/coredump.h
index 77e6e195d1d6..058ae3f2bec8 100644
--- 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'; \
printk_ratelimited(Level "coredump: %d(%*pE): " Format "\n", \
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';
),
TP_printk("%d,%d %s %u (%s) %llu + %u %s,%u,%u [%s]",
@@ -352,6 +353,7 @@ DECLARE_EVENT_CLASS(block_bio,
__entry->nr_sector = bio_sectors(bio);
blk_fill_rwbs(__entry->rwbs, bio->bi_opf);
memcpy(__entry->comm, current->comm, TASK_COMM_LEN);
+ __entry->comm[TASK_COMM_LEN - 1] = '\0';
),
TP_printk("%d,%d %s %llu + %u [%s]",
@@ -439,6 +441,7 @@ TRACE_EVENT(block_plug,
TP_fast_assign(
memcpy(__entry->comm, current->comm, TASK_COMM_LEN);
+ __entry->comm[TASK_COMM_LEN - 1] = '\0';
),
TP_printk("[%s]", __entry->comm)
@@ -458,6 +461,7 @@ DECLARE_EVENT_CLASS(block_unplug,
TP_fast_assign(
__entry->nr_rq = depth;
memcpy(__entry->comm, current->comm, TASK_COMM_LEN);
+ __entry->comm[TASK_COMM_LEN - 1] = '\0';
),
TP_printk("[%s] %d", __entry->comm, __entry->nr_rq)
@@ -509,6 +513,7 @@ TRACE_EVENT(block_split,
__entry->new_sector = new_sector;
blk_fill_rwbs(__entry->rwbs, bio->bi_opf);
memcpy(__entry->comm, current->comm, TASK_COMM_LEN);
+ __entry->comm[TASK_COMM_LEN - 1] = '\0';
),
TP_printk("%d,%d %s %llu / %llu [%s]",
diff --git a/include/trace/events/oom.h b/include/trace/events/oom.h
index 9f0a5d1482c4..a5641ed4285f 100644
--- a/include/trace/events/oom.h
+++ b/include/trace/events/oom.h
@@ -24,6 +24,7 @@ TRACE_EVENT(oom_score_adj_update,
TP_fast_assign(
__entry->pid = task->pid;
memcpy(__entry->comm, task->comm, TASK_COMM_LEN);
+ __entry->comm[TASK_COMM_LEN - 1] = '\0';
__entry->oom_score_adj = task->signal->oom_score_adj;
),
diff --git a/include/trace/events/osnoise.h b/include/trace/events/osnoise.h
index 3f4273623801..0321b3f8d532 100644
--- a/include/trace/events/osnoise.h
+++ b/include/trace/events/osnoise.h
@@ -117,6 +117,7 @@ TRACE_EVENT(thread_noise,
TP_fast_assign(
memcpy(__entry->comm, t->comm, TASK_COMM_LEN);
+ __entry->comm[TASK_COMM_LEN - 1] = '\0';
__entry->pid = t->pid;
__entry->start = start;
__entry->duration = duration;
diff --git a/include/trace/events/sched.h b/include/trace/events/sched.h
index 8994e97d86c1..3126d44c43ee 100644
--- a/include/trace/events/sched.h
+++ b/include/trace/events/sched.h
@@ -26,6 +26,7 @@ TRACE_EVENT(sched_kthread_stop,
TP_fast_assign(
memcpy(__entry->comm, t->comm, TASK_COMM_LEN);
+ __entry->comm[TASK_COMM_LEN - 1] = '\0';
__entry->pid = t->pid;
),
@@ -153,6 +154,7 @@ DECLARE_EVENT_CLASS(sched_wakeup_template,
TP_fast_assign(
memcpy(__entry->comm, p->comm, TASK_COMM_LEN);
+ __entry->comm[TASK_COMM_LEN - 1] = '\0';
__entry->pid = p->pid;
__entry->prio = p->prio; /* XXX SCHED_DEADLINE */
__entry->target_cpu = task_cpu(p);
@@ -238,10 +240,12 @@ TRACE_EVENT(sched_switch,
TP_fast_assign(
memcpy(__entry->prev_comm, prev->comm, TASK_COMM_LEN);
+ __entry->comm[TASK_COMM_LEN - 1] = '\0';
__entry->prev_pid = prev->pid;
__entry->prev_prio = prev->prio;
__entry->prev_state = __trace_sched_switch_state(preempt, prev_state, prev);
memcpy(__entry->next_comm, next->comm, TASK_COMM_LEN);
+ __entry->next_comm[TASK_COMM_LEN - 1] = '\0';
__entry->next_pid = next->pid;
__entry->next_prio = next->prio;
/* XXX SCHED_DEADLINE */
@@ -285,6 +289,7 @@ TRACE_EVENT(sched_migrate_task,
TP_fast_assign(
memcpy(__entry->comm, p->comm, TASK_COMM_LEN);
+ __entry->comm[TASK_COMM_LEN - 1] = '\0';
__entry->pid = p->pid;
__entry->prio = p->prio; /* XXX SCHED_DEADLINE */
__entry->orig_cpu = task_cpu(p);
@@ -310,6 +315,7 @@ DECLARE_EVENT_CLASS(sched_process_template,
TP_fast_assign(
memcpy(__entry->comm, p->comm, TASK_COMM_LEN);
+ __entry->comm[TASK_COMM_LEN - 1] = '\0';
__entry->pid = p->pid;
__entry->prio = p->prio; /* XXX SCHED_DEADLINE */
),
@@ -356,6 +362,7 @@ TRACE_EVENT(sched_process_wait,
TP_fast_assign(
memcpy(__entry->comm, current->comm, TASK_COMM_LEN);
+ __entry->comm[TASK_COMM_LEN - 1] = '\0';
__entry->pid = pid_nr(pid);
__entry->prio = current->prio; /* XXX SCHED_DEADLINE */
),
@@ -382,8 +389,10 @@ TRACE_EVENT(sched_process_fork,
TP_fast_assign(
memcpy(__entry->parent_comm, parent->comm, TASK_COMM_LEN);
+ __entry->parent_comm[TASK_COMM_LEN - 1] = '\0';
__entry->parent_pid = parent->pid;
memcpy(__entry->child_comm, child->comm, TASK_COMM_LEN);
+ __entry->child_comm[TASK_COMM_LEN - 1] = '\0';
__entry->child_pid = child->pid;
),
@@ -480,6 +489,7 @@ DECLARE_EVENT_CLASS_SCHEDSTAT(sched_stat_template,
TP_fast_assign(
memcpy(__entry->comm, tsk->comm, TASK_COMM_LEN);
+ __entry->comm[TASK_COMM_LEN - 1] = '\0';
__entry->pid = tsk->pid;
__entry->delay = delay;
),
@@ -538,6 +548,7 @@ DECLARE_EVENT_CLASS(sched_stat_runtime,
TP_fast_assign(
memcpy(__entry->comm, tsk->comm, TASK_COMM_LEN);
+ __entry->comm[TASK_COMM_LEN - 1] = '\0';
__entry->pid = tsk->pid;
__entry->runtime = runtime;
),
@@ -570,6 +581,7 @@ TRACE_EVENT(sched_pi_setprio,
TP_fast_assign(
memcpy(__entry->comm, tsk->comm, TASK_COMM_LEN);
+ __entry->comm[TASK_COMM_LEN - 1] = '\0';
__entry->pid = tsk->pid;
__entry->oldprio = tsk->prio;
__entry->newprio = pi_task ?
@@ -595,6 +607,7 @@ TRACE_EVENT(sched_process_hang,
TP_fast_assign(
memcpy(__entry->comm, tsk->comm, TASK_COMM_LEN);
+ __entry->comm[TASK_COMM_LEN - 1] = '\0';
__entry->pid = tsk->pid;
),
diff --git a/include/trace/events/signal.h b/include/trace/events/signal.h
index 1db7e4b07c01..7f490e553db5 100644
--- a/include/trace/events/signal.h
+++ b/include/trace/events/signal.h
@@ -68,6 +68,7 @@ TRACE_EVENT(signal_generate,
__entry->sig = sig;
TP_STORE_SIGINFO(__entry, info);
memcpy(__entry->comm, task->comm, TASK_COMM_LEN);
+ __entry->comm[TASK_COMM_LEN - 1] = '\0';
__entry->pid = task->pid;
__entry->group = group;
__entry->result = result;
diff --git a/include/trace/events/task.h b/include/trace/events/task.h
index af535b053033..4ddf21b69372 100644
--- a/include/trace/events/task.h
+++ b/include/trace/events/task.h
@@ -22,6 +22,7 @@ TRACE_EVENT(task_newtask,
TP_fast_assign(
__entry->pid = task->pid;
memcpy(__entry->comm, task->comm, TASK_COMM_LEN);
+ __entry->comm[TASK_COMM_LEN - 1] = '\0';
__entry->clone_flags = clone_flags;
__entry->oom_score_adj = task->signal->oom_score_adj;
),
@@ -45,6 +46,7 @@ TRACE_EVENT(task_rename,
TP_fast_assign(
memcpy(entry->oldcomm, task->comm, TASK_COMM_LEN);
+ entry->oldcomm[TASK_COMM_LEN - 1] = '\0';
strscpy(entry->newcomm, comm, TASK_COMM_LEN);
__entry->oom_score_adj = task->signal->oom_score_adj;
),
--
2.38.1Hi Bhupesh,
kernel test robot noticed the following build errors:
[auto build test ERROR on trace/for-next]
[also build test ERROR on tip/sched/core akpm-mm/mm-everything linus/master v6.15-rc5 next-20250508]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Bhupesh/exec-Remove-obsolete-comments/20250507-190740
base: https://git.kernel.org/pub/scm/linux/kernel/git/trace/linux-trace for-next
patch link: https://lore.kernel.org/r/20250507110444.963779-3-bhupesh%40igalia.com
patch subject: [PATCH v3 2/3] treewide: Switch memcpy() users of 'task->comm' to a more safer implementation
config: x86_64-defconfig (https://download.01.org/0day-ci/archive/20250508/202505082038.A5ejhbR4-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-12) 11.3.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250508/202505082038.A5ejhbR4-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202505082038.A5ejhbR4-lkp@intel.com/
All errors (new ones prefixed by >>):
In file included from include/trace/define_trace.h:119,
from include/trace/events/sched.h:856,
from kernel/sched/core.c:84:
include/trace/events/sched.h: In function 'do_trace_event_raw_event_sched_switch':
>> include/trace/events/sched.h:245:24: error: 'struct trace_event_raw_sched_switch' has no member named 'comm'
245 | __entry->comm[TASK_COMM_LEN - 1] = '\0';
| ^~
include/trace/trace_events.h:427:11: note: in definition of macro '__DECLARE_EVENT_CLASS'
427 | { assign; } \
| ^~~~~~
include/trace/trace_events.h:435:23: note: in expansion of macro 'PARAMS'
435 | PARAMS(assign), PARAMS(print)) \
| ^~~~~~
include/trace/trace_events.h:40:9: note: in expansion of macro 'DECLARE_EVENT_CLASS'
40 | DECLARE_EVENT_CLASS(name, \
| ^~~~~~~~~~~~~~~~~~~
include/trace/trace_events.h:44:30: note: in expansion of macro 'PARAMS'
44 | PARAMS(assign), \
| ^~~~~~
include/trace/events/sched.h:224:1: note: in expansion of macro 'TRACE_EVENT'
224 | TRACE_EVENT(sched_switch,
| ^~~~~~~~~~~
include/trace/events/sched.h:243:9: note: in expansion of macro 'TP_fast_assign'
243 | TP_fast_assign(
| ^~~~~~~~~~~~~~
In file included from include/trace/define_trace.h:120,
from include/trace/events/sched.h:856,
from kernel/sched/core.c:84:
include/trace/events/sched.h: In function 'do_perf_trace_sched_switch':
>> include/trace/events/sched.h:245:24: error: 'struct trace_event_raw_sched_switch' has no member named 'comm'
245 | __entry->comm[TASK_COMM_LEN - 1] = '\0';
| ^~
include/trace/perf.h:51:11: note: in definition of macro '__DECLARE_EVENT_CLASS'
51 | { assign; } \
| ^~~~~~
include/trace/perf.h:67:23: note: in expansion of macro 'PARAMS'
67 | PARAMS(assign), PARAMS(print)) \
| ^~~~~~
include/trace/trace_events.h:40:9: note: in expansion of macro 'DECLARE_EVENT_CLASS'
40 | DECLARE_EVENT_CLASS(name, \
| ^~~~~~~~~~~~~~~~~~~
include/trace/trace_events.h:44:30: note: in expansion of macro 'PARAMS'
44 | PARAMS(assign), \
| ^~~~~~
include/trace/events/sched.h:224:1: note: in expansion of macro 'TRACE_EVENT'
224 | TRACE_EVENT(sched_switch,
| ^~~~~~~~~~~
include/trace/events/sched.h:243:9: note: in expansion of macro 'TP_fast_assign'
243 | TP_fast_assign(
| ^~~~~~~~~~~~~~
vim +245 include/trace/events/sched.h
225
226 TP_PROTO(bool preempt,
227 struct task_struct *prev,
228 struct task_struct *next,
229 unsigned int prev_state),
230
231 TP_ARGS(preempt, prev, next, prev_state),
232
233 TP_STRUCT__entry(
234 __array( char, prev_comm, TASK_COMM_LEN )
235 __field( pid_t, prev_pid )
236 __field( int, prev_prio )
237 __field( long, prev_state )
238 __array( char, next_comm, TASK_COMM_LEN )
239 __field( pid_t, next_pid )
240 __field( int, next_prio )
241 ),
242
243 TP_fast_assign(
244 memcpy(__entry->prev_comm, prev->comm, TASK_COMM_LEN);
> 245 __entry->comm[TASK_COMM_LEN - 1] = '\0';
246 __entry->prev_pid = prev->pid;
247 __entry->prev_prio = prev->prio;
248 __entry->prev_state = __trace_sched_switch_state(preempt, prev_state, prev);
249 memcpy(__entry->next_comm, next->comm, TASK_COMM_LEN);
250 __entry->next_comm[TASK_COMM_LEN - 1] = '\0';
251 __entry->next_pid = next->pid;
252 __entry->next_prio = next->prio;
253 /* XXX SCHED_DEADLINE */
254 ),
255
256 TP_printk("prev_comm=%s prev_pid=%d prev_prio=%d prev_state=%s%s ==> next_comm=%s next_pid=%d next_prio=%d",
257 __entry->prev_comm, __entry->prev_pid, __entry->prev_prio,
258
259 (__entry->prev_state & (TASK_REPORT_MAX - 1)) ?
260 __print_flags(__entry->prev_state & (TASK_REPORT_MAX - 1), "|",
261 { TASK_INTERRUPTIBLE, "S" },
262 { TASK_UNINTERRUPTIBLE, "D" },
263 { __TASK_STOPPED, "T" },
264 { __TASK_TRACED, "t" },
265 { EXIT_DEAD, "X" },
266 { EXIT_ZOMBIE, "Z" },
267 { TASK_PARKED, "P" },
268 { TASK_DEAD, "I" }) :
269 "R",
270
271 __entry->prev_state & TASK_REPORT_MAX ? "+" : "",
272 __entry->next_comm, __entry->next_pid, __entry->next_prio)
273 );
274
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
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.
> ),
Best Regards,
Petr
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/
Regards,
Bhupesh
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.
On Thu, 8 May 2025 13:52:01 +0530 Bhupesh Sharma <bhsharma@igalia.com> wrote: > > [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". Note I didn't switch all the events, just most of the sched events. This may affect user space that parses the raw events and may expect a hard coded string instead of a dynamic one (as the sched_switch and sched_waking events do). We will need to investigate before we make these changes, which is why I posted it as a RFC. -- Steve
© 2016 - 2025 Red Hat, Inc.