linux-kernel - Re: [PATCH -next] virtio: balloon: fix missing unlock on error in fill

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20121112153453.fa3edc3d.akpm@linux-foundation.org>
Date:	Mon, 12 Nov 2012 15:34:53 -0800
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	Wei Yongjun <weiyj.lk@...il.com>
Cc:	rusty@...tcorp.com.au, mst@...hat.com, aquini@...hat.com,
	yongjun_wei@...ndmicro.com.cn,
	virtualization@...ts.linux-foundation.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH -next] virtio: balloon: fix missing unlock on error in
 fill_balloon()

On Mon, 12 Nov 2012 21:50:40 +0800
Wei Yongjun <weiyj.lk@...il.com> wrote:

> From: Wei Yongjun <yongjun_wei@...ndmicro.com.cn>
> 
> Add the missing unlock before return from function fill_balloon()
> in the error handling case.
> 
> Introduced by 9864a8(virtio_balloon: introduce migration primitives
> to balloon pages)
> 
> dpatch engine is used to auto generate this patch.
> (https://github.com/weiyj/dpatch)
> 
> Signed-off-by: Wei Yongjun <yongjun_wei@...ndmicro.com.cn>
> ---
>  drivers/virtio/virtio_balloon.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index f70151b..72e8dcb 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -152,8 +152,10 @@ static void fill_balloon(struct virtio_balloon *vb, size_t num)
>  	}
>  
>  	/* Didn't get any?  Oh well. */
> -	if (vb->num_pfns == 0)
> +	if (vb->num_pfns == 0) {
> +		mutex_unlock(&vb->balloon_lock);
>  		return;
> +	}
>  
>  	tell_host(vb, vb->inflate_vq);
>  	mutex_unlock(&vb->balloon_lock);

Well.  Why did this bug occur in the first place?  Because
fill_balloon() has multiple return points and one of those was
overlooked when adding new locking.

This (and the addition of memory leaks) is quite common.  It is part of
the reason why so much kernel code uses goto's to avoid multiple return
points.  A maintainability thing.

And we can fix this shortcoming in fill_balloon() without even needing a
goto:

--- a/drivers/virtio/virtio_balloon.c~virtio_balloon-introduce-migration-primitives-to-balloon-pages-fix-fix
+++ a/drivers/virtio/virtio_balloon.c
@@ -151,13 +151,9 @@ static void fill_balloon(struct virtio_b
 		totalram_pages--;
 	}
 
-	/* Didn't get any?  Oh well. */
-	if (vb->num_pfns == 0) {
-		mutex_unlock(&vb->balloon_lock);
-		return;
-	}
-
-	tell_host(vb, vb->inflate_vq);
+	/* Did we get any? */
+	if (vb->num_pfns != 0)
+		tell_host(vb, vb->inflate_vq);
 	mutex_unlock(&vb->balloon_lock);
 }
 
_

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/