Skip to contentSkip to conversation

kvm-all: Use 'tmpcpu' instead of 'cpu' in sub-looping to avoid 'cpu' be NULL

PATCH In review Chen Gang 3 participants 4 messages Jul 19, 2014 – Jul 21, 2014 KVM

lore
Share & tools
lore

Chen Gang

author
#0 lore raw

PATCH 1/1 kvm-all: Use 'tmpcpu' instead of 'cpu' in sub-looping to avoid 'cpu' be NULL

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 <[email protected]>
---
 kvm-all.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
kvm-all.c +3 -2
index 3ae30ee..1402f4f 100644
@@ -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;
                 }
             }
Signature · 1 line
1.7.11.7
#1 lore raw
Quoted 24 lines · Chen Gang

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 <[email protected]>
---
 kvm-all.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
kvm-all.c +1 -0
index 3ae30ee..1402f4f 100644
@@ -2077,12 +2077,13 @@ void kvm_remove_all_breakpoints(CPUState *cpu)
 {
     struct kvm_sw_breakpoint *bp, *next;
     KVMState *s = cpu->kvm_state;
+    CPUState *tmpcpu;

kvm-all.c void kvm_remove_all_breakpoints(CPUState *cpu) · line 2089

Quoted 10 lines
kvm-all.c +2 -2
     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

author
#2 lore raw
Quoted 24 lines · Jan Kiszka
Quoted 23 lines · Chen Gang

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 <[email protected]>
---
 kvm-all.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
kvm-all.c +1 -0
index 3ae30ee..1402f4f 100644
@@ -2077,12 +2077,13 @@ void kvm_remove_all_breakpoints(CPUState *cpu)
 {
     struct kvm_sw_breakpoint *bp, *next;
     KVMState *s = cpu->kvm_state;
+    CPUState *tmpcpu;
Quoted 16 lines

kvm-all.c void kvm_remove_all_breakpoints(CPUState *cpu) · line 2089

Quoted 11 lines
kvm-all.c +2 -2
     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.

Signature · 3 lines
Chen Gang

Open share and attitude like air water and life which God blessed
maintainer
#3 lore raw
In reply to Chen Gang

Il 19/07/2014 03:21, Chen Gang ha scritto:

Quoted 24 lines

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 <[email protected]>
---
 kvm-all.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
kvm-all.c +1 -0
index 3ae30ee..1402f4f 100644
@@ -2077,12 +2077,13 @@ void kvm_remove_all_breakpoints(CPUState *cpu)
 {
     struct kvm_sw_breakpoint *bp, *next;
     KVMState *s = cpu->kvm_state;
+    CPUState *tmpcpu;

kvm-all.c void kvm_remove_all_breakpoints(CPUState *cpu) · line 2089

Quoted 10 lines
kvm-all.c +2 -2
     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