: double free in trace extension
 

>> First, just like some other contributors, I've come across an issue
>> triggered by a dump being corrupt. In my case it's this code in
>> kernel.c:cpu_maps_init():
>>
>> * * if (*maskptr & (0x1UL << c)) {
>> * * * *cpu = (i * BITS_PER_LONG) + c;
>> * * * *kt->cpu_flags[cpu] |= mapinfo[m].cpu_flag;
>> * * }
>>
>> The mask is corrupt, making Crash believe there are more CPU's than the
>> four we have allocated space for in kernel.c:kernel_init. How do you
>> think this should be handled?
>
> Does the "crash --cpus <number> ..." command-line option work around it?
>

Nope, setting "--cpus 2" I still arrive at the code above with

(gdb) p/x *maskptr
$3 = 0x540dcebf
(gdb)

As you can see, it's not really a valid cpu mask. Either that or I'm
totally deluded as to the capabilities of the hardware CPU-wise =o)

Regards,
Per

> Dave
>
> --
> Crash-utility mailing list
> Crash-utility@redhat.com
> https://www.redhat.com/mailman/listinfo/crash-utility

--
Crash-utility mailing list
Crash-utility@redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility

: double free in trace extension
 

----- Original Message -----
> >> First, just like some other contributors, I've come across an issue
> >> triggered by a dump being corrupt. In my case it's this code in
> >> kernel.c:cpu_maps_init():
> >>
> >> * * if (*maskptr & (0x1UL << c)) {
> >> * * * *cpu = (i * BITS_PER_LONG) + c;
> >> * * * *kt->cpu_flags[cpu] |= mapinfo[m].cpu_flag;
> >> * * }
> >>
> >> The mask is corrupt, making Crash believe there are more CPU's than the
> >> four we have allocated space for in kernel.c:kernel_init. How do you
> >> think this should be handled?
> >
> > Does the "crash --cpus <number> ..." command-line option work around it?
> >
>
> Nope, setting "--cpus 2" I still arrive at the code above with
>
> (gdb) p/x *maskptr
> $3 = 0x540dcebf
> (gdb)
>
> As you can see, it's not really a valid cpu mask. Either that or I'm
> totally deluded as to the capabilities of the hardware CPU-wise =o)

Right, I understand that cpu_maps_init() will still be called, and that
kt->cpu_flags will be invalid. But then what happens?

Dave

--
Crash-utility mailing list
Crash-utility@redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility

: double free in trace extension
 

On Wed, May 9, 2012 at 3:57 PM, Dave Anderson <anderson@redhat.com> wrote:
>
>
> ----- Original Message -----
>> >> First, just like some other contributors, I've come across an issue
>> >> triggered by a dump being corrupt. In my case it's this code in
>> >> kernel.c:cpu_maps_init():
>> >>
>> >> * * if (*maskptr & (0x1UL << c)) {
>> >> * * * *cpu = (i * BITS_PER_LONG) + c;
>> >> * * * *kt->cpu_flags[cpu] |= mapinfo[m].cpu_flag;
>> >> * * }
>> >>
>> >> The mask is corrupt, making Crash believe there are more CPU's than the
>> >> four we have allocated space for in kernel.c:kernel_init. How do you
>> >> think this should be handled?
>> >
>> > Does the "crash --cpus <number> ..." command-line option work around it?
>> >
>>
>> Nope, setting "--cpus 2" I still arrive at the code above with
>>
>> (gdb) p/x *maskptr
>> $3 = 0x540dcebf
>> (gdb)
>>
>> As you can see, it's not really a valid cpu mask. Either that or I'm
>> totally deluded as to the capabilities of the hardware CPU-wise =o)
>
> Right, I understand that cpu_maps_init() will still be called, and that
> kt->cpu_flags will be invalid. *But then what happens?
>

Well, it will fill in flags for imagined cpus up to #30, since that's
the highest bit set in the mask, using those cpu numbers to index into
a four element large array allocated on the heap, potentially
overwriting stuff it shouldn't. For me, I never actually got any
symptoms from it - I just came across it through valgrind while
investigating the trace extension problem I reported.

/Per

> Dave
>
> --
> Crash-utility mailing list
> Crash-utility@redhat.com
> https://www.redhat.com/mailman/listinfo/crash-utility

--
Crash-utility mailing list
Crash-utility@redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility

: double free in trace extension
 

