Re: [PATCH] virtio: avoid possible OOM lockup at virtballoon_oom_notify()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 10/10/2017 06:47 PM, Tetsuo Handa wrote:
In leak_balloon(), mutex_lock(&vb->balloon_lock) is called in order to
serialize against fill_balloon(). But in fill_balloon(),
alloc_page(GFP_HIGHUSER[_MOVABLE] | __GFP_NOMEMALLOC | __GFP_NORETRY) is
called with vb->balloon_lock mutex held. Since GFP_HIGHUSER[_MOVABLE]
implies __GFP_DIRECT_RECLAIM | __GFP_IO | __GFP_FS, despite __GFP_NORETRY
is specified, this allocation attempt might indirectly depend on somebody
else's __GFP_DIRECT_RECLAIM memory allocation. And such indirect
__GFP_DIRECT_RECLAIM memory allocation might call leak_balloon() via
virtballoon_oom_notify() via blocking_notifier_call_chain() callback via
out_of_memory() when it reached __alloc_pages_may_oom() and held oom_lock
mutex. Since vb->balloon_lock mutex is already held by fill_balloon(), it
will cause OOM lockup. Thus, do not wait for vb->balloon_lock mutex if
leak_balloon() is called from out_of_memory().

   Thread1                                       Thread2
     fill_balloon()
       takes a balloon_lock
       balloon_page_enqueue()
         alloc_page(GFP_HIGHUSER_MOVABLE)
           direct reclaim (__GFP_FS context)       takes a fs lock
             waits for that fs lock                  alloc_page(GFP_NOFS)
                                                       __alloc_pages_may_oom()
                                                         takes the oom_lock
                                                         out_of_memory()
                                                           blocking_notifier_call_chain()
                                                             leak_balloon()
                                                               tries to take that balloon_lock and deadlocks

Signed-off-by: Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx>


I think we could also use 'bool wait' to let the OOM use the traditional leak_balloon code path, which doesn't need memory allocation from xb_preload(). That is, inside leak_balloon(), we can have

use_sg = virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_SG) & wait;


Reviewed-by: Wei Wang <wei.w.wang@xxxxxxxxx>


---
  drivers/virtio/virtio_balloon.c | 16 +++++++++++-----
  1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index f0b3a0b..03e6078 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -192,7 +192,7 @@ static void release_pages_balloon(struct virtio_balloon *vb,
  	}
  }
-static unsigned leak_balloon(struct virtio_balloon *vb, size_t num)
+static unsigned leak_balloon(struct virtio_balloon *vb, size_t num, bool wait)
  {
  	unsigned num_freed_pages;
  	struct page *page;
@@ -202,7 +202,13 @@ static unsigned leak_balloon(struct virtio_balloon *vb, size_t num)
  	/* We can only do one array worth at a time. */
  	num = min(num, ARRAY_SIZE(vb->pfns));
- mutex_lock(&vb->balloon_lock);
+	if (wait)
+		mutex_lock(&vb->balloon_lock);
+	else if (!mutex_trylock(&vb->balloon_lock)) {
+		pr_info("virtio_balloon: Unable to release %lu pages due to lock contention.\n",
+			(unsigned long) min(num, (size_t)vb->num_pages));
+		return 0;
+	}
  	/* We can't release more pages than taken */
  	num = min(num, (size_t)vb->num_pages);
  	for (vb->num_pfns = 0; vb->num_pfns < num;
@@ -367,7 +373,7 @@ static int virtballoon_oom_notify(struct notifier_block *self,
  		return NOTIFY_OK;
freed = parm;
-	num_freed_pages = leak_balloon(vb, oom_pages);
+	num_freed_pages = leak_balloon(vb, oom_pages, false);
  	update_balloon_size(vb);
  	*freed += num_freed_pages;
@@ -395,7 +401,7 @@ static void update_balloon_size_func(struct work_struct *work)
  	if (diff > 0)
  		diff -= fill_balloon(vb, diff);
  	else if (diff < 0)
-		diff += leak_balloon(vb, -diff);
+		diff += leak_balloon(vb, -diff, true);
  	update_balloon_size(vb);
if (diff)
@@ -597,7 +603,7 @@ static void remove_common(struct virtio_balloon *vb)
  {
  	/* There might be pages left in the balloon: free them. */
  	while (vb->num_pages)
-		leak_balloon(vb, vb->num_pages);
+		leak_balloon(vb, vb->num_pages, true);
  	update_balloon_size(vb);
/* Now we reset the device so we can clean up the queues. */

_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization



[Index of Archives]     [KVM Development]     [Libvirt Development]     [Libvirt Users]     [CentOS Virtualization]     [Netdev]     [Ethernet Bridging]     [Linux Wireless]     [Kernel Newbies]     [Security]     [Linux for Hams]     [Netfilter]     [Bugtraq]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux RAID]     [Linux Admin]     [Samba]

  Powered by Linux