* [PATCH] ftrace: unsigned idx cannot be less than 0
@ 2009-01-02 14:49 Roel Kluin
2009-01-02 15:48 ` Frederic Weisbecker
0 siblings, 1 reply; 7+ messages in thread
From: Roel Kluin @ 2009-01-02 14:49 UTC (permalink / raw)
To: Steven Rostedt; +Cc: lkml
// 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--;
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] ftrace: unsigned idx cannot be less than 0
2009-01-02 14:49 [PATCH] ftrace: unsigned idx cannot be less than 0 Roel Kluin
@ 2009-01-02 15:48 ` Frederic Weisbecker
2009-01-02 19:20 ` Roel Kluin
2009-01-06 15:49 ` [PATCH] " Steven Rostedt
0 siblings, 2 replies; 7+ messages in thread
From: Frederic Weisbecker @ 2009-01-02 15:48 UTC (permalink / raw)
To: Roel Kluin; +Cc: Steven Rostedt, lkml, Ingo Molnar
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/
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ftrace: unsigned idx cannot be less than 0
2009-01-02 15:48 ` Frederic Weisbecker
@ 2009-01-02 19:20 ` Roel Kluin
2009-01-02 21:11 ` Frederic Weisbecker
2009-01-06 15:49 ` [PATCH] " Steven Rostedt
1 sibling, 1 reply; 7+ messages in thread
From: Roel Kluin @ 2009-01-02 19:20 UTC (permalink / raw)
To: Frederic Weisbecker; +Cc: Steven Rostedt, lkml, Ingo Molnar
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ftrace: unsigned idx cannot be less than 0
2009-01-02 19:20 ` Roel Kluin
@ 2009-01-02 21:11 ` Frederic Weisbecker
2009-01-03 15:55 ` [PATCH v2] " Roel Kluin
0 siblings, 1 reply; 7+ messages in thread
From: Frederic Weisbecker @ 2009-01-02 21:11 UTC (permalink / raw)
To: Roel Kluin; +Cc: Steven Rostedt, lkml, Ingo Molnar
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2] ftrace: unsigned idx cannot be less than 0
2009-01-02 21:11 ` Frederic Weisbecker
@ 2009-01-03 15:55 ` Roel Kluin
0 siblings, 0 replies; 7+ messages in thread
From: Roel Kluin @ 2009-01-03 15:55 UTC (permalink / raw)
To: Frederic Weisbecker; +Cc: Steven Rostedt, lkml, Ingo Molnar
>>>> - 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;
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] ftrace: unsigned idx cannot be less than 0
2009-01-02 15:48 ` Frederic Weisbecker
2009-01-02 19:20 ` Roel Kluin
@ 2009-01-06 15:49 ` Steven Rostedt
2009-01-06 15:58 ` Frédéric Weisbecker
1 sibling, 1 reply; 7+ messages in thread
From: Steven Rostedt @ 2009-01-06 15:49 UTC (permalink / raw)
To: Frederic Weisbecker; +Cc: Roel Kluin, lkml, Ingo Molnar, Liming Wang
[ 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ftrace: unsigned idx cannot be less than 0
2009-01-06 15:49 ` [PATCH] " Steven Rostedt
@ 2009-01-06 15:58 ` Frédéric Weisbecker
0 siblings, 0 replies; 7+ messages in thread
From: Frédéric Weisbecker @ 2009-01-06 15:58 UTC (permalink / raw)
To: Steven Rostedt; +Cc: Roel Kluin, lkml, Ingo Molnar, Liming Wang
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2009-01-06 15:58 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-01-02 14:49 [PATCH] ftrace: unsigned idx cannot be less than 0 Roel Kluin
2009-01-02 15:48 ` Frederic Weisbecker
2009-01-02 19:20 ` Roel Kluin
2009-01-02 21:11 ` Frederic Weisbecker
2009-01-03 15:55 ` [PATCH v2] " Roel Kluin
2009-01-06 15:49 ` [PATCH] " Steven Rostedt
2009-01-06 15:58 ` Frédéric Weisbecker
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.