Re: [PATCH] kmmio/mmiotrace: fix double free of kmmio_fault_pages

[Pekka Paalanen <pq@iki.fi>]

On Sat, 5 Jun 2010 18:49:42 +0200
Marcin Slusarz <marcin.slusarz@gmail.com> wrote:

> After every iounmap mmiotrace has to free kmmio_fault_pages, but
> it can't do it directly, so it defers freeing by RCU.
> 
> It usually works, but when mmiotraced code calls ioremap-iounmap
> multiple times without sleeping between (so RCU won't kick in and
> start freeing) it can be given the same virtual address, so at
> every iounmap mmiotrace will schedule the same pages for release.
> Obviously it will explode on second free.
> 
> Fix it by marking kmmio_fault_pages which are scheduled for
> release and not adding them second time.
> 
> Signed-off-by: Marcin Slusarz <marcin.slusarz@gmail.com>
> Cc: Pekka Paalanen <pq@iki.fi>
> Cc: Stuart Bennett <stuart@freedesktop.org>

Excellent work! Unfortunately I cannot review this patch
right now, I am sick. The description sounds good, though,
and I have no objections.


Thank you very much!

> ---
>  arch/x86/mm/kmmio.c |   16 +++++++++++++---
>  1 files changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/mm/kmmio.c b/arch/x86/mm/kmmio.c
> index 5d0e67f..e5d5e2c 100644
> --- a/arch/x86/mm/kmmio.c
> +++ b/arch/x86/mm/kmmio.c
> @@ -45,6 +45,8 @@ struct kmmio_fault_page {
>  	 * Protected by kmmio_lock, when linked into
> kmmio_page_table. */
>  	int count;
> +
> +	bool scheduled_for_release;
>  };
>  
>  struct kmmio_delayed_release {
> @@ -398,8 +400,11 @@ static void
> release_kmmio_fault_page(unsigned long page, BUG_ON(f->count < 0);
>  	if (!f->count) {
>  		disarm_kmmio_fault_page(f);
> -		f->release_next = *release_list;
> -		*release_list = f;
> +		if (!f->scheduled_for_release) {
> +			f->release_next = *release_list;
> +			*release_list = f;
> +			f->scheduled_for_release = true;
> +		}
>  	}
>  }
>  
> @@ -471,8 +476,10 @@ static void remove_kmmio_fault_pages(struct
> rcu_head *head) prevp = &f->release_next;
>  		} else {
>  			*prevp = f->release_next;
> +			f->release_next = NULL;
> +			f->scheduled_for_release = false;
>  		}
> -		f = f->release_next;
> +		f = *prevp;
>  	}
>  	spin_unlock_irqrestore(&kmmio_lock, flags);
>  
> @@ -510,6 +517,9 @@ void unregister_kmmio_probe(struct
> kmmio_probe *p) kmmio_count--;
>  	spin_unlock_irqrestore(&kmmio_lock, flags);
>  
> +	if (!release_list)
> +		return;
> +
>  	drelease = kmalloc(sizeof(*drelease), GFP_ATOMIC);
>  	if (!drelease) {
>  		pr_crit("leaking kmmio_fault_page objects.\n");
> -- 
> 1.7.1
> 
> 


-- 
Pekka Paalanen
http://www.iki.fi/pq/