xen: Fix a grant table resource leak

PATCH In review Dominic Curran 3 participants 4 messages May 11, 2012 Xen hypervisor interface

Share & tools

Dominic Curran

author

2012-05-11 14:20 UTC #0 lore raw

PATCH 1/1 xen: Fix a grant table resource leak

From: Dominic Curran <[email protected]>
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 <[email protected]>

---
 drivers/xen/grant-table.c |   72 
+++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 68 insertions(+), 4 deletions(-)

drivers/xen/grant-table.c 2012-05-08 16:53:52.000000000 +0100 renamed from drivers/xen/grant-table.c 2012-05-08 13:52:54.000000000 +0100 +68 -4

Index: linux-2.6/drivers/xen/grant-table.c
===================================================================
@@ -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;

Jan Beulich

2012-05-11 14:30 UTC #1 lore raw

Quoted 8 lines

Quoted 1 line
Quoted 1 line · Dominic Curran
From: Dominic Curran <[email protected]>
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.

Did you see

http://lists.xen.org/archives/html/xen-devel/2012-04/msg00406.html

Jan

Quoted 141 lines

Signed-off-by Dominic Curran <[email protected]>

---
  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

drivers/xen/grant-table.c 2012-05-08 16:53:52.000000000 +0100 renamed from drivers/xen/grant-table.c 2012-05-08 13:52:54.000000000 +0100 +68 -4

@@ -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;

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Konrad Rzeszutek Wilk

2012-05-11 14:30 UTC #2 lore raw

Quoted 5 lines · Jan Beulich

Quoted 5 lines
Quoted 1 line
Quoted 1 line · Dominic Curran
From: Dominic Curran <[email protected]>
Date: Fri, 11 May 2012 12:24:42 +0100
Subject: [PATCH] xen: Fix a grant table resource leak

Quoted 7 lines

Quoted 3 lines
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.
Did you see
http://lists.xen.org/archives/html/xen-devel/2012-04/msg00406.html

Which is on the 3.5 queue list:
http://git.kernel.org/?p=linux/kernel/git/konrad/xen.git;a=commit;h=569ca5b3f94cd0b3295ec5943aa457cf4a4f6a3a

Quoted 145 lines

Jan

Quoted 141 lines

Signed-off-by Dominic Curran <[email protected]>

---
  drivers/xen/grant-table.c |   72 
+++++++++++++++++++++++++++++++++++++++++++---
  1 file changed, 68 insertions(+), 4 deletions(-)

drivers/xen/grant-table.c 2012-05-08 16:53:52.000000000 +0100 renamed from drivers/xen/grant-table.c 2012-05-08 13:52:54.000000000 +0100 +68 -4

@@ -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;

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Dominic Curran

author

2012-05-11 14:55 UTC #3 lore raw

In reply to Jan Beulich

Quoted 5 lines · Jan Beulich

Quoted 5 lines
Quoted 1 line
Quoted 1 line · Dominic Curran
From: Dominic Curran<[email protected]>
Date: Fri, 11 May 2012 12:24:42 +0100
Subject: [PATCH] xen: Fix a grant table resource leak

Quoted 8 lines

Quoted 3 lines
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.
Did you see
http://lists.xen.org/archives/html/xen-devel/2012-04/msg00406.html
Jan

Oh!