Re: [Xen-devel] [PATCH] xen: Fix a grant table resource leak

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 11.05.12 at 16:20, Dominic Curran <dominic.curran@xxxxxxxxxx> wrote:
> From: Dominic Curran <dominic.curran@xxxxxxxxxx>
> 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

> Signed-off-by: Dominic Curran <dominic.curran@xxxxxxxxxx>
> ---
>   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;
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxx 
> http://lists.xen.org/xen-devel 



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel