[PATCH] ftrace: unsigned idx cannot be less than 0

[PATCH] ftrace: unsigned idx cannot be less than 0

[Roel Kluin]

// vi kernel/trace/ftrace.c +787
struct ftrace_iterator {
...
        unsigned                idx;
...
};

idx is unsigned and cannot be less than 0.

Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
---
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 2f32969..a344add 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -842,7 +842,7 @@ static void *t_start(struct seq_file *m, loff_t *pos)
 	void *p = NULL;
 
 	if (*pos > 0) {
-		if (iter->idx < 0)
+		if (iter->idx == 0)
 			return p;
 		(*pos)--;
 		iter->idx--;

Re: [PATCH] ftrace: unsigned idx cannot be less than 0

[Frederic Weisbecker]

On Fri, Jan 02, 2009 at 03:49:43PM +0100, Roel Kluin wrote:
> // vi kernel/trace/ftrace.c +787
> struct ftrace_iterator {
> ...
>         unsigned                idx;
> ...
> };
> 
> idx is unsigned and cannot be less than 0.
> 
> Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
> ---
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index 2f32969..a344add 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -842,7 +842,7 @@ static void *t_start(struct seq_file *m, loff_t *pos)
>  	void *p = NULL;
>  
>  	if (*pos > 0) {
> -		if (iter->idx < 0)
> +		if (iter->idx == 0)
>  			return p;
>  		(*pos)--;
>  		iter->idx--;


Hi Roel,

I'm not sure this is the right fix.
If you look at t_next, if there is no more page to look at,
iter_idx takes -1.

A 0 value would mean: we are in the first index on the page, which means
there is something to read and we don't want to return NULL.

I guess that would be better to turn idx into a signed int.

> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] ftrace: unsigned idx cannot be less than 0

[Roel Kluin]

Frederic Weisbecker wrote:
> On Fri, Jan 02, 2009 at 03:49:43PM +0100, Roel Kluin wrote:
>> // vi kernel/trace/ftrace.c +787
>> struct ftrace_iterator {
>> ...
>>         unsigned                idx;
>> ...
>> };
>>
>> idx is unsigned and cannot be less than 0.
>>
>> Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
>> ---
>> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
>> index 2f32969..a344add 100644
>> --- a/kernel/trace/ftrace.c
>> +++ b/kernel/trace/ftrace.c
>> @@ -842,7 +842,7 @@ static void *t_start(struct seq_file *m, loff_t *pos)
>>  	void *p = NULL;
>>  
>>  	if (*pos > 0) {
>> -		if (iter->idx < 0)
>> +		if (iter->idx == 0)
>>  			return p;
>>  		(*pos)--;
>>  		iter->idx--;
> 
> 
> Hi Roel,
> 
> I'm not sure this is the right fix.
> If you look at t_next, if there is no more page to look at,
> iter_idx takes -1.
> 
> A 0 value would mean: we are in the first index on the page, which means
> there is something to read and we don't want to return NULL.
> 
> I guess that would be better to turn idx into a signed int.

If we turn idx in a signed int, isn't it true that
in kernel/trace/ftrace.c, line 806:

retry:
if (iter->idx >= iter->pg->index) {
	...
} else {
	iter->idx++;
	if ( a certain rec-> and iter->flags )
		goto retry;
}

since iter->pg->index is an unsigned long, when larger than INT_MAX this
could result in an endless loop?

Roel

Re: [PATCH] ftrace: unsigned idx cannot be less than 0

[Frederic Weisbecker]

On Fri, Jan 02, 2009 at 08:20:21PM +0100, Roel Kluin wrote:
> Frederic Weisbecker wrote:
> > On Fri, Jan 02, 2009 at 03:49:43PM +0100, Roel Kluin wrote:
> >> // vi kernel/trace/ftrace.c +787
> >> struct ftrace_iterator {
> >> ...
> >>         unsigned                idx;
> >> ...
> >> };
> >>
> >> idx is unsigned and cannot be less than 0.
> >>
> >> Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
> >> ---
> >> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> >> index 2f32969..a344add 100644
> >> --- a/kernel/trace/ftrace.c
> >> +++ b/kernel/trace/ftrace.c
> >> @@ -842,7 +842,7 @@ static void *t_start(struct seq_file *m, loff_t *pos)
> >>  	void *p = NULL;
> >>  
> >>  	if (*pos > 0) {
> >> -		if (iter->idx < 0)
> >> +		if (iter->idx == 0)
> >>  			return p;
> >>  		(*pos)--;
> >>  		iter->idx--;
> > 
> > 
> > Hi Roel,
> > 
> > I'm not sure this is the right fix.
> > If you look at t_next, if there is no more page to look at,
> > iter_idx takes -1.
> > 
> > A 0 value would mean: we are in the first index on the page, which means
> > there is something to read and we don't want to return NULL.
> > 
> > I guess that would be better to turn idx into a signed int.
> 
> If we turn idx in a signed int, isn't it true that
> in kernel/trace/ftrace.c, line 806:
> 
> retry:
> if (iter->idx >= iter->pg->index) {
> 	...
> } else {
> 	iter->idx++;
> 	if ( a certain rec-> and iter->flags )
> 		goto retry;
> }
> 
> since iter->pg->index is an unsigned long, when larger than INT_MAX this
> could result in an endless loop?
> 
> Roel


Actually, this is not supposed to reach such a threshold.
Looks like it wouldn't increase over ENTRIES_PER_PAGE (defined
in ftrace.c) which is smaller than PAGE_SIZE.
So it will stay far from an overflow.

I don't think this type conversion would be an issue. But perhaps
there are other things that I don't see.

[PATCH v2] ftrace: unsigned idx cannot be less than 0

[Roel Kluin]

>>>> -		if (iter->idx < 0)
>>>> +		if (iter->idx == 0)

>>> I'm not sure this is the right fix.
>>> If you look at t_next, if there is no more page to look at,
>>> iter_idx takes -1.
>>>
>>> A 0 value would mean: we are in the first index on the page, which means
>>> there is something to read and we don't want to return NULL.
>>>
>>> I guess that would be better to turn idx into a signed int.

>> If we turn idx in a signed int, isn't it true that
>> in kernel/trace/ftrace.c, line 806:

>> since iter->pg->index is an unsigned long, when larger than INT_MAX this
>> could result in an endless loop?
> 
> Actually, this is not supposed to reach such a threshold.
> Looks like it wouldn't increase over ENTRIES_PER_PAGE (defined
> in ftrace.c) which is smaller than PAGE_SIZE.
> So it will stay far from an overflow.

unless signed idx cannot become less than 0

Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
---
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 2f32969..e256648 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -786,7 +786,7 @@ enum {
 
 struct ftrace_iterator {
 	struct ftrace_page	*pg;
-	unsigned		idx;
+	int			idx;
 	unsigned		flags;
 	unsigned char		buffer[FTRACE_BUFF_MAX+1];
 	unsigned		buffer_idx;

Re: [PATCH] ftrace: unsigned idx cannot be less than 0

[Steven Rostedt]

[ added Liming to CC ]

On Fri, 2 Jan 2009, Frederic Weisbecker wrote:

> On Fri, Jan 02, 2009 at 03:49:43PM +0100, Roel Kluin wrote:
> > // vi kernel/trace/ftrace.c +787
> > struct ftrace_iterator {
> > ...
> >         unsigned                idx;
> > ...
> > };
> > 
> > idx is unsigned and cannot be less than 0.
> > 
> > Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
> > ---
> > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> > index 2f32969..a344add 100644
> > --- a/kernel/trace/ftrace.c
> > +++ b/kernel/trace/ftrace.c
> > @@ -842,7 +842,7 @@ static void *t_start(struct seq_file *m, loff_t *pos)
> >  	void *p = NULL;
> >  
> >  	if (*pos > 0) {
> > -		if (iter->idx < 0)
> > +		if (iter->idx == 0)
> >  			return p;
> >  		(*pos)--;
> >  		iter->idx--;
> 
> 
> Hi Roel,
> 
> I'm not sure this is the right fix.
> If you look at t_next, if there is no more page to look at,
> iter_idx takes -1.
> 
> A 0 value would mean: we are in the first index on the page, which means
> there is something to read and we don't want to return NULL.
> 
> I guess that would be better to turn idx into a signed int.

Correct. This bug was added by:

  50cdaf08a8ec1d7f43987705da7aff7cf949708f
  ftrace: improve seq_operation of ftrace

So the correct fix is to turn it into a signed int.

Thanks,

-- Steve

Re: [PATCH] ftrace: unsigned idx cannot be less than 0

[Frédéric Weisbecker]

2009/1/6 Steven Rostedt <rostedt@goodmis.org>:
>
> [ added Liming to CC ]
>
> On Fri, 2 Jan 2009, Frederic Weisbecker wrote:
>
>> On Fri, Jan 02, 2009 at 03:49:43PM +0100, Roel Kluin wrote:
>> > // vi kernel/trace/ftrace.c +787
>> > struct ftrace_iterator {
>> > ...
>> >         unsigned                idx;
>> > ...
>> > };
>> >
>> > idx is unsigned and cannot be less than 0.
>> >
>> > Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
>> > ---
>> > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
>> > index 2f32969..a344add 100644
>> > --- a/kernel/trace/ftrace.c
>> > +++ b/kernel/trace/ftrace.c
>> > @@ -842,7 +842,7 @@ static void *t_start(struct seq_file *m, loff_t *pos)
>> >     void *p = NULL;
>> >
>> >     if (*pos > 0) {
>> > -           if (iter->idx < 0)
>> > +           if (iter->idx == 0)
>> >                     return p;
>> >             (*pos)--;
>> >             iter->idx--;
>>
>>
>> Hi Roel,
>>
>> I'm not sure this is the right fix.
>> If you look at t_next, if there is no more page to look at,
>> iter_idx takes -1.
>>
>> A 0 value would mean: we are in the first index on the page, which means
>> there is something to read and we don't want to return NULL.
>>
>> I guess that would be better to turn idx into a signed int.
>
> Correct. This bug was added by:
>
>  50cdaf08a8ec1d7f43987705da7aff7cf949708f
>  ftrace: improve seq_operation of ftrace
>
> So the correct fix is to turn it into a signed int.
>
> Thanks,
>
> -- Steve
>

Both iter->idx and pg->index should be signed int I guess.

Roel is perhaps right, I don't know how is caught an unsigned long
when compared with a signed int if
this int is less than 0.