re: kvm: irqchip: Break up high order allocations of kvm_irq_routing_table

re: kvm: irqchip: Break up high order allocations of kvm_irq_routing_table

[Dan Carpenter]

Hello Joerg Roedel,

The patch e73f61e41f3b: "kvm: irqchip: Break up high order
allocations of kvm_irq_routing_table" from May 8, 2015, leads to the
following static checker warning:

	arch/x86/kvm/../../../virt/kvm/irqchip.c:238 kvm_set_irq_routing()
	warn: possible memory leak of 'e'

arch/x86/kvm/../../../virt/kvm/irqchip.c
   207          for (i = 0; i < nr; ++i) {
   208                  struct kvm_kernel_irq_routing_entry *e;
   209  
   210                  r = -ENOMEM;
   211                  e = kzalloc(sizeof(*e), GFP_KERNEL);
   212                  if (!e)
   213                          goto out;
   214  
   215                  r = -EINVAL;
   216                  if (ue->flags)
   217                          goto out;
				^^^^^^^^
Leaked here.  Move in front of the allocation?

   218                  r = setup_routing_entry(new, e, ue);
   219                  if (r)
   220                          goto out;
   221                  ++ue;
   222          }
   223  
   224          mutex_lock(&kvm->irq_lock);
   225          old = kvm->irq_routing;
   226          rcu_assign_pointer(kvm->irq_routing, new);
   227          kvm_irq_routing_update(kvm);
   228          mutex_unlock(&kvm->irq_lock);
   229  
   230          synchronize_srcu_expedited(&kvm->irq_srcu);
   231  
   232          new = old;
   233          r = 0;
   234  
   235  out:
   236          free_irq_routing_table(new);
   237  
   238          return r;
   239  }


regards,
dan carpenter

[PATCH] kvm: irqchip: Fix possible memory leak in kvm_set_irq_routing()

[Joerg Roedel]

Hi Dan,

On Fri, Jun 26, 2015 at 12:00:22PM +0300, Dan Carpenter wrote:
> The patch e73f61e41f3b: "kvm: irqchip: Break up high order
> allocations of kvm_irq_routing_table" from May 8, 2015, leads to the
> following static checker warning:
>    215                  r = -EINVAL;
>    216                  if (ue->flags)
>    217                          goto out;
> 				^^^^^^^^
> Leaked here.  Move in front of the allocation?

Right, this is a potential leak, thanks for the report. The patch below
should fix it:

>From 14abe455d04f7208a16237a2f1321fd5e5c5d115 Mon Sep 17 00:00:00 2001
From: Joerg Roedel <jroedel@suse.de>
Date: Fri, 26 Jun 2015 18:02:47 +0200
Subject: [PATCH] kvm: irqchip: Fix possible memory leak in
 kvm_set_irq_routing()

If ue->flags field is checked after the allocation of the
kvm_kernel_irq_routing_entry, it will be leaked if the check
succeeds. Do the check before the allocation instead to
avoid this leak.

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Fixes: e73f61e41f3b: "kvm: irqchip: Break up high order allocations of kvm_irq_routing_table"
Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 virt/kvm/irqchip.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/virt/kvm/irqchip.c b/virt/kvm/irqchip.c
index 21c1424..239f4ec 100644
--- a/virt/kvm/irqchip.c
+++ b/virt/kvm/irqchip.c
@@ -207,14 +207,15 @@ int kvm_set_irq_routing(struct kvm *kvm,
 	for (i = 0; i < nr; ++i) {
 		struct kvm_kernel_irq_routing_entry *e;
 
+		r = -EINVAL;
+		if (ue->flags)
+			goto out;
+
 		r = -ENOMEM;
 		e = kzalloc(sizeof(*e), GFP_KERNEL);
 		if (!e)
 			goto out;
 
-		r = -EINVAL;
-		if (ue->flags)
-			goto out;
 		r = setup_routing_entry(new, e, ue);
 		if (r)
 			goto out;
-- 
1.8.4.5