----- Original Message -----
> On Wed, May 9, 2012 at 3:57 PM, Dave Anderson <anderson@redhat.com>
> wrote:
> >
> >
> > ----- Original Message -----
> >> >> First, just like some other contributors, I've come across an
> >> >> issue
> >> >> triggered by a dump being corrupt. In my case it's this code in
> >> >> kernel.c:cpu_maps_init():
> >> >>
> >> >> * * if (*maskptr & (0x1UL << c)) {
> >> >> * * * *cpu = (i * BITS_PER_LONG) + c;
> >> >> * * * *kt->cpu_flags[cpu] |= mapinfo[m].cpu_flag;
> >> >> * * }
> >> >>
> >> >> The mask is corrupt, making Crash believe there are more CPU's
> >> >> than the
> >> >> four we have allocated space for in kernel.c:kernel_init. How
> >> >> do you
> >> >> think this should be handled?
> >> >
> >> > Does the "crash --cpus <number> ..." command-line option work
> >> > around it?
> >> >
> >>
> >> Nope, setting "--cpus 2" I still arrive at the code above with
> >>
> >> (gdb) p/x *maskptr
> >> $3 = 0x540dcebf
> >> (gdb)
> >>
> >> As you can see, it's not really a valid cpu mask. Either that or I'm
> >> totally deluded as to the capabilities of the hardware CPU-wise =o)
> >
> > Right, I understand that cpu_maps_init() will still be called, and that
> > kt->cpu_flags will be invalid. *But then what happens?
> >
>
> Well, it will fill in flags for imagined cpus up to #30, since that's
> the highest bit set in the mask, using those cpu numbers to index into
> a four element large array allocated on the heap, potentially
> overwriting stuff it shouldn't. For me, I never actually got any
> symptoms from it - I just came across it through valgrind while
> investigating the trace extension problem I reported.
>
> /Per

So then the code should just recognize that the "cpu" value
is beyond the architecture's maximum array index, report the
inconsistency, and "continue" on to the next map?

Dave


--
Crash-utility mailing list
Crash-utility@redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility

: double free in trace extension
 

>
> So then the code should just recognize that the "cpu" value
> is beyond the architecture's maximum array index, report the
> inconsistency, and "continue" on to the next map?
>

Well, that's more or less exactly what I did. See attached patch.

/Per

> Dave
>
>
> --
> Crash-utility mailing list
> Crash-utility@redhat.com
> https://www.redhat.com/mailman/listinfo/crash-utility
--
Crash-utility mailing list
Crash-utility@redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility

: double free in trace extension
 

----- Original Message -----
> >
> > So then the code should just recognize that the "cpu" value
> > is beyond the architecture's maximum array index, report the
> > inconsistency, and "continue" on to the next map?
> >
>
> Well, that's more or less exactly what I did. See attached patch.
>
> /Per

Looks good to me -- queued for crash-6.0.7.

Thanks,
Dave


--
Crash-utility mailing list
Crash-utility@redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility

: double free in trace extension
 

>
> Looks good to me -- queued for crash-6.0.7.
>

Thanks. Any thoughts on the trace extension issue...?

/Per

> Thanks,
> *Dave
>
>
> --
> Crash-utility mailing list
> Crash-utility@redhat.com
> https://www.redhat.com/mailman/listinfo/crash-utility

--
Crash-utility mailing list
Crash-utility@redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility

: double free in trace extension
 

----- Original Message -----
> >
> > Looks good to me -- queued for crash-6.0.7.
> >
>
> Thanks. Any thoughts on the trace extension issue...?
>
> /Per
>

Looks OK to me, but I don't own it. I've added Lai Jiangshan's
email address to this response, but I presume that he won't see
this discussion until China wakes up...

Dave

--
Crash-utility mailing list
Crash-utility@redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility

: double free in trace extension
 

On Wed, May 9, 2012 at 4:59 PM, Dave Anderson <anderson@redhat.com> wrote:
>
>
> ----- Original Message -----
>> >
>> > Looks good to me -- queued for crash-6.0.7.
>> >
>>
>> Thanks. Any thoughts on the trace extension issue...?
>>
>> /Per
>>
>
> Looks OK to me, but I don't own it. *I've added Lai Jiangshan's
> email address to this response, but I presume that he won't see
> this discussion until China wakes up...
>

Ok. Sounds good.

Thanks for your work.

/Per

> Dave
>
> --
> Crash-utility mailing list
> Crash-utility@redhat.com
> https://www.redhat.com/mailman/listinfo/crash-utility

--
Crash-utility mailing list
Crash-utility@redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility

: double free in trace extension
 

>
> Second, I believe there is a double free in the trace extension. When ftrace_init_pages() fails it will free
>
> cpu_buffer->pages
>
> and
>
> cpu_buffer->linear_pages
>
> But when ftrace_init_pages() fails, ftrace_init_buffers() will call ftrace_destroy_buffers() which also free's this space. For me this resulted in a segfault in a malloc() a little later.
>
>

Good.
Acked.

Thanks,
Lai

--
Crash-utility mailing list
Crash-utility@redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility