On 04.02.19 10:00, David Hildenbrand wrote:
> commit e0a352fabce61f730341d119fbedf71ffdb8663f upstream.
>
> We had a race in the old balloon compaction code before b1123ea6d3b3
> ("mm: balloon: use general non-lru movable page feature") refactored it
> that became visible after backporting 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 b1123ea6d3b3 ("mm: balloon:
> use general non-lru movable page feature"). 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 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).
>
> Link: http://lkml.kernel.org/r/20190129233217.10747-1-david@xxxxxxxxxx
> Fixes: d6d86c0a7f8d ("mm/balloon_compaction: redesign ballooned pages management")
> Signed-off-by: David Hildenbrand <david@xxxxxxxxxx>
> Reported-by: Vratislav Bendel <vbendel@xxxxxxxxxx>
> Acked-by: Michal Hocko <mhocko@xxxxxxxx>
> Acked-by: Rafael Aquini <aquini@xxxxxxxxxx>
> Cc: Mel Gorman <mgorman@xxxxxxxxxxxxxxxxxxx>
> Cc: "Kirill A. Shutemov" <kirill.shutemov@xxxxxxxxxxxxxxx>
> Cc: Michal Hocko <mhocko@xxxxxxxx>
> Cc: Naoya Horiguchi <n-horiguchi@xxxxxxxxxxxxx>
> Cc: Jan Kara <jack@xxxxxxx>
> Cc: Andrea Arcangeli <aarcange@xxxxxxxxxx>
> Cc: Dominik Brodowski <linux@xxxxxxxxxxxxxxxxxxxx>
> Cc: Matthew Wilcox <willy@xxxxxxxxxxxxx>
> Cc: Vratislav Bendel <vbendel@xxxxxxxxxx>
> Cc: Rafael Aquini <aquini@xxxxxxxxxx>
> Cc: Konstantin Khlebnikov <k.khlebnikov@xxxxxxxxxxx>
> Cc: Minchan Kim <minchan@xxxxxxxxxx>
> Cc: <stable@xxxxxxxxxxxxxxx> [3.12 - 4.7]
> Signed-off-by: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
Lack of coffee, missed to add my s-o.
Greg, can you add
Signed-off-by: David Hildenbrand <david@xxxxxxxxxx>
?
> ---
> mm/migrate.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/mm/migrate.c b/mm/migrate.c
> index afedcfab60e2..ce88dff1da98 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -936,6 +936,7 @@ static ICE_noinline int unmap_and_move(new_page_t get_new_page,
> int rc = MIGRATEPAGE_SUCCESS;
> int *result = NULL;
> struct page *newpage;
> + bool is_lru = !isolated_balloon_page(page);
>
> newpage = get_new_page(page, private, &result);
> if (!newpage)
> @@ -983,11 +984,13 @@ out:
> /*
> * If migration was not successful and there's a freeing callback, use
> * it. Otherwise, putback_lru_page() will drop the reference grabbed
> - * during isolation.
> + * during isolation. Use the old state of the isolated source page to
> + * determine if we migrated a LRU page. newpage was already unlocked
> + * and possibly modified by its owner - don't rely on the page state.
> */
> if (put_new_page)
> put_new_page(newpage, private);
> - else if (unlikely(__is_movable_balloon_page(newpage))) {
> + else if (rc == MIGRATEPAGE_SUCCESS && unlikely(!is_lru)) {
> /* drop our reference, page already in the balloon */
> put_page(newpage);
> } else
>
--
Thanks,
David / dhildenb
