linux-kernel - Re: [PATCH] mm/page_alloc: Fix race conditions on getting migratetype in buffered

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:	Sun, 18 Jan 2015 11:19:55 +0100
From:	Vlastimil Babka <vbabka@...e.cz>
To:	Hui Zhu <teawater@...il.com>, akpm@...ux-foundation.org,
	mgorman@...e.de, hannes@...xchg.org, rientjes@...gle.com,
	iamjoonsoo.kim@....com, sasha.levin@...cle.com,
	linux-kernel@...r.kernel.org, linux-mm@...ck.org
CC:	Hui Zhu <zhuhui@...omi.com>
Subject: Re: [PATCH] mm/page_alloc: Fix race conditions on getting migratetype
 in buffered_rmqueue

On 18.1.2015 10:17, Hui Zhu wrote:
> From: Hui Zhu <zhuhui@...omi.com>
>
> To test the patch [1], I use KGTP and a script [2] to show NR_FREE_CMA_PAGES
> and gross of cma_nr_free.  The values are always not same.
> I check the code of pages alloc and free and found that race conditions
> on getting migratetype in buffered_rmqueue.

Can you elaborate? What does this races with, are you dynamically changing
the size of CMA area, or what? The migratetype here is based on which free
list the page was found on. Was it misplaced then? Wasn't Joonsoo's recent
series supposed to eliminate this?

> Then I add move the code of getting migratetype inside the zone->lock
> protection part.

Not just that, you are also reading migratetype from pageblock bitmap
instead of the one embedded in the free page. Which is more expensive
and we already do that more often than we would like to because of CMA.
And it appears to be a wrong fix for a possible misplacement bug. If there's
such misplacement, the wrong stats are not the only problem.

>
> Because this issue will affect system even if the Linux kernel does't
> have [1].  So I post this patch separately.

But we can't test that without [1], right? Maybe the issue is introduced 
by [1]?

>
> This patchset is based on fc7f0dd381720ea5ee5818645f7d0e9dece41cb0.
>
> [1] https://lkml.org/lkml/2015/1/18/28
> [2] https://github.com/teawater/kgtp/blob/dev/add-ons/cma_free.py
>
> Signed-off-by: Hui Zhu <zhuhui@...omi.com>
> ---
>   mm/page_alloc.c | 11 +++++++----
>   1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 7633c50..f3d6922 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -1694,11 +1694,12 @@ again:
>   		}
>   		spin_lock_irqsave(&zone->lock, flags);
>   		page = __rmqueue(zone, order, migratetype);
> +		if (page)
> +			migratetype = get_pageblock_migratetype(page);
> +		else
> +			goto failed_unlock;
>   		spin_unlock(&zone->lock);
> -		if (!page)
> -			goto failed;
> -		__mod_zone_freepage_state(zone, -(1 << order),
> -					  get_freepage_migratetype(page));
> +		__mod_zone_freepage_state(zone, -(1 << order), migratetype);
>   	}
>   
>   	__mod_zone_page_state(zone, NR_ALLOC_BATCH, -(1 << order));
> @@ -1715,6 +1716,8 @@ again:
>   		goto again;
>   	return page;
>   
> +failed_unlock:
> +	spin_unlock(&zone->lock);
>   failed:
>   	local_irq_restore(flags);
>   	return NULL;

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/