Re: [PATCH -next] virtio: balloon: fix missing unlock on error in fill_balloon()

2012-11-13 Thread Rafael Aquini 
On Mon, Nov 12, 2012 at 03:34:53PM -0800, Andrew Morton wrote:
 On Mon, 12 Nov 2012 21:50:40 +0800
 Wei Yongjun weiyj...@gmail.com wrote:
 
  From: Wei Yongjun yongjun_...@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_...@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);
  }


Your fix is far better, as it simplifies the code without loosing that test --
which only evaluates as true, if we fail to allocate a page right after getting
into the loop -- /me thinks driver folks place it there to not bother
tell_host() and the virt_queues if no pageframe is present at that pfns array.


___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH -next] virtio: balloon: fix missing unlock on error in fill_balloon()

2012-11-12 Thread Rafael Aquini 
On Mon, Nov 12, 2012 at 09:50:40PM +0800, Wei Yongjun wrote:
 From: Wei Yongjun yongjun_...@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_...@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
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH -next] virtio: balloon: fix missing unlock on error in fill_balloon()

2012-11-12 Thread Andrew Morton 
On Mon, 12 Nov 2012 21:50:40 +0800
Wei Yongjun weiyj...@gmail.com wrote:

 From: Wei Yongjun yongjun_...@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_...@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);
 }
 
_

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization