[PATCH 0/9] KVM: Fine-tuning for several function implementations

[PATCH 0/9] KVM: Fine-tuning for several function implementations

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 22 Jan 2017 19:06:54 +0100

Several update suggestions were taken into account
from static source code analysis.

Markus Elfring (9):
  Return directly after a failed copy_from_user() in kvm_vm_compat_ioctl()
  Move error code settings in kvm_vm_ioctl()
  Move error code settings in kvm_vcpu_compat_ioctl()
  Move error code settings in kvm_vcpu_ioctl()
  Improve size determinations in kvm_vcpu_ioctl()
  Return an error code only as a constant in kvm_get_dirty_log_protect()
  Return an error code only as a constant in kvm_get_dirty_log()
  Adjust seven checks for null pointers
  Improve another size determination in kvm_create_vm()

 virt/kvm/kvm_main.c | 278 +++++++++++++++++++++++-----------------------------
 1 file changed, 120 insertions(+), 158 deletions(-)

-- 
2.11.0

[PATCH 1/9] KVM: Return directly after a failed copy_from_user() in kvm_vm_compat_ioctl()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 22 Jan 2017 11:30:21 +0100

* Return directly after a call of the function "copy_from_user" failed
  in a case block.

  This issue was detected by using the Coccinelle software.

* Delete the jump label "out" which became unnecessary with
  this refactoring.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 virt/kvm/kvm_main.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index de102cae7125..eeb340708d97 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -3133,10 +3133,9 @@ static long kvm_vm_compat_ioctl(struct file *filp,
 		struct compat_kvm_dirty_log compat_log;
 		struct kvm_dirty_log log;
 
-		r = -EFAULT;
 		if (copy_from_user(&compat_log, (void __user *)arg,
 				   sizeof(compat_log)))
-			goto out;
+			return -EFAULT;
 		log.slot	 = compat_log.slot;
 		log.padding1	 = compat_log.padding1;
 		log.padding2	 = compat_log.padding2;
@@ -3148,8 +3147,6 @@ static long kvm_vm_compat_ioctl(struct file *filp,
 	default:
 		r = kvm_vm_ioctl(filp, ioctl, arg);
 	}
-
-out:
 	return r;
 }
 #endif
-- 
2.11.0

[PATCH 2/9] KVM: Move error code settings in kvm_vm_ioctl()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 22 Jan 2017 13:45:18 +0100

* A local variable was set to an error code before a concrete error
  situation was detected. Thus move the corresponding assignments
  into if branches to indicate a software failure there.

  This issue was detected by using the Coccinelle software.

* Return directly after a call of the function "copy_from_user"
  (or two other checks) failed in a case block.

