Re: [PATCH v2 for-4.4-stable] mm: migrate: don't rely on __PageMovable() of newpage after unlocking it

[David Hildenbrand <david@redhat.com>]

On 01.02.19 15:09, Greg KH wrote:
> On Fri, Feb 01, 2019 at 02:43:47PM +0100, David Hildenbrand wrote:
>> This is the backport for 4.4-stable.
>>
>> We had a race in the old balloon compaction code before commit b1123ea6d3b3
>> ("mm: balloon: use general non-lru movable page feature") refactored it
>> that became visible after backporting commit 195a8c43e93d
>> ("virtio-balloon: deflate via a page list") without the refactoring.
>>
>> The bug existed from commit d6d86c0a7f8d ("mm/balloon_compaction: redesign
>> ballooned pages management") till commit b1123ea6d3b3 ("mm: balloon: use
>> general non-lru movable page feature"). commit d6d86c0a7f8d
>> ("mm/balloon_compaction: redesign ballooned pages management") was
>> backported to 3.12, so the broken kernels are stable kernels [3.12 - 4.7].
>>
>> There was a subtle race between dropping the page lock of the newpage
>> in __unmap_and_move() and checking for
>> __is_movable_balloon_page(newpage).
>>
>> Just after dropping this page lock, virtio-balloon could go ahead and
>> deflate the newpage, effectively dequeueing it and clearing PageBalloon,
>> in turn making __is_movable_balloon_page(newpage) fail.
>>
>> This resulted in dropping the reference of the newpage via
>> putback_lru_page(newpage) instead of put_page(newpage), leading to
>> page->lru getting modified and a !LRU page ending up in the LRU lists.
>> With commit 195a8c43e93d ("virtio-balloon: deflate via a page list")
>> backported, one would suddenly get corrupted lists in
>> release_pages_balloon():
>> - WARNING: CPU: 13 PID: 6586 at lib/list_debug.c:59 __list_del_entry+0xa1/0xd0
>> - list_del corruption. prev->next should be ffffe253961090a0, but was dead000000000100
>>
>> Nowadays this race is no longer possible, but it is hidden behind very
>> ugly handling of __ClearPageMovable() and __PageMovable().
>>
>> __ClearPageMovable() will not make __PageMovable() fail, only
>> PageMovable(). So the new check (__PageMovable(newpage)) will still hold
>> even after newpage was dequeued by virtio-balloon.
>>
>> If anybody would ever change that special handling, the BUG would be
>> introduced again. So instead, make it explicit and use the information
>> of the original isolated page before migration.
>>
>> This patch can be backported fairly easy to stable kernels (in contrast
>> to the refactoring).
>>
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> Cc: Mel Gorman <mgorman@techsingularity.net>
>> Cc: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
>> Cc: Michal Hocko <mhocko@suse.com>
>> Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
>> Cc: Jan Kara <jack@suse.cz>
>> Cc: Andrea Arcangeli <aarcange@redhat.com>
>> Cc: Dominik Brodowski <linux@dominikbrodowski.net>
>> Cc: Matthew Wilcox <willy@infradead.org>
>> Cc: Vratislav Bendel <vbendel@redhat.com>
>> Cc: Rafael Aquini <aquini@redhat.com>
>> Cc: Konstantin Khlebnikov <k.khlebnikov@samsung.com>
>> Cc: Minchan Kim <minchan@kernel.org>
>> Cc: Sasha Levin <sashal@kernel.org>
>> Cc: stable@vger.kernel.org # 3.12 - 4.7
>> Fixes: d6d86c0a7f8d ("mm/balloon_compaction: redesign ballooned pages management")
>> Reported-by: Vratislav Bendel <vbendel@redhat.com>
>> Acked-by: Michal Hocko <mhocko@suse.com>
>> Acked-by: Rafael Aquini <aquini@redhat.com>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  mm/migrate.c | 7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> What is the git commit id of this patch in Linus's tree?

It's still in Andrew's tree as far as I know. I just wanted to share the
backport right away (to show that it is easy :) ). Will resend it again
(with proper commit idea) once upstream.

> 
> thanks,
> 
> greg k-h
> 


-- 

Thanks,

David / dhildenb