[PATCH 0/1] function tracing: fix wrong position computing of stack_trace

[PATCH 0/1] function tracing: fix wrong position computing of stack_trace

[Liming Wang]

Hi Steven and Ingo,

Before I have submitted a patch to fix output of available_filter_functions,
later I checked other places that use seq_operations, I found stack_trace
also has this potential problem.

This fix use a more sane method to implement seq_operations. And this avoids
checking return value of seq_printf. If you think this implementation is OK,
I think others can be convert to this cleaner way, which will make future 
usage of seq_operations less errors.

walimis

[PATCH 1/1] function tracing: fix wrong position computing of stack_trace

[Liming Wang]

Impact: make output of stack_trace complete if buffer overflow

When read buffer overflows, the output of stack_trace isn't complete.

When printing records with seq_printf in t_show, if the read buffer
has overflowed by the current record, then this record won't be printed
to user space through read buffer, it will just be dropped in this printing.

When next printing, t_start should return the "*pos"th record, which is the one
dropped by previous printing, but it just returns (m->private + *pos)th record.

Here we use a more sane method to implement seq_operations which can be found
in kernel code. Thus we needn't initialize m->private.

About testing, it's not easy to overflow read buffer, but we can use
seq_printf to print more padding bytes in t_show, then it's easy to check
whether or not records are lost.

This commit has been tested on both condition of overflow and non overflow.

Signed-off-by: Liming Wang <liming.wang@windriver.com>
---
 kernel/trace/trace_stack.c |   24 +++++++++++++++---------
 1 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/kernel/trace/trace_stack.c b/kernel/trace/trace_stack.c