* Delete the jump label "out" and two zero assignments which became
  unnecessary with this refactoring.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 virt/kvm/kvm_main.c | 66 ++++++++++++++++++-----------------------------------
 1 file changed, 22 insertions(+), 44 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index eeb340708d97..2773e5012948 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -2960,20 +2960,17 @@ static long kvm_vm_ioctl(struct file *filp,
 	case KVM_SET_USER_MEMORY_REGION: {
 		struct kvm_userspace_memory_region kvm_userspace_mem;
 
-		r = -EFAULT;
 		if (copy_from_user(&kvm_userspace_mem, argp,
 						sizeof(kvm_userspace_mem)))
-			goto out;
-
+			return -EFAULT;
 		r = kvm_vm_ioctl_set_memory_region(kvm, &kvm_userspace_mem);
 		break;
 	}
 	case KVM_GET_DIRTY_LOG: {
 		struct kvm_dirty_log log;
 
-		r = -EFAULT;
 		if (copy_from_user(&log, argp, sizeof(log)))
-			goto out;
+			return -EFAULT;
 		r = kvm_vm_ioctl_get_dirty_log(kvm, &log);
 		break;
 	}
@@ -2981,18 +2978,16 @@ static long kvm_vm_ioctl(struct file *filp,
 	case KVM_REGISTER_COALESCED_MMIO: {
 		struct kvm_coalesced_mmio_zone zone;
 
-		r = -EFAULT;
 		if (copy_from_user(&zone, argp, sizeof(zone)))
-			goto out;
+			return -EFAULT;
 		r = kvm_vm_ioctl_register_coalesced_mmio(kvm, &zone);
 		break;
 	}
 	case KVM_UNREGISTER_COALESCED_MMIO: {
 		struct kvm_coalesced_mmio_zone zone;
 
-		r = -EFAULT;
 		if (copy_from_user(&zone, argp, sizeof(zone)))
-			goto out;
+			return -EFAULT;
 		r = kvm_vm_ioctl_unregister_coalesced_mmio(kvm, &zone);
 		break;
 	}
@@ -3000,18 +2995,16 @@ static long kvm_vm_ioctl(struct file *filp,
 	case KVM_IRQFD: {
 		struct kvm_irqfd data;
 
-		r = -EFAULT;
 		if (copy_from_user(&data, argp, sizeof(data)))
-			goto out;
+			return -EFAULT;
 		r = kvm_irqfd(kvm, &data);
 		break;
 	}
 	case KVM_IOEVENTFD: {
 		struct kvm_ioeventfd data;
 
-		r = -EFAULT;
 		if (copy_from_user(&data, argp, sizeof(data)))
-			goto out;
+			return -EFAULT;
 		r = kvm_ioeventfd(kvm, &data);
 		break;
 	}
@@ -3019,9 +3012,8 @@ static long kvm_vm_ioctl(struct file *filp,
 	case KVM_SIGNAL_MSI: {
 		struct kvm_msi msi;
 
-		r = -EFAULT;
 		if (copy_from_user(&msi, argp, sizeof(msi)))
-			goto out;
+			return -EFAULT;
 		r = kvm_send_userspace_msi(kvm, &msi);
 		break;
 	}
@@ -3031,22 +3023,17 @@ static long kvm_vm_ioctl(struct file *filp,
 	case KVM_IRQ_LINE: {
 		struct kvm_irq_level irq_event;
 
-		r = -EFAULT;
 		if (copy_from_user(&irq_event, argp, sizeof(irq_event)))
-			goto out;
+			return -EFAULT;
 
 		r = kvm_vm_ioctl_irq_line(kvm, &irq_event,
 					ioctl == KVM_IRQ_LINE_STATUS);
 		if (r)
-			goto out;
-
-		r = -EFAULT;
-		if (ioctl == KVM_IRQ_LINE_STATUS) {
-			if (copy_to_user(argp, &irq_event, sizeof(irq_event)))
-				goto out;
-		}
+			return r;
 
-		r = 0;
+		if (ioctl == KVM_IRQ_LINE_STATUS &&
+		    copy_to_user(argp, &irq_event, sizeof(irq_event)))
+			return -EFAULT;
 		break;
 	}
 #endif
@@ -3056,24 +3043,20 @@ static long kvm_vm_ioctl(struct file *filp,
 		struct kvm_irq_routing __user *urouting;
 		struct kvm_irq_routing_entry *entries = NULL;
 
-		r = -EFAULT;
 		if (copy_from_user(&routing, argp, sizeof(routing)))
-			goto out;
-		r = -EINVAL;
-		if (routing.nr > KVM_MAX_IRQ_ROUTES)
-			goto out;
-		if (routing.flags)
-			goto out;
+			return -EFAULT;
+		if (routing.nr > KVM_MAX_IRQ_ROUTES || routing.flags)
+			return -EINVAL;
 		if (routing.nr) {
-			r = -ENOMEM;
 			entries = vmalloc(routing.nr * sizeof(*entries));
 			if (!entries)
-				goto out;
-			r = -EFAULT;
+				return -ENOMEM;
 			urouting = argp;
 			if (copy_from_user(entries, urouting->entries,
-					   routing.nr * sizeof(*entries)))
+					   routing.nr * sizeof(*entries))) {
+				r = -EFAULT;
 				goto out_free_irq_routing;
+			}
 		}
 		r = kvm_set_irq_routing(kvm, entries, routing.nr,
 					routing.flags);
@@ -3085,19 +3068,15 @@ static long kvm_vm_ioctl(struct file *filp,
 	case KVM_CREATE_DEVICE: {
 		struct kvm_create_device cd;
 
-		r = -EFAULT;
 		if (copy_from_user(&cd, argp, sizeof(cd)))
-			goto out;
+			return -EFAULT;
 
 		r = kvm_ioctl_create_device(kvm, &cd);
 		if (r)
-			goto out;
+			return r;
 
-		r = -EFAULT;
 		if (copy_to_user(argp, &cd, sizeof(cd)))
-			goto out;
-
-		r = 0;
+			return -EFAULT;
 		break;
 	}
 	case KVM_CHECK_EXTENSION:
@@ -3106,7 +3085,6 @@ static long kvm_vm_ioctl(struct file *filp,
 	default:
 		r = kvm_arch_vm_ioctl(filp, ioctl, arg);
 	}
-out:
 	return r;
 }
 
-- 
2.11.0

[PATCH 3/9] KVM: Move error code settings in kvm_vcpu_compat_ioctl()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 22 Jan 2017 13:57:40 +0100

* A local variable was set to an error code before a concrete error
  situation was detected. Thus move the corresponding settings
  into if branches to indicate a software failure there.

  This issue was detected by using the Coccinelle software.

* Return directly after a call of the function "copy_from_user"
  (or an other check) failed in a case block.

* Delete the jump label "out" which became unnecessary with
  this refactoring.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 virt/kvm/kvm_main.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 2773e5012948..62f24d8eaaa2 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -2743,17 +2743,14 @@ static long kvm_vcpu_compat_ioctl(struct file *filp,
 		sigset_t sigset;
 
 		if (argp) {
-			r = -EFAULT;
 			if (copy_from_user(&kvm_sigmask, argp,
 					   sizeof(kvm_sigmask)))
-				goto out;
-			r = -EINVAL;
+				return -EFAULT;
 			if (kvm_sigmask.len != sizeof(csigset))
-				goto out;
-			r = -EFAULT;
+				return -EINVAL;
 			if (copy_from_user(&csigset, sigmask_arg->sigset,
 					   sizeof(csigset)))
-				goto out;
+				return -EFAULT;
 			sigset_from_compat(&sigset, &csigset);
 			r = kvm_vcpu_ioctl_set_sigmask(vcpu, &sigset);
 		} else
@@ -2763,8 +2760,6 @@ static long kvm_vcpu_compat_ioctl(struct file *filp,
 	default:
 		r = kvm_vcpu_ioctl(filp, ioctl, arg);
 	}
-
-out:
 	return r;
 }
 #endif
-- 
2.11.0

[PATCH 4/9] KVM: Move error code settings in kvm_vcpu_ioctl()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 22 Jan 2017 17:00:19 +0100

* A local variable was set to an error code before a concrete error
  situation was detected. Thus move the corresponding assignments
  into if branches to indicate a software failure there.

  This issue was detected by using the Coccinelle software.

* The kfree() function was called in some cases by the
  kvm_vcpu_ioctl() function even if the passed variable contained
  a null pointer.

  Adjust jump targets according to the Linux coding style convention.

  Move the definition for two variables into case blocks
  so that extra initialisations can be avoided at the beginning.

* Delete the jump label "out" and seven zero assignments which became
  unnecessary with this refactoring.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 virt/kvm/kvm_main.c | 131 +++++++++++++++++++++++++++-------------------------
 1 file changed, 69 insertions(+), 62 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 62f24d8eaaa2..9d463b7a3912 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -2527,8 +2527,6 @@ static long kvm_vcpu_ioctl(struct file *filp,
 	struct kvm_vcpu *vcpu = filp->private_data;
 	void __user *argp = (void __user *)arg;
 	int r;
-	struct kvm_fpu *fpu = NULL;
-	struct kvm_sregs *kvm_sregs = NULL;
 
 	if (vcpu->kvm->mm != current->mm)
 		return -EIO;
@@ -2551,9 +2549,10 @@ static long kvm_vcpu_ioctl(struct file *filp,
 		return r;
 	switch (ioctl) {
 	case KVM_RUN:
-		r = -EINVAL;
-		if (arg)
-			goto out;
+		if (arg) {
+			r = -EINVAL;
+			goto put_vcpu;
+		}
 		if (unlikely(vcpu->pid != current->pids[PIDTYPE_PID].pid)) {
 			/* The thread running this VCPU changed. */
 			struct pid *oldpid = vcpu->pid;
@@ -2570,56 +2569,59 @@ static long kvm_vcpu_ioctl(struct file *filp,
 	case KVM_GET_REGS: {
 		struct kvm_regs *kvm_regs;
 
-		r = -ENOMEM;
 		kvm_regs = kzalloc(sizeof(struct kvm_regs), GFP_KERNEL);
-		if (!kvm_regs)
-			goto out;
+		if (!kvm_regs) {
+			r = -ENOMEM;
+			goto put_vcpu;
+		}
 		r = kvm_arch_vcpu_ioctl_get_regs(vcpu, kvm_regs);
 		if (r)
-			goto out_free1;
-		r = -EFAULT;
+			goto free_regs;
 		if (copy_to_user(argp, kvm_regs, sizeof(struct kvm_regs)))
-			goto out_free1;
-		r = 0;
-out_free1:
+			r = -EFAULT;
+free_regs:
 		kfree(kvm_regs);
 		break;
 	}
 	case KVM_SET_REGS: {
 		struct kvm_regs *kvm_regs;
 
-		r = -ENOMEM;
 		kvm_regs = memdup_user(argp, sizeof(*kvm_regs));
 		if (IS_ERR(kvm_regs)) {
 			r = PTR_ERR(kvm_regs);
-			goto out;
+			goto put_vcpu;
 		}
 		r = kvm_arch_vcpu_ioctl_set_regs(vcpu, kvm_regs);
 		kfree(kvm_regs);
 		break;
 	}
 	case KVM_GET_SREGS: {
+		struct kvm_sregs *kvm_sregs;
+
 		kvm_sregs = kzalloc(sizeof(struct kvm_sregs), GFP_KERNEL);
-		r = -ENOMEM;
-		if (!kvm_sregs)
-			goto out;
+		if (!kvm_sregs) {
+			r = -ENOMEM;
+			goto put_vcpu;
+		}
 		r = kvm_arch_vcpu_ioctl_get_sregs(vcpu, kvm_sregs);
 		if (r)
-			goto out;
-		r = -EFAULT;
+			goto free_sregs;
 		if (copy_to_user(argp, kvm_sregs, sizeof(struct kvm_sregs)))
-			goto out;
-		r = 0;
+			r = -EFAULT;
+free_sregs:
+		kfree(kvm_sregs);
 		break;
 	}
 	case KVM_SET_SREGS: {
+		struct kvm_sregs *kvm_sregs;
+
 		kvm_sregs = memdup_user(argp, sizeof(*kvm_sregs));
 		if (IS_ERR(kvm_sregs)) {
 			r = PTR_ERR(kvm_sregs);
-			kvm_sregs = NULL;
-			goto out;
+			goto put_vcpu;
 		}
 		r = kvm_arch_vcpu_ioctl_set_sregs(vcpu, kvm_sregs);
+		kfree(kvm_sregs);
 		break;
 	}
 	case KVM_GET_MP_STATE: {
@@ -2627,43 +2629,42 @@ static long kvm_vcpu_ioctl(struct file *filp,
 
 		r = kvm_arch_vcpu_ioctl_get_mpstate(vcpu, &mp_state);
 		if (r)
-			goto out;
-		r = -EFAULT;
+			goto put_vcpu;
 		if (copy_to_user(argp, &mp_state, sizeof(mp_state)))
-			goto out;
-		r = 0;
+			r = -EFAULT;
 		break;
 	}
 	case KVM_SET_MP_STATE: {
 		struct kvm_mp_state mp_state;
 
-		r = -EFAULT;
-		if (copy_from_user(&mp_state, argp, sizeof(mp_state)))
-			goto out;
+		if (copy_from_user(&mp_state, argp, sizeof(mp_state))) {
+			r = -EFAULT;
+			goto put_vcpu;
+		}
 		r = kvm_arch_vcpu_ioctl_set_mpstate(vcpu, &mp_state);
 		break;
 	}
 	case KVM_TRANSLATE: {
 		struct kvm_translation tr;
 
-		r = -EFAULT;
-		if (copy_from_user(&tr, argp, sizeof(tr)))
-			goto out;
+		if (copy_from_user(&tr, argp, sizeof(tr))) {
+			r = -EFAULT;
+			goto put_vcpu;
+		}
 		r = kvm_arch_vcpu_ioctl_translate(vcpu, &tr);
 		if (r)
-			goto out;
-		r = -EFAULT;
+			goto put_vcpu;
 		if (copy_to_user(argp, &tr, sizeof(tr)))
-			goto out;
-		r = 0;
+			r = -EFAULT;
 		break;
 	}
 	case KVM_SET_GUEST_DEBUG: {
 		struct kvm_guest_debug dbg;
 
-		r = -EFAULT;
-		if (copy_from_user(&dbg, argp, sizeof(dbg)))
-			goto out;
+		if (copy_from_user(&dbg, argp, sizeof(dbg))) {
+			r = -EFAULT;
+			goto put_vcpu;
+		}
 		r = kvm_arch_vcpu_ioctl_set_guest_debug(vcpu, &dbg);
 		break;
 	}
@@ -2674,53 +2675,59 @@ static long kvm_vcpu_ioctl(struct file *filp,
 
 		p = NULL;
 		if (argp) {
-			r = -EFAULT;
 			if (copy_from_user(&kvm_sigmask, argp,
-					   sizeof(kvm_sigmask)))
-				goto out;
-			r = -EINVAL;
-			if (kvm_sigmask.len != sizeof(sigset))
-				goto out;
-			r = -EFAULT;
+					   sizeof(kvm_sigmask))) {
+				r = -EFAULT;
+				goto put_vcpu;
+			}
+			if (kvm_sigmask.len != sizeof(sigset)) {
+				r = -EINVAL;
+				goto put_vcpu;
+			}
 			if (copy_from_user(&sigset, sigmask_arg->sigset,
-					   sizeof(sigset)))
-				goto out;
+					   sizeof(sigset))) {
+				r = -EFAULT;
+				goto put_vcpu;
+			}
 			p = &sigset;
 		}
 		r = kvm_vcpu_ioctl_set_sigmask(vcpu, p);
 		break;
 	}
 	case KVM_GET_FPU: {
+		struct kvm_fpu *fpu;
+
 		fpu = kzalloc(sizeof(struct kvm_fpu), GFP_KERNEL);
-		r = -ENOMEM;
-		if (!fpu)
-			goto out;
+		if (!fpu) {
+			r = -ENOMEM;
+			goto put_vcpu;
+		}
 		r = kvm_arch_vcpu_ioctl_get_fpu(vcpu, fpu);
 		if (r)
-			goto out;
-		r = -EFAULT;
+			goto free_fpu;
 		if (copy_to_user(argp, fpu, sizeof(struct kvm_fpu)))
-			goto out;
-		r = 0;
+			r = -EFAULT;
+free_fpu:
+		kfree(fpu);
 		break;
 	}
 	case KVM_SET_FPU: {
+		struct kvm_fpu *fpu;
+
 		fpu = memdup_user(argp, sizeof(*fpu));
 		if (IS_ERR(fpu)) {
 			r = PTR_ERR(fpu);
-			fpu = NULL;
-			goto out;
+			goto put_vcpu;
 		}
 		r = kvm_arch_vcpu_ioctl_set_fpu(vcpu, fpu);
+		kfree(fpu);
 		break;
 	}
 	default:
 		r = kvm_arch_vcpu_ioctl(filp, ioctl, arg);
 	}
-out:
+put_vcpu:
 	vcpu_put(vcpu);
-	kfree(fpu);
-	kfree(kvm_sregs);
 	return r;
 }
 
-- 
2.11.0

[PATCH 5/9] KVM: Improve size determinations in kvm_vcpu_ioctl()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 22 Jan 2017 17:11:16 +0100

Replace the specification of data structures by pointer dereferences
as the parameter for the operator "sizeof" to make the corresponding size
determination a bit safer according to the Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 virt/kvm/kvm_main.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 9d463b7a3912..6a74a3796b3f 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -2569,7 +2569,7 @@ static long kvm_vcpu_ioctl(struct file *filp,
 	case KVM_GET_REGS: {
 		struct kvm_regs *kvm_regs;
 
-		kvm_regs = kzalloc(sizeof(struct kvm_regs), GFP_KERNEL);
+		kvm_regs = kzalloc(sizeof(*kvm_regs), GFP_KERNEL);
 		if (!kvm_regs) {
 			r = -ENOMEM;
 			goto put_vcpu;
@@ -2577,7 +2577,7 @@ static long kvm_vcpu_ioctl(struct file *filp,
 		r = kvm_arch_vcpu_ioctl_get_regs(vcpu, kvm_regs);
 		if (r)
 			goto free_regs;
-		if (copy_to_user(argp, kvm_regs, sizeof(struct kvm_regs)))
+		if (copy_to_user(argp, kvm_regs, sizeof(*kvm_regs)))
 			r = -EFAULT;
 free_regs:
 		kfree(kvm_regs);
@@ -2598,7 +2598,7 @@ static long kvm_vcpu_ioctl(struct file *filp,
 	case KVM_GET_SREGS: {
 		struct kvm_sregs *kvm_sregs;
 
-		kvm_sregs = kzalloc(sizeof(struct kvm_sregs), GFP_KERNEL);
+		kvm_sregs = kzalloc(sizeof(*kvm_sregs), GFP_KERNEL);
 		if (!kvm_sregs) {
 			r = -ENOMEM;
 			goto put_vcpu;
@@ -2606,7 +2606,7 @@ static long kvm_vcpu_ioctl(struct file *filp,
 		r = kvm_arch_vcpu_ioctl_get_sregs(vcpu, kvm_sregs);
 		if (r)
 			goto free_sregs;
-		if (copy_to_user(argp, kvm_sregs, sizeof(struct kvm_sregs)))
+		if (copy_to_user(argp, kvm_sregs, sizeof(*kvm_sregs)))
 			r = -EFAULT;
 free_sregs:
 		kfree(kvm_sregs);
@@ -2697,7 +2697,7 @@ static long kvm_vcpu_ioctl(struct file *filp,
 	case KVM_GET_FPU: {
 		struct kvm_fpu *fpu;
 
-		fpu = kzalloc(sizeof(struct kvm_fpu), GFP_KERNEL);
+		fpu = kzalloc(sizeof(*fpu), GFP_KERNEL);
 		if (!fpu) {
 			r = -ENOMEM;
 			goto put_vcpu;
@@ -2705,7 +2705,7 @@ static long kvm_vcpu_ioctl(struct file *filp,
 		r = kvm_arch_vcpu_ioctl_get_fpu(vcpu, fpu);
 		if (r)
 			goto free_fpu;
-		if (copy_to_user(argp, fpu, sizeof(struct kvm_fpu)))
+		if (copy_to_user(argp, fpu, sizeof(*fpu)))
 			r = -EFAULT;
 free_fpu:
 		kfree(fpu);
-- 
2.11.0

[PATCH 6/9] KVM: Return an error code only as a constant in kvm_get_dirty_log_protect()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 22 Jan 2017 17:30:16 +0100

* Return an error code without storing it in an intermediate variable.

* Delete the local variable "r" and the jump label "out" which became
  unnecessary with this refactoring.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 virt/kvm/kvm_main.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 6a74a3796b3f..a651a137ef21 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -1156,24 +1156,22 @@ int kvm_get_dirty_log_protect(struct kvm *kvm,
 {
 	struct kvm_memslots *slots;
 	struct kvm_memory_slot *memslot;
-	int r, i, as_id, id;
+	int i, as_id, id;
 	unsigned long n;
 	unsigned long *dirty_bitmap;
 	unsigned long *dirty_bitmap_buffer;
 
-	r = -EINVAL;
 	as_id = log->slot >> 16;
 	id = (u16)log->slot;
 	if (as_id >= KVM_ADDRESS_SPACE_NUM || id >= KVM_USER_MEM_SLOTS)
-		goto out;
+		return -EINVAL;
 
 	slots = __kvm_memslots(kvm, as_id);
 	memslot = id_to_memslot(slots, id);
 
 	dirty_bitmap = memslot->dirty_bitmap;
-	r = -ENOENT;
 	if (!dirty_bitmap)
-		goto out;
+		return -ENOENT;
 
 	n = kvm_dirty_bitmap_bytes(memslot);
 
@@ -1202,14 +1200,9 @@ int kvm_get_dirty_log_protect(struct kvm *kvm,
 	}
 
 	spin_unlock(&kvm->mmu_lock);
-
-	r = -EFAULT;
 	if (copy_to_user(log->dirty_bitmap, dirty_bitmap_buffer, n))
-		goto out;
-
-	r = 0;
-out:
-	return r;
+		return -EFAULT;
+	return 0;
 }
 EXPORT_SYMBOL_GPL(kvm_get_dirty_log_protect);
 #endif
-- 
2.11.0

[PATCH 7/9] KVM: Return an error code only as a constant in kvm_get_dirty_log()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 22 Jan 2017 17:41:07 +0100

* Return an error code without storing it in an intermediate variable.

* Delete the local variable "r" and the jump label "out" which became
  unnecessary with this refactoring.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 virt/kvm/kvm_main.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index a651a137ef21..ca88f3620ee3 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -1094,37 +1094,31 @@ int kvm_get_dirty_log(struct kvm *kvm,
 {
 	struct kvm_memslots *slots;
 	struct kvm_memory_slot *memslot;
-	int r, i, as_id, id;
+	int i, as_id, id;
 	unsigned long n;
 	unsigned long any = 0;
 
-	r = -EINVAL;
 	as_id = log->slot >> 16;
 	id = (u16)log->slot;
 	if (as_id >= KVM_ADDRESS_SPACE_NUM || id >= KVM_USER_MEM_SLOTS)
-		goto out;
+		return -EINVAL;
 
 	slots = __kvm_memslots(kvm, as_id);
 	memslot = id_to_memslot(slots, id);
-	r = -ENOENT;
 	if (!memslot->dirty_bitmap)
-		goto out;
+		return -ENOENT;
 
 	n = kvm_dirty_bitmap_bytes(memslot);
 
 	for (i = 0; !any && i < n/sizeof(long); ++i)
 		any = memslot->dirty_bitmap[i];
 
-	r = -EFAULT;
 	if (copy_to_user(log->dirty_bitmap, memslot->dirty_bitmap, n))
-		goto out;
+		return -EFAULT;
 
 	if (any)
 		*is_dirty = 1;
-
-	r = 0;
-out:
-	return r;
+	return 0;
 }
 EXPORT_SYMBOL_GPL(kvm_get_dirty_log);
 
-- 
2.11.0

[PATCH 8/9] KVM: Adjust seven checks for null pointers

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 22 Jan 2017 18:54:22 +0100
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The script "checkpatch.pl" pointed information out like the following.

Comparison to NULL could be written …

Thus fix affected source code places.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 virt/kvm/kvm_main.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index ca88f3620ee3..4267a2c6f2aa 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -184,7 +184,7 @@ bool kvm_make_all_cpus_request(struct kvm *kvm, unsigned int req)
 		/* Set ->requests bit before we read ->mode. */
 		smp_mb__after_atomic();
 
-		if (cpus != NULL && cpu != -1 && cpu != me &&
+		if (cpus && cpu != -1 && cpu != me &&
 		      kvm_vcpu_exiting_guest_mode(vcpu) != OUTSIDE_GUEST_MODE)
 			cpumask_set_cpu(cpu, cpus);
 	}
@@ -1532,10 +1532,9 @@ static kvm_pfn_t hva_to_pfn(unsigned long addr, bool atomic, bool *async,
 
 retry:
 	vma = find_vma_intersection(current->mm, addr, addr + 1);
-
-	if (vma == NULL)
+	if (!vma) {
 		pfn = KVM_PFN_ERR_FAULT;
-	else if (vma->vm_flags & (VM_IO | VM_PFNMAP)) {
+	} else if (vma->vm_flags & (VM_IO | VM_PFNMAP)) {
 		r = hva_to_pfn_remapped(vma, addr, async, write_fault, &pfn);
 		if (r == -EAGAIN)
 			goto retry;
@@ -2835,7 +2834,7 @@ int kvm_register_device_ops(struct kvm_device_ops *ops, u32 type)
 	if (type >= ARRAY_SIZE(kvm_device_ops_table))
 		return -ENOSPC;
 
-	if (kvm_device_ops_table[type] != NULL)
+	if (kvm_device_ops_table[type])
 		return -EEXIST;
 
 	kvm_device_ops_table[type] = ops;
@@ -2844,7 +2843,7 @@ int kvm_register_device_ops(struct kvm_device_ops *ops, u32 type)
 
 void kvm_unregister_device_ops(u32 type)
 {
-	if (kvm_device_ops_table[type] != NULL)
+	if (kvm_device_ops_table[type])
 		kvm_device_ops_table[type] = NULL;
 }
 
@@ -2860,7 +2859,7 @@ static int kvm_ioctl_create_device(struct kvm *kvm,
 		return -ENODEV;
 
 	ops = kvm_device_ops_table[cd->type];
-	if (ops == NULL)
+	if (!ops)
 		return -ENODEV;
 
 	if (test)
@@ -3392,7 +3391,7 @@ static int kvm_io_bus_get_first_dev(struct kvm_io_bus *bus,
 
 	range = bsearch(&key, bus->range, bus->dev_count,
 			sizeof(struct kvm_io_range), kvm_io_bus_sort_cmp);
-	if (range == NULL)
+	if (!range)
 		return -ENOENT;
 
 	off = range - bus->range;
@@ -3803,7 +3802,7 @@ static int kvm_init_debug(void)
 	struct kvm_stats_debugfs_item *p;
 
 	kvm_debugfs_dir = debugfs_create_dir("kvm", NULL);
-	if (kvm_debugfs_dir == NULL)
+	if (!kvm_debugfs_dir)
 		goto out;
 
 	kvm_debugfs_num_entries = 0;
-- 
2.11.0

Re: [PATCH 8/9] KVM: Adjust seven checks for null pointers

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1934 bytes --]

Hi Markus,

[auto build test WARNING on kvm/linux-next]
[also build test WARNING on v4.10-rc4 next-20170120]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/SF-Markus-Elfring/KVM-Fine-tuning-for-several-function-implementations/20170123-022323
base:   https://git.kernel.org/pub/scm/virt/kvm/kvm.git linux-next
config: x86_64-allyesdebian (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All warnings (new ones prefixed by >>):

   arch/x86/kvm/../../../virt/kvm/kvm_main.c: In function 'kvm_make_all_cpus_request':
>> arch/x86/kvm/../../../virt/kvm/kvm_main.c:187:7: warning: the address of 'cpus' will always evaluate as 'true' [-Waddress]
      if (cpus && cpu != -1 && cpu != me &&
          ^~~~

vim +187 arch/x86/kvm/../../../virt/kvm/kvm_main.c

   171	{
   172		int i, cpu, me;
   173		cpumask_var_t cpus;
   174		bool called = true;
   175		struct kvm_vcpu *vcpu;
   176	
   177		zalloc_cpumask_var(&cpus, GFP_ATOMIC);
   178	
   179		me = get_cpu();
   180		kvm_for_each_vcpu(i, vcpu, kvm) {
   181			kvm_make_request(req, vcpu);
   182			cpu = vcpu->cpu;
   183	
   184			/* Set ->requests bit before we read ->mode. */
   185			smp_mb__after_atomic();
   186	
 > 187			if (cpus && cpu != -1 && cpu != me &&
   188			      kvm_vcpu_exiting_guest_mode(vcpu) != OUTSIDE_GUEST_MODE)
   189				cpumask_set_cpu(cpu, cpus);
   190		}
   191		if (unlikely(cpus == NULL))
   192			smp_call_function_many(cpu_online_mask, ack_flush, NULL, 1);
   193		else if (!cpumask_empty(cpus))
   194			smp_call_function_many(cpus, ack_flush, NULL, 1);
   195		else

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 38049 bytes --]

Re: [PATCH 8/9] KVM: Adjust seven checks for null pointers

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2156 bytes --]

Hi Markus,

[auto build test ERROR on kvm/linux-next]
[also build test ERROR on v4.10-rc4 next-20170120]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/SF-Markus-Elfring/KVM-Fine-tuning-for-several-function-implementations/20170123-022323
base:   https://git.kernel.org/pub/scm/virt/kvm/kvm.git linux-next
config: mips-malta_kvm_defconfig (attached as .config)
compiler: mipsel-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=mips 

All errors (new ones prefixed by >>):

   arch/mips/kvm/../../../virt/kvm/kvm_main.c: In function 'kvm_make_all_cpus_request':
>> arch/mips/kvm/../../../virt/kvm/kvm_main.c:187:7: error: the address of 'cpus' will always evaluate as 'true' [-Werror=address]
      if (cpus && cpu != -1 && cpu != me &&
          ^~~~
   cc1: all warnings being treated as errors

vim +187 arch/mips/kvm/../../../virt/kvm/kvm_main.c

   171	{
   172		int i, cpu, me;
   173		cpumask_var_t cpus;
   174		bool called = true;
   175		struct kvm_vcpu *vcpu;
   176	
   177		zalloc_cpumask_var(&cpus, GFP_ATOMIC);
   178	
   179		me = get_cpu();
   180		kvm_for_each_vcpu(i, vcpu, kvm) {
   181			kvm_make_request(req, vcpu);
   182			cpu = vcpu->cpu;
   183	
   184			/* Set ->requests bit before we read ->mode. */
   185			smp_mb__after_atomic();
   186	
 > 187			if (cpus && cpu != -1 && cpu != me &&
   188			      kvm_vcpu_exiting_guest_mode(vcpu) != OUTSIDE_GUEST_MODE)
   189				cpumask_set_cpu(cpu, cpus);
   190		}
   191		if (unlikely(cpus == NULL))
   192			smp_call_function_many(cpu_online_mask, ack_flush, NULL, 1);
   193		else if (!cpumask_empty(cpus))
   194			smp_call_function_many(cpus, ack_flush, NULL, 1);
   195		else

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 18249 bytes --]

Re: [PATCH 8/9] KVM: Adjust seven checks for null pointers

[Dan Carpenter]

Hm...  GCC complains about "if (array)" but not about
"if (array != NULL) ".  I view this as a bug in GCC...

But you should still be seeing these compile warnings.  This is
something like the 6th warning that kbuild has had to send you.

regards,
dan carpenter

[PATCH 9/9] KVM: Improve another size determination in kvm_create_vm()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 22 Jan 2017 18:56:26 +0100

Replace the specification of a data structure by a pointer dereference
as the parameter for the operator "sizeof" to make the corresponding size
determination a bit safer according to the Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 virt/kvm/kvm_main.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 4267a2c6f2aa..0feefe45aa21 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -651,8 +651,7 @@ static struct kvm *kvm_create_vm(unsigned long type)
 	if (init_srcu_struct(&kvm->irq_srcu))
 		goto out_err_no_irq_srcu;
 	for (i = 0; i < KVM_NR_BUSES; i++) {
-		kvm->buses[i] = kzalloc(sizeof(struct kvm_io_bus),
-					GFP_KERNEL);
+		kvm->buses[i] = kzalloc(sizeof(*kvm->buses[i]), GFP_KERNEL);
 		if (!kvm->buses[i])
 			goto out_err;
 	}
-- 
2.11.0

Re: [PATCH 0/9] KVM: Fine-tuning for several function implementations

[Paolo Bonzini]

On 22/01/2017 19:09, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sun, 22 Jan 2017 19:06:54 +0100
> 
> Several update suggestions were taken into account
> from static source code analysis.

Patches 1, 6 and 7 are acceptable.  The others are useless churn or they
make the result uselessly different from the rest of KVM code.

Paolo

> Markus Elfring (9):
>   Return directly after a failed copy_from_user() in kvm_vm_compat_ioctl()
>   Move error code settings in kvm_vm_ioctl()
>   Move error code settings in kvm_vcpu_compat_ioctl()
>   Move error code settings in kvm_vcpu_ioctl()
>   Improve size determinations in kvm_vcpu_ioctl()
>   Return an error code only as a constant in kvm_get_dirty_log_protect()
>   Return an error code only as a constant in kvm_get_dirty_log()
>   Adjust seven checks for null pointers
>   Improve another size determination in kvm_create_vm()
> 
>  virt/kvm/kvm_main.c | 278 +++++++++++++++++++++++-----------------------------
>  1 file changed, 120 insertions(+), 158 deletions(-)
> 

Re: KVM: Fine-tuning for several function implementations

[SF Markus Elfring]

> Patches 1, 6 and 7 are acceptable.

Thanks for a bit of positive feedback.


> The others are useless churn or they make the result uselessly different
> from the rest of KVM code.

I got an other view about potential benefits around the suggested movements
for error code settings.
Would others like to help in approaches for checking corresponding run time changes
a bit more?

Regards,
Markus

Re: KVM: Fine-tuning for several function implementations

[Paolo Bonzini]

On 23/01/2017 10:48, SF Markus Elfring wrote:
>> The others are useless churn or they make the result uselessly different
>> from the rest of KVM code.
>
> I got an other view about potential benefits around the suggested movements
> for error code settings.

I'll consider them when your emails stop looking like the output a
Markov chain.

Paolo

> Would others like to help in approaches for checking corresponding run time changes
> a bit more?

Re: KVM: Fine-tuning for several function implementations

[Bernd Petrovitsch]

On Mon, 2017-01-23 at 10:48 +0100, SF Markus Elfring wrote:
[...]
> > The others are useless churn or they make the result uselessly
> > different from the rest of KVM code.
> 
> I got an other view about potential benefits around the suggested
> movements for error code settings.

Which is not explained here.

> Would others like to help in approaches for checking corresponding
> run time changes a bit more?

You propose the patch and others should do the work to get it accepted?

Kind regards,
	Bernd
-- 
Bernd Petrovitsch                  Email : bernd@petrovitsch.priv.at
                     LUGA : http://www.luga.at

Re: KVM: Fine-tuning for several function implementations

[SF Markus Elfring]

>> Would others like to help in approaches for checking corresponding
>> run time changes a bit more?
> 
> You propose the patch

I suggested additional changes which can get an usual mixture of agreements
and rejections.


> and others should do the work to get it accepted?

Not directly. - I imagine that there are further collaboration possibilities,
aren't there?

Which benchmarks (and test metrics) would you trust for the desired software evolution?

Regards,
Markus