| Message ID | 53C9C82A.2060003@gmail.com |
|---|---|
| State | New |
| Headers | show |
On 2014-07-19 03:21, Chen Gang wrote: > If kvm_arch_remove_sw_breakpoint() in CPU_FOREACH() always be fail, it > will let 'cpu' NULL. And the next kvm_arch_remove_sw_breakpoint() in > QTAILQ_FOREACH_SAFE() will get NULL parameter for 'cpu'. > > And kvm_arch_remove_sw_breakpoint() can assumes 'cpu' must never be NULL, > so need define additional temporary variable for 'cpu' to avoid the case. > > > Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com> > --- > kvm-all.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/kvm-all.c b/kvm-all.c > index 3ae30ee..1402f4f 100644 > --- a/kvm-all.c > +++ b/kvm-all.c > @@ -2077,12 +2077,13 @@ void kvm_remove_all_breakpoints(CPUState *cpu) > { > struct kvm_sw_breakpoint *bp, *next; > KVMState *s = cpu->kvm_state; > + CPUState *tmpcpu; > > QTAILQ_FOREACH_SAFE(bp, &s->kvm_sw_breakpoints, entry, next) { > if (kvm_arch_remove_sw_breakpoint(cpu, bp) != 0) { > /* Try harder to find a CPU that currently sees the breakpoint. */ > - CPU_FOREACH(cpu) { > - if (kvm_arch_remove_sw_breakpoint(cpu, bp) == 0) { > + CPU_FOREACH(tmpcpu) { > + if (kvm_arch_remove_sw_breakpoint(tmpcpu, bp) == 0) { > break; > } > } > Good catch. To make it clear in the changelog: The actual issue is that we misuse "cpu" as an iteration variable while its original value is still in use. That cpu can eventually become NULL this way is one result. Jan
On 07/20/2014 03:29 PM, Jan Kiszka wrote: > On 2014-07-19 03:21, Chen Gang wrote: >> If kvm_arch_remove_sw_breakpoint() in CPU_FOREACH() always be fail, it >> will let 'cpu' NULL. And the next kvm_arch_remove_sw_breakpoint() in >> QTAILQ_FOREACH_SAFE() will get NULL parameter for 'cpu'. >> >> And kvm_arch_remove_sw_breakpoint() can assumes 'cpu' must never be NULL, >> so need define additional temporary variable for 'cpu' to avoid the case. >> >> >> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com> >> --- >> kvm-all.c | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/kvm-all.c b/kvm-all.c >> index 3ae30ee..1402f4f 100644 >> --- a/kvm-all.c >> +++ b/kvm-all.c >> @@ -2077,12 +2077,13 @@ void kvm_remove_all_breakpoints(CPUState *cpu) >> { >> struct kvm_sw_breakpoint *bp, *next; >> KVMState *s = cpu->kvm_state; >> + CPUState *tmpcpu; >> >> QTAILQ_FOREACH_SAFE(bp, &s->kvm_sw_breakpoints, entry, next) { >> if (kvm_arch_remove_sw_breakpoint(cpu, bp) != 0) { >> /* Try harder to find a CPU that currently sees the breakpoint. */ >> - CPU_FOREACH(cpu) { >> - if (kvm_arch_remove_sw_breakpoint(cpu, bp) == 0) { >> + CPU_FOREACH(tmpcpu) { >> + if (kvm_arch_remove_sw_breakpoint(tmpcpu, bp) == 0) { >> break; >> } >> } >> > > Good catch. To make it clear in the changelog: The actual issue is that > we misuse "cpu" as an iteration variable while its original value is > still in use. That cpu can eventually become NULL this way is one result. > OK, thanks. If necessary, I shall send patch v2 for additional comments. (if really necessary to send, please let me know) Thanks.
Il 19/07/2014 03:21, Chen Gang ha scritto: > If kvm_arch_remove_sw_breakpoint() in CPU_FOREACH() always be fail, it > will let 'cpu' NULL. And the next kvm_arch_remove_sw_breakpoint() in > QTAILQ_FOREACH_SAFE() will get NULL parameter for 'cpu'. > > And kvm_arch_remove_sw_breakpoint() can assumes 'cpu' must never be NULL, > so need define additional temporary variable for 'cpu' to avoid the case. > > > Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com> > --- > kvm-all.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/kvm-all.c b/kvm-all.c > index 3ae30ee..1402f4f 100644 > --- a/kvm-all.c > +++ b/kvm-all.c > @@ -2077,12 +2077,13 @@ void kvm_remove_all_breakpoints(CPUState *cpu) > { > struct kvm_sw_breakpoint *bp, *next; > KVMState *s = cpu->kvm_state; > + CPUState *tmpcpu; > > QTAILQ_FOREACH_SAFE(bp, &s->kvm_sw_breakpoints, entry, next) { > if (kvm_arch_remove_sw_breakpoint(cpu, bp) != 0) { > /* Try harder to find a CPU that currently sees the breakpoint. */ > - CPU_FOREACH(cpu) { > - if (kvm_arch_remove_sw_breakpoint(cpu, bp) == 0) { > + CPU_FOREACH(tmpcpu) { > + if (kvm_arch_remove_sw_breakpoint(tmpcpu, bp) == 0) { > break; > } > } > Applying to uq/master, thanks. Paolo
diff --git a/kvm-all.c b/kvm-all.c index 3ae30ee..1402f4f 100644 --- a/kvm-all.c +++ b/kvm-all.c @@ -2077,12 +2077,13 @@ void kvm_remove_all_breakpoints(CPUState *cpu) { struct kvm_sw_breakpoint *bp, *next; KVMState *s = cpu->kvm_state; + CPUState *tmpcpu; QTAILQ_FOREACH_SAFE(bp, &s->kvm_sw_breakpoints, entry, next) { if (kvm_arch_remove_sw_breakpoint(cpu, bp) != 0) { /* Try harder to find a CPU that currently sees the breakpoint. */ - CPU_FOREACH(cpu) { - if (kvm_arch_remove_sw_breakpoint(cpu, bp) == 0) { + CPU_FOREACH(tmpcpu) { + if (kvm_arch_remove_sw_breakpoint(tmpcpu, bp) == 0) { break; } }
If kvm_arch_remove_sw_breakpoint() in CPU_FOREACH() always be fail, it will let 'cpu' NULL. And the next kvm_arch_remove_sw_breakpoint() in QTAILQ_FOREACH_SAFE() will get NULL parameter for 'cpu'. And kvm_arch_remove_sw_breakpoint() can assumes 'cpu' must never be NULL, so need define additional temporary variable for 'cpu' to avoid the case. Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com> --- kvm-all.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)