ftrace: fix task's invalid comm of <...> when big pid
PATCH 1/1 ftrace: fix task's invalid comm of <...> when big pid
when pid is bigger than PID_MAX_DEFAULT, the comm of task is <...>, it is better use pid_max to compare
--- kernel/trace/trace.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) mode change 100644 => 100755 kernel/trace/trace.c
kernel/trace/trace.c +1 -1
@@ -1976,7 +1976,7 @@ static void __trace_find_cmdline(int pid, char comm[])return ; } -if (pid >PID_MAX_DEFAULT ) { +if (pid > pid_max) {strcpy (comm,"<...>" );return ; }
Signature · 1 line
1.8.3.1
Steven Rostedt
maintainerQuoted 16 lines · On Wed, 28 Mar 2018 20:32:27 +0800 Wang Yu <[email protected]> wrote:
when pid is bigger than
PID_MAX_DEFAULT, the comm of task is <...>, it is better usepid_maxto compareSigned-off-by Wang Yu <[email protected]>--- kernel/trace/trace.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) mode change 100644 => 100755 kernel/trace/trace.ckernel/trace/trace.c +0
-0
kernel/trace/trace.c static void __trace_find_cmdline(int pid, char comm[]) · line 1979
Quoted 5 lines
kernel/trace/trace.c +1
-1@@ -1976,7 +1976,7 @@ static void __trace_find_cmdline(int pid, char comm[])return ; } -if (pid >PID_MAX_DEFAULT ) { +if (pid > pid_max) {
Thanks! this probably should go to stable.
-- Steve
Quoted 3 lines
kernel/trace/trace.c +0
-0strcpy (comm,"<...>" );return ; }
Steven Rostedt
maintainerQuoted 22 lines · On Wed, 28 Mar 2018 11:35:22 -0400 Steven Rostedt <[email protected]> wrote:
Quoted 19 lines · On Wed, 28 Mar 2018 20:32:27 +0800 Wang Yu <[email protected]> wrote:
when pid is bigger than
PID_MAX_DEFAULT, the comm of task is <...>, it is better usepid_maxto compareSigned-off-by Wang Yu <[email protected]>--- kernel/trace/trace.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) mode change 100644 => 100755 kernel/trace/trace.ckernel/trace/trace.c +0
-0@@ -1976,7 +1976,7 @@ static void __trace_find_cmdline(int pid, char comm[])return ; }
Quoted 4 lines
kernel/trace/trace.c static void __trace_find_cmdline(int pid, char comm[]) · line 1979
Quoted 2 lines
kernel/trace/trace.c +1
-1-if (pid >PID_MAX_DEFAULT ) { +if (pid > pid_max) {Thanks! this probably should go to stable.
I take that back. This patch can cause a buffer overflow access.
If you looked at the line after this check, you would see:
if (pid > PID_MAX_DEFAULT) {
strcpy(comm, "<...>");
return;
}
map = savedcmd->map_pid_to_cmdline[pid];And if you looked to see what map_pid_to_cmdline is:
struct saved_cmdlines_buffer {
unsigned map_pid_to_cmdline[PID_MAX_DEFAULT+1];Your patch will access memory past the end of that array, and cause a bug.
If you want to support more than PID_MAX_DEFAULT, a lot more needs to change than this. And a change like this isn't going to go to stable.
What you can do is make that map_pid_to_cmdline array bigger.
-- Steve
Quoted 7 lines
-- Steve
Quoted 3 lines
kernel/trace/trace.c +0
-0strcpy (comm,"<...>" );return ; }
Quoted 47 lines · Steven Rostedt
Quoted 22 lines · On Wed, 28 Mar 2018 11:35:22 -0400 Steven Rostedt <[email protected]> wrote:
Quoted 19 lines · On Wed, 28 Mar 2018 20:32:27 +0800 Wang Yu <[email protected]> wrote:
when pid is bigger than
PID_MAX_DEFAULT, the comm of task is <...>, it is better usepid_maxto compareSigned-off-by Wang Yu <[email protected]>--- kernel/trace/trace.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)mode change 100644 => 100755
kernel/trace/trace.ckernel/trace/trace.c +0
-0@@ -1976,7 +1976,7 @@ static void __trace_find_cmdline(int pid, char comm[])return ; }Quoted 3 lines
kernel/trace/trace.c static void __trace_find_cmdline(int pid, char comm[]) · line 1979
Quoted 2 lines
kernel/trace/trace.c +1
-1-if (pid >PID_MAX_DEFAULT ) { +if (pid > pid_max) {Thanks! this probably should go to stable.
I take that back. This patch can cause a buffer overflow access.
If you looked at the line after this check, you would see:
if (pid > PID_MAX_DEFAULT) { strcpy(comm, "<...>"); return; } map = savedcmd->map_pid_to_cmdline[pid];And if you looked to see what
map_pid_to_cmdlineis:struct saved_cmdlines_buffer { unsigned map_pid_to_cmdline[PID_MAX_DEFAULT+1];Your patch will access memory past the end of that array, and cause a bug.
Quoted 6 lines
If you want to support more than
PID_MAX_DEFAULT, a lot more needs to change than this. And a change like this isn't going to go to stable.What you can do is make that
map_pid_to_cmdlinearray bigger.-- Steve
I am sorry about it, and as the number of cpu cores increases, the current
PID_MAX_DEFAULT is too small, our online machines set the pid_max 65536 as default, so the task pid number bigger than PID_MAX_DEFAULT can't show the real comm (only <...>), so i want to ajust the PID_MAX_DEFAULT upto 4x, and what do you think?
* This controls the default maximum pid allocated to a process */
-#define PID_MAX_DEFAULT (CONFIG_BASE_SMALL ? 0x1000 : 0x8000) +#define PID_MAX_DEFAULT (CONFIG_BASE_SMALL ? 0x1000 : 0x20000)
Quoted 7 lines
Quoted 7 lines
-- Steve
Quoted 3 lines
kernel/trace/trace.c +0
-0strcpy (comm,"<...>" );return ; }
Steven Rostedt
maintainerQuoted 12 lines · On Thu, 29 Mar 2018 10:16:22 +0800 Wang Yu <[email protected]> wrote:
Quoted 3 lines
What you can do is make that
map_pid_to_cmdlinearray bigger.-- Steve
I am sorry about it, and as the number of cpu cores increases, the current
PID_MAX_DEFAULTis too small, our online machines set thepid_max65536 as default, so the task pid number bigger thanPID_MAX_DEFAULTcan't show the real comm (only <...>), so i want to ajust thePID_MAX_DEFAULTupto 4x, and what do you think?* This controls the default maximum pid allocated to a process */
Review
Quoted 2 lines
(quoted diff) +1
-1-#define PID_MAX_DEFAULT (CONFIG_BASE_SMALL ? 0x1000 : 0x8000) +#define PID_MAX_DEFAULT (CONFIG_BASE_SMALL ? 0x1000 : 0x20000)
What I was thinking is to make the map_pid_to_cmdline array dynamic (not static), and be set to pid_max (after pid_max is determined).
Now, pid_max can be changed at run time. Thus, the tracing code will need to keep a separate variable for that array to store the length. It can not rely on pid_max. But if a pid that is greater than pid_max is found, we could kick off a work thread to increase the array.
-- Steve
Quoted 19 lines · Steven Rostedt
Review
Quoted 13 lines · On Thu, 29 Mar 2018 10:16:22 +0800 Wang Yu <[email protected]> wrote:
Quoted 3 lines
What you can do is make that
map_pid_to_cmdlinearray bigger.-- Steve
I am sorry about it, and as the number of cpu cores increases, the current
PID_MAX_DEFAULTis too small, our online machines set thepid_max65536 as default, so the task pid number bigger thanPID_MAX_DEFAULTcan't show the real comm (only <...>), so i want to ajust thePID_MAX_DEFAULTupto 4x, and what do you think?* This controls the default maximum pid allocated to a process */
(quoted diff) +1
-1-#define PID_MAX_DEFAULT (CONFIG_BASE_SMALL ? 0x1000 : 0x8000) +#define PID_MAX_DEFAULT (CONFIG_BASE_SMALL ? 0x1000 : 0x20000)What I was thinking is to make the
map_pid_to_cmdlinearray dynamic (not static), and be set topid_max(afterpid_maxis determined).
Quoted 4 lines
Now,
pid_maxcan be changed at run time. Thus, the tracing code will need to keep a separate variable for that array to store the length. It can not rely onpid_max. But if a pid that is greater thanpid_maxis found, we could kick off a work thread to increase the array.
thanks for your reply, thanks, if map_pid_to_cmdline need dynamic, saved_cmdlines_buffer