[PATCH] ftrace: fix task's invalid comm of <...> when big pid

[PATCH] ftrace: fix task's invalid comm of <...> when big pid

[Wang Yu]

when pid is bigger than PID_MAX_DEFAULT, the comm of task
is <...>, it is better use pid_max to compare

Signed-off-by: Wang Yu <yuwang@linux.alibaba.com>
---
 kernel/trace/trace.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
 mode change 100644 => 100755 kernel/trace/trace.c

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
old mode 100644
new mode 100755
index 20a2300..0d4bc7a
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -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;
 	}
-- 
1.8.3.1

Re: [PATCH] ftrace: fix task's invalid comm of <...> when big pid

[Steven Rostedt]

On Wed, 28 Mar 2018 20:32:27 +0800
Wang Yu <yuwang@linux.alibaba.com> wrote:

> when pid is bigger than PID_MAX_DEFAULT, the comm of task
> is <...>, it is better use pid_max to compare
> 
> Signed-off-by: Wang Yu <yuwang@linux.alibaba.com>
> ---
>  kernel/trace/trace.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>  mode change 100644 => 100755 kernel/trace/trace.c
> 
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> old mode 100644
> new mode 100755
> index 20a2300..0d4bc7a
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -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

>  		strcpy(comm, "<...>");
>  		return;
>  	}

Re: [PATCH] ftrace: fix task's invalid comm of <...> when big pid

[Steven Rostedt]

On Wed, 28 Mar 2018 11:35:22 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Wed, 28 Mar 2018 20:32:27 +0800
> Wang Yu <yuwang@linux.alibaba.com> wrote:
> 
> > when pid is bigger than PID_MAX_DEFAULT, the comm of task
> > is <...>, it is better use pid_max to compare
> > 
> > Signed-off-by: Wang Yu <yuwang@linux.alibaba.com>
> > ---
> >  kernel/trace/trace.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >  mode change 100644 => 100755 kernel/trace/trace.c
> > 
> > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> > old mode 100644
> > new mode 100755
> > index 20a2300..0d4bc7a
> > --- a/kernel/trace/trace.c
> > +++ b/kernel/trace/trace.c
> > @@ -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.

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


> 
> -- Steve
> 
> >  		strcpy(comm, "<...>");
> >  		return;
> >  	}  
> 

Re: [PATCH] ftrace: fix task's invalid comm of <...> when big pid

[Wang Yu]

于 18/3/28 下午11:44, Steven Rostedt 写道:
> On Wed, 28 Mar 2018 11:35:22 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
>
>> On Wed, 28 Mar 2018 20:32:27 +0800
>> Wang Yu <yuwang@linux.alibaba.com> wrote:
>>
>>> when pid is bigger than PID_MAX_DEFAULT, the comm of task
>>> is <...>, it is better use pid_max to compare
>>>
>>> Signed-off-by: Wang Yu <yuwang@linux.alibaba.com>
>>> ---
>>>   kernel/trace/trace.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>   mode change 100644 => 100755 kernel/trace/trace.c
>>>
>>> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
>>> old mode 100644
>>> new mode 100755
>>> index 20a2300..0d4bc7a
>>> --- a/kernel/trace/trace.c
>>> +++ b/kernel/trace/trace.c
>>> @@ -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.
> 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

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)

>   
>
>> -- Steve
>>
>>>   		strcpy(comm, "<...>");
>>>   		return;
>>>   	}

Re: [PATCH] ftrace: fix task's invalid comm of <...> when big pid

[Steven Rostedt]

On Thu, 29 Mar 2018 10:16:22 +0800
Wang Yu <yuwang@linux.alibaba.com> wrote:

> > What you can do is make that map_pid_to_cmdline array 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)

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

Re: [PATCH] ftrace: fix task's invalid comm of <...> when big pid

[Wang Yu]

在 18/3/29 下午10:26, Steven Rostedt 写道:
> On Thu, 29 Mar 2018 10:16:22 +0800
> Wang Yu <yuwang@linux.alibaba.com> wrote:
>
>>> What you can do is make that map_pid_to_cmdline array 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)
> 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.
thanks for your reply, thanks, if map_pid_to_cmdline need dynamic, 
saved_cmdlines_buffer