| Message ID | 20240812022933.69850-8-laoar.shao@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Improve the copy of task comm | expand |
Hi, On Mon, Aug 12, 2024 at 10:29:31AM GMT, Yafang Shao wrote: > Using strscpy() to read the task comm ensures that the name is > always NUL-terminated, regardless of the source string. This approach also > facilitates future extensions to the task comm. Thanks for sending patches replacing str{n}cpy's! I believe there's at least two more instances of strncpy in trace.c as well as in trace_events_hist.c (for a grand total of 6 instances in the files you've touched in this specific patch). It'd be great if you could replace those instances in this patch as well :>) This would help greatly with [1]. > > Signed-off-by: Yafang Shao <laoar.shao@gmail.com> > Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org> > Cc: Steven Rostedt <rostedt@goodmis.org> > Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> > --- > kernel/trace/trace.c | 2 +- > kernel/trace/trace_events_hist.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c > index 578a49ff5c32..1b2577f9d734 100644 > --- a/kernel/trace/trace.c > +++ b/kernel/trace/trace.c > @@ -1907,7 +1907,7 @@ __update_max_tr(struct trace_array *tr, struct task_struct *tsk, int cpu) > max_data->critical_start = data->critical_start; > max_data->critical_end = data->critical_end; > > - strncpy(max_data->comm, tsk->comm, TASK_COMM_LEN); > + strscpy(max_data->comm, tsk->comm, TASK_COMM_LEN); If max_data->comm wants to be NUL-terminated then this is the right replacement. Without knowing how the trace stack works at all, it's hard for me to tell if that is the case. There's a length-supplied format specifier for which this comm field is used with; Either this is just another safeguard against spilling over the buffer or this field really doesn't care about NUL-termination. | seq_printf(m, "# | task: %.16s-%d " | "(uid:%d nice:%ld policy:%ld rt_prio:%ld)\n", | data->comm, data->pid, In the event this field doesn't need to be NUL-terminated then we are introducing an off-by-one error where we are copying one less useful byte with strscpy -- Linus pointed out earlier [2] that these things all just want to be c-strings so this is probably the right change :>) > max_data->pid = tsk->pid; > /* > * If tsk == current, then use current_uid(), as that does not use > diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c > index 6ece1308d36a..4cd24c25ce05 100644 > --- a/kernel/trace/trace_events_hist.c > +++ b/kernel/trace/trace_events_hist.c > @@ -1599,7 +1599,7 @@ static inline void save_comm(char *comm, struct task_struct *task) > return; > } > > - strncpy(comm, task->comm, TASK_COMM_LEN); > + strscpy(comm, task->comm, TASK_COMM_LEN); > } > > static void hist_elt_data_free(struct hist_elt_data *elt_data) > -- > 2.43.5 > Link: https://github.com/KSPP/linux/issues/90 [1] Link: https://lore.kernel.org/all/CAHk-=whWtUC-AjmGJveAETKOMeMFSTwKwu99v7+b6AyHMmaDFA@mail.gmail.com/ [2] Thanks Justin
On Tue, Aug 13, 2024 at 3:19 PM Justin Stitt <justinstitt@google.com> wrote: > > Hi, > > On Mon, Aug 12, 2024 at 10:29:31AM GMT, Yafang Shao wrote: > > Using strscpy() to read the task comm ensures that the name is > > always NUL-terminated, regardless of the source string. This approach also > > facilitates future extensions to the task comm. > > Thanks for sending patches replacing str{n}cpy's! > > I believe there's at least two more instances of strncpy in trace.c as > well as in trace_events_hist.c (for a grand total of 6 instances in the > files you've touched in this specific patch). > > It'd be great if you could replace those instances in this patch as well :>) > > This would help greatly with [1]. > I just saw that Jinjie Ruan sent replacements for these strncpy's too and tracked down and replaced an instance of strscpy() that was present in trace.c but was moved to trace_sched_switch.c during a refactor. They even used the new 2-argument strscpy which is pretty neat. See their patch here: https://lore.kernel.org/all/20240731075058.617588-1-ruanjinjie@huawei.com/ > Link: https://github.com/KSPP/linux/issues/90 [1] > Link: https://lore.kernel.org/all/CAHk-=whWtUC-AjmGJveAETKOMeMFSTwKwu99v7+b6AyHMmaDFA@mail.gmail.com/ [2] > > Thanks > Justin
On Wed, Aug 14, 2024 at 6:31 AM Justin Stitt <justinstitt@google.com> wrote: > > On Tue, Aug 13, 2024 at 3:19 PM Justin Stitt <justinstitt@google.com> wrote: > > > > Hi, > > > > On Mon, Aug 12, 2024 at 10:29:31AM GMT, Yafang Shao wrote: > > > Using strscpy() to read the task comm ensures that the name is > > > always NUL-terminated, regardless of the source string. This approach also > > > facilitates future extensions to the task comm. > > > > Thanks for sending patches replacing str{n}cpy's! > > > > I believe there's at least two more instances of strncpy in trace.c as > > well as in trace_events_hist.c (for a grand total of 6 instances in the > > files you've touched in this specific patch). > > > > It'd be great if you could replace those instances in this patch as well :>) > > > > This would help greatly with [1]. > > > > I just saw that Jinjie Ruan sent replacements for these strncpy's too > and tracked down and replaced an instance of strscpy() that was > present in trace.c but was moved to trace_sched_switch.c during a > refactor. > > They even used the new 2-argument strscpy which is pretty neat. > > See their patch here: > https://lore.kernel.org/all/20240731075058.617588-1-ruanjinjie@huawei.com/ + Jinjie That sounds good. Since this change can be handled as a separate patch, I will drop it from the next version and leave it to Jinjie. Please note that Steven might have a better solution for handling task->comm in trace events, so it’s probably best to leave any changes related to trace events to him [0]. [0] https://lore.kernel.org/all/20240603184016.3374559f@gandalf.local.home/#t -- Regards Yafang
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c index 578a49ff5c32..1b2577f9d734 100644 --- a/kernel/trace/trace.c +++ b/kernel/trace/trace.c @@ -1907,7 +1907,7 @@ __update_max_tr(struct trace_array *tr, struct task_struct *tsk, int cpu) max_data->critical_start = data->critical_start; max_data->critical_end = data->critical_end; - strncpy(max_data->comm, tsk->comm, TASK_COMM_LEN); + strscpy(max_data->comm, tsk->comm, TASK_COMM_LEN); max_data->pid = tsk->pid; /* * If tsk == current, then use current_uid(), as that does not use diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c index 6ece1308d36a..4cd24c25ce05 100644 --- a/kernel/trace/trace_events_hist.c +++ b/kernel/trace/trace_events_hist.c @@ -1599,7 +1599,7 @@ static inline void save_comm(char *comm, struct task_struct *task) return; } - strncpy(comm, task->comm, TASK_COMM_LEN); + strscpy(comm, task->comm, TASK_COMM_LEN); } static void hist_elt_data_free(struct hist_elt_data *elt_data)