index d39e8b7..7801052 100644
--- a/kernel/trace/trace_stack.c
+++ b/kernel/trace/trace_stack.c
@@ -180,11 +180,16 @@ static struct file_operations stack_max_size_fops = {
 static void *
 t_next(struct seq_file *m, void *v, loff_t *pos)
 {
-	long i = (long)m->private;
+	long i;
 
 	(*pos)++;
 
-	i++;
+	if (v == SEQ_START_TOKEN )
+		i = 0;
+	else {
+		i = *(long *)v;
+		i++;
+	}
 
 	if (i >= max_stack_trace.nr_entries ||
 	    stack_dump_trace[i] == ULONG_MAX)
@@ -197,12 +202,15 @@ t_next(struct seq_file *m, void *v, loff_t *pos)
 
 static void *t_start(struct seq_file *m, loff_t *pos)
 {
-	void *t = &m->private;
+	void *t = SEQ_START_TOKEN;
 	loff_t l = 0;
 
 	local_irq_disable();
 	__raw_spin_lock(&max_stack_lock);
 
+	if (*pos == 0)
+		return SEQ_START_TOKEN;
+
 	for (; t && l < *pos; t = t_next(m, t, &l))
 		;
 
@@ -231,10 +239,10 @@ static int trace_lookup_stack(struct seq_file *m, long i)
 
 static int t_show(struct seq_file *m, void *v)
 {
-	long i = *(long *)v;
+	long i;
 	int size;
 
-	if (i < 0) {
+	if (v == SEQ_START_TOKEN ) {
 		seq_printf(m, "        Depth   Size      Location"
 			   "    (%d entries)\n"
 			   "        -----   ----      --------\n",
@@ -242,6 +250,8 @@ static int t_show(struct seq_file *m, void *v)
 		return 0;
 	}
 
+	i = *(long *)v;
+
 	if (i >= max_stack_trace.nr_entries ||
 	    stack_dump_trace[i] == ULONG_MAX)
 		return 0;
@@ -271,10 +281,6 @@ static int stack_trace_open(struct inode *inode, struct file *file)
 	int ret;
 
 	ret = seq_open(file, &stack_trace_seq_ops);
-	if (!ret) {
-		struct seq_file *m = file->private_data;
-		m->private = (void *)-1;
-	}
 
 	return ret;
 }
-- 
1.6.0.3

Re: [PATCH 1/1] function tracing: fix wrong position computing of stack_trace

[Ingo Molnar]

* Liming Wang <liming.wang@windriver.com> wrote:

> Impact: make output of stack_trace complete if buffer overflow
> 
> When read buffer overflows, the output of stack_trace isn't 
> complete.
> 
> When printing records with seq_printf in t_show, if the read buffer 
> has overflowed by the current record, then this record won't be 
> printed to user space through read buffer, it will just be dropped 
> in this printing.
> 
> When next printing, t_start should return the "*pos"th record, which 
> is the one dropped by previous printing, but it just returns 
> (m->private + *pos)th record.
> 
> Here we use a more sane method to implement seq_operations which can 
> be found in kernel code. Thus we needn't initialize m->private.
> 
> About testing, it's not easy to overflow read buffer, but we can use 
> seq_printf to print more padding bytes in t_show, then it's easy to 
> check whether or not records are lost.
> 
> This commit has been tested on both condition of overflow and non 
> overflow.
> 
> Signed-off-by: Liming Wang <liming.wang@windriver.com>
> ---
>  kernel/trace/trace_stack.c |   24 +++++++++++++++---------
>  1 files changed, 15 insertions(+), 9 deletions(-)

applied to tip/tracing/urgent, thanks!

Note, i changed 'buffer overflow' to 'buffer overrun'. (buffer 
overflow is a term typically used in a security context)

> -	i++;
> +	if (v == SEQ_START_TOKEN )
> +		i = 0;
> +	else {
> +		i = *(long *)v;
> +		i++;
> +	}

i also fixed these two minor style problems pointed out by 
scripts/checkpatch.pl:

| ERROR: space prohibited before that close parenthesis ')'
| #47: FILE: kernel/trace/trace_stack.c:187:
| +	if (v == SEQ_START_TOKEN )
|
| ERROR: space prohibited before that close parenthesis ')'
| #82: FILE: kernel/trace/trace_stack.c:245:
| +	if (v == SEQ_START_TOKEN ) {
|
| total: 2 errors, 0 warnings, 0 checks, 64 lines checked

	Ingo

Re: [PATCH 1/1] function tracing: fix wrong position computing of stack_trace

[Wang Liming]

Ingo Molnar wrote:
> * Liming Wang <liming.wang@windriver.com> wrote:
> 
>> Impact: make output of stack_trace complete if buffer overflow
>>
>> When read buffer overflows, the output of stack_trace isn't 
>> complete.
>>
>> When printing records with seq_printf in t_show, if the read buffer 
>> has overflowed by the current record, then this record won't be 
>> printed to user space through read buffer, it will just be dropped 
>> in this printing.
>>
>> When next printing, t_start should return the "*pos"th record, which 
>> is the one dropped by previous printing, but it just returns 
>> (m->private + *pos)th record.
>>
>> Here we use a more sane method to implement seq_operations which can 
>> be found in kernel code. Thus we needn't initialize m->private.
>>
>> About testing, it's not easy to overflow read buffer, but we can use 
>> seq_printf to print more padding bytes in t_show, then it's easy to 
>> check whether or not records are lost.
>>
>> This commit has been tested on both condition of overflow and non 
>> overflow.
>>
>> Signed-off-by: Liming Wang <liming.wang@windriver.com>
>> ---
>>  kernel/trace/trace_stack.c |   24 +++++++++++++++---------
>>  1 files changed, 15 insertions(+), 9 deletions(-)
> 
> applied to tip/tracing/urgent, thanks!
> 
> Note, i changed 'buffer overflow' to 'buffer overrun'. (buffer 
> overflow is a term typically used in a security context)
OK.

> 
>> -	i++;
>> +	if (v == SEQ_START_TOKEN )
>> +		i = 0;
>> +	else {
>> +		i = *(long *)v;
>> +		i++;
>> +	}
> 
> i also fixed these two minor style problems pointed out by 
> scripts/checkpatch.pl:
> 
> | ERROR: space prohibited before that close parenthesis ')'
> | #47: FILE: kernel/trace/trace_stack.c:187:
> | +	if (v == SEQ_START_TOKEN )
> |
> | ERROR: space prohibited before that close parenthesis ')'
> | #82: FILE: kernel/trace/trace_stack.c:245:
> | +	if (v == SEQ_START_TOKEN ) {
> |
> | total: 2 errors, 0 warnings, 0 checks, 64 lines checked
It's my fault, thanks a lot!

walimis
> 
> 	Ingo
> 

Re: [PATCH 1/1] function tracing: fix wrong position computing of stack_trace

[Steven Rostedt]

On Fri, 21 Nov 2008, Liming Wang wrote:

> Impact: make output of stack_trace complete if buffer overflow
> 
> When read buffer overflows, the output of stack_trace isn't complete.
> 
> When printing records with seq_printf in t_show, if the read buffer
> has overflowed by the current record, then this record won't be printed
> to user space through read buffer, it will just be dropped in this printing.
> 
> When next printing, t_start should return the "*pos"th record, which is the one
> dropped by previous printing, but it just returns (m->private + *pos)th record.
> 
> Here we use a more sane method to implement seq_operations which can be found
> in kernel code. Thus we needn't initialize m->private.
> 
> About testing, it's not easy to overflow read buffer, but we can use
> seq_printf to print more padding bytes in t_show, then it's easy to check
> whether or not records are lost.
> 
> This commit has been tested on both condition of overflow and non overflow.
> 
> Signed-off-by: Liming Wang <liming.wang@windriver.com>

Acked-by: Steven Rostedt <srostedt@redhat.com>

-- Steve