[Devel] [PATCH rh7 v2] tcache: fix use-after-free in tcache_invalidate_node_pages()

[Vladimir Davydov]

On Wed, Dec 09, 2015 at 12:44:34PM +0300, Andrey Ryabinin wrote:
> tcache_invalidate_node_pages() temporarly drops/takes back node->tree_lock.
> Once lock was dropped, we can't continue iterating to the next slot, because
> another thread might remove and free it. If lock was dropped tree iteration
> has to be restarted.
> Wit this patch we also drop the lock iff we need to resched the task.
> 
> https://jira.sw.ru/browse/PSBM-42104
> 
> Signed-off-by: Andrey Ryabinin <aryabinin at virtuozzo.com>
> ---
>  mm/tcache.c | 23 +++++++++++++++--------
>  1 file changed, 15 insertions(+), 8 deletions(-)
> 
> diff --git a/mm/tcache.c b/mm/tcache.c
> index b8757cf..a09ae49 100644
> --- a/mm/tcache.c
> +++ b/mm/tcache.c
> @@ -662,6 +662,7 @@ tcache_invalidate_node_pages(struct tcache_node *node)
>  	struct radix_tree_iter iter;
>  	struct page *page;
>  	void **slot;
> +	pgoff_t index = 0;
>  
>  	spin_lock_irq(&node->tree_lock);
>  
> @@ -674,19 +675,25 @@ tcache_invalidate_node_pages(struct tcache_node *node)
>  	 * Now truncate all pages. Be careful, because pages can still be
>  	 * deleted from this node by the shrinker or by concurrent lookups.
>  	 */
> -	radix_tree_for_each_slot(slot, &node->page_tree, &iter, 0) {
> +restart:
> +	radix_tree_for_each_slot(slot, &node->page_tree, &iter, index) {
>  		page = radix_tree_deref_slot_protected(slot, &node->tree_lock);
>  		BUG_ON(!__tcache_page_tree_delete(node, page->index, page));
> -		spin_unlock(&node->tree_lock);
> -
>  		tcache_lru_del(page);
>  		put_page(page);

You forgot to update the comment describing the locking rules. Please
do.

>  
> -		local_irq_enable();
> -		cond_resched();
> -		local_irq_disable();
> -
> -		spin_lock(&node->tree_lock);
> +		if (need_resched()) {
> +			spin_unlock_irq(&node->tree_lock);
> +			cond_resched();
> +			spin_lock_irq(&node->tree_lock);
> +			/*
> +			 * Restart iteration over the radix tree, because the
> +			 * current node could have been freed when we dropped
> +			 * the lock.
> +			 */
> +			index = page->index + 1;

We released the page, so its ->index might be irrelevant. We must read
it before calling put_page.

> +			goto restart;
> +		}
>  	}
>  
>  	BUG_ON(node->nr_pages != 0);