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

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Fri, May 11, 2012 at 03:30:05PM +0100, Jan Beulich wrote:
> >>> On 11.05.12 at 16:20, Dominic Curran <dominic.curran@citrix.com> wrote:
> > 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.
> 
> 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

> 
> Jan
> 
> > 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;
> > 
> > 
> > 
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org 
> > http://lists.xen.org/xen-devel 
>