[PATCH] xen: Fix a grant table resource leak

* [PATCH] xen: Fix a grant table resource leak
@ 2012-05-11 14:20 Dominic Curran
  2012-05-11 14:30 ` Jan Beulich
  0 siblings, 1 reply; 4+ messages in thread
From: Dominic Curran @ 2012-05-11 14:20 UTC (permalink / raw)
  To: xen-devel, konrad.wilk

From: Dominic Curran <dominic.curran@citrix.com>
Date: Fri, 11 May 2012 12:24:42 +0100
Subject: [PATCH] xen: Fix a grant table resource leak

If the resource (my testing shows an object in TX/RX ring) still has a
reference held on it, then add to a list and kick-off a timer to try
and free later.

Signed-off-by: Dominic Curran <dominic.curran@citrix.com>
---
  drivers/xen/grant-table.c |   72 
+++++++++++++++++++++++++++++++++++++++++++---
  1 file changed, 68 insertions(+), 4 deletions(-)

Index: linux-2.6/drivers/xen/grant-table.c
===================================================================

--- linux-2.6.orig/drivers/xen/grant-table.c    2012-05-08 
13:52:54.000000000 +0100
+++ linux-2.6/drivers/xen/grant-table.c    2012-05-08 16:53:52.000000000 
+0100
@@ -33,6 +33,7 @@

  #include <linux/module.h>
  #include <linux/sched.h>
+#include <linux/timer.h>
  #include <linux/mm.h>
  #include <linux/slab.h>
  #include <linux/vmalloc.h>
@@ -57,6 +58,7 @@
  (grant_table_version == 1 ?                      \
  (PAGE_SIZE / sizeof(struct grant_entry_v1)) :   \
  (PAGE_SIZE / sizeof(union grant_entry_v2)))
+#define GNTTAB_CLEANUP_TIMER 10

  static grant_ref_t **gnttab_list;
  static unsigned int nr_grant_frames;
@@ -66,6 +68,9 @@ static grant_ref_t gnttab_free_head;
  static DEFINE_SPINLOCK(gnttab_list_lock);
  unsigned long xen_hvm_resume_frames;
  EXPORT_SYMBOL_GPL(xen_hvm_resume_frames);
+static LIST_HEAD(gnttab_used_ref_list);
+static DEFINE_SPINLOCK(gnttab_used_ref_lock);
+static struct timer_list gnttab_timer;

  static union {
      struct grant_entry_v1 *v1;
@@ -145,6 +150,16 @@ struct gnttab_ops {
                     domid_t trans_domid, grant_ref_t trans_gref);
  };

+/* This structure is used to hold references to pages until they become
+   ready to free.
+*/
+struct gnttab_used_ref {
+    struct list_head    list;
+    grant_ref_t            ref;
+    unsigned long        page;
+    int                readonly;
+};
+
  static struct gnttab_ops *gnttab_interface;

  /*This reflects status of grant entries, so act as a global value*/
@@ -464,18 +479,62 @@ int gnttab_end_foreign_access_ref(grant_
  }
  EXPORT_SYMBOL_GPL(gnttab_end_foreign_access_ref);

+static void gnttab_attempt_clean_used_refs(unsigned long arg)
+{
+    struct gnttab_used_ref  *u;
+    struct list_head        *pos, *q;
+    unsigned long            flags;
+
+    spin_lock_irqsave(&gnttab_used_ref_lock, flags);
+
+    list_for_each_safe(pos, q, &gnttab_used_ref_list) {
+        u = list_entry(pos, struct gnttab_used_ref, list);
+
+        if (gnttab_end_foreign_access_ref(u->ref, u->readonly)) {
+            list_del(pos);
+
+            put_free_entry(u->ref);
+            if (u->page != 0)
+                free_page(u->page);
+
+            kfree(u);
+        }
+    }
+    spin_unlock_irqrestore(&gnttab_used_ref_lock, flags);
+
+    if (!list_empty(&gnttab_used_ref_list))
+        mod_timer(&gnttab_timer, jiffies + HZ);
+}
+
  void gnttab_end_foreign_access(grant_ref_t ref, int readonly,
                     unsigned long page)
  {
+    struct gnttab_used_ref *u;
+    unsigned long flags;
+
      if (gnttab_end_foreign_access_ref(ref, readonly)) {
          put_free_entry(ref);
          if (page != 0)
              free_page(page);
      } else {
-        /* XXX This needs to be fixed so that the ref and page are
-           placed on a list to be freed up later. */
-        printk(KERN_WARNING
-               "WARNING: leaking g.e. and page still in use!\n");
+        /* References that aren't cleaned up immediately sit on
+         * a list until they are abandoned & can be cleaned safely.
+         */
+        u = kmalloc(sizeof(struct gnttab_used_ref), GFP_KERNEL);
+        if (!u)
+            return;
+
+        u->page = page;
+        u->ref = ref;
+        u->readonly = readonly;
+
+        spin_lock_irqsave(&gnttab_used_ref_lock, flags);
+        list_add(&(u->list), &gnttab_used_ref_list);
+        spin_unlock_irqrestore(&gnttab_used_ref_lock, flags);
+
+        /* Kick off the cleanup timer. */
+        if (!timer_pending(&gnttab_timer))
+            add_timer(&gnttab_timer);
      }
  }
  EXPORT_SYMBOL_GPL(gnttab_end_foreign_access);
@@ -1068,6 +1127,11 @@ int gnttab_init(void)
      gnttab_free_count = nr_init_grefs - NR_RESERVED_ENTRIES;
      gnttab_free_head  = NR_RESERVED_ENTRIES;

+    init_timer(&gnttab_timer);
+    gnttab_timer.data = 0;
+    gnttab_timer.function = gnttab_attempt_clean_used_refs;
+    gnttab_timer.expires = jiffies + 
msecs_to_jiffies(GNTTAB_CLEANUP_TIMER);
+
      printk("Grant table initialized\n");
      return 0;

^ permalink raw reply	[flat|nested] 4+ messages in thread