Wei Yongjun
2012-Nov-12 13:50 UTC
[PATCH -next] virtio: balloon: fix missing unlock on error in fill_balloon()
From: Wei Yongjun <yongjun_wei at trendmicro.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 at trendmicro.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);
Rafael Aquini
2012-Nov-12 14:16 UTC
[PATCH -next] virtio: balloon: fix missing unlock on error in fill_balloon()
On Mon, Nov 12, 2012 at 09:50:40PM +0800, Wei Yongjun wrote:> From: Wei Yongjun <yongjun_wei at trendmicro.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 at trendmicro.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); >I missed that one when rewriting v10 to v11. :( Good catch, Wei! thanks! Andrew, we need this pick for the virtio_balloon v12 patch, as well. -- Rafael
Andrew Morton
2012-Nov-12 23:34 UTC
[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 at gmail.com> wrote:> From: Wei Yongjun <yongjun_wei at trendmicro.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 at trendmicro.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); } _
Apparently Analagous Threads
- [PATCH -next] virtio: balloon: fix missing unlock on error in fill_balloon()
- [PATCH] xen-pciback: fix error return code in pcistub_irq_handler_switch()
- [PATCH] xen-pciback: fix error return code in pcistub_irq_handler_switch()
- [Bridge] [PATCH -next] bridge: using for_each_set_bit_from to simplify the code
- [PATCH -next] virtio_console: make local symbols static