lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Tue, 18 Oct 2016 16:56:48 -0400
From:   Steven Rostedt <rostedt@...dmis.org>
To:     Christoph Hellwig <hch@....de>
Cc:     akpm@...ux-foundation.org, joelaf@...gle.com, jszhang@...vell.com,
        chris@...is-wilson.co.uk, joaodias@...gle.com, linux-mm@...ck.org,
        linux-rt-users@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 6/6] mm: add preempt points into __purge_vmap_area_lazy

On Tue, Oct 18, 2016 at 08:56:11AM +0200, Christoph Hellwig wrote:
> From: Joel Fernandes <joelaf@...gle.com>
> 
> Use cond_resched_lock to avoid holding the vmap_area_lock for a
> potentially long time.
> 
> Signed-off-by: Joel Fernandes <joelaf@...gle.com>
> [hch: split from a larger patch by Joel, wrote the crappy changelog]
> Signed-off-by: Christoph Hellwig <hch@....de>
> ---
>  mm/vmalloc.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index 6c7eb8d..98b19ea 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -628,7 +628,7 @@ static bool __purge_vmap_area_lazy(unsigned long start, unsigned long end)
>  	struct llist_node *valist;
>  	struct vmap_area *va;
>  	struct vmap_area *n_va;
> -	int nr = 0;
> +	bool do_free = false;
>  
>  	lockdep_assert_held(&vmap_purge_lock);
>  
> @@ -638,18 +638,22 @@ static bool __purge_vmap_area_lazy(unsigned long start, unsigned long end)
>  			start = va->va_start;
>  		if (va->va_end > end)
>  			end = va->va_end;
> -		nr += (va->va_end - va->va_start) >> PAGE_SHIFT;
> +		do_free = true;
>  	}
>  
> -	if (!nr)
> +	if (!do_free)
>  		return false;
>  
> -	atomic_sub(nr, &vmap_lazy_nr);
>  	flush_tlb_kernel_range(start, end);
>  
>  	spin_lock(&vmap_area_lock);
> -	llist_for_each_entry_safe(va, n_va, valist, purge_list)
> +	llist_for_each_entry_safe(va, n_va, valist, purge_list) {
> +		int nr = (va->va_end - va->va_start) >> PAGE_SHIFT;
> +
>  		__free_vmap_area(va);
> +		atomic_sub(nr, &vmap_lazy_nr);
> +		cond_resched_lock(&vmap_area_lock);

Is releasing the lock within a llist_for_each_entry_safe() actually safe? Is
vmap_area_lock the one to protect the valist?

That is llist_for_each_entry_safe(va, n_va, valist, purg_list) does:

	for (va = llist_entry(valist, typeof(*va), purge_list);
	     &va->purge_list != NULL &&
	     n_va = llist_entry(va->purge_list.next, typeof(*n_va),
				purge_list, true);
	     pos = n)

Thus n_va is pointing to the next element to process when we release the
lock. Is it possible for another task to get into this same path and process
the item that n_va is pointing to? Then when the preempted task comes back,
grabs the vmap_area_lock, and then continues the loop with what n_va has,
could that cause problems? That is, the next iteration after releasing the
lock does va = n_va. What happens if n_va no longer exits?

I don't know this code that well, and perhaps vmap_area_lock is not protecting
the list and this is all fine.

-- Steve


> +	}
>  	spin_unlock(&vmap_area_lock);
>  	return true;
>  }
> -- 
> 2.1.4

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