Re: [PATCH v1] mm: migrate: don't rely on PageMovable() of newpage after unlocking it

[David Hildenbrand]

On 28.01.19 17:04, David Hildenbrand wrote:
> While debugging some crashes related to virtio-balloon deflation that
> happened under the old balloon migration code, I stumbled over a race
> that still exists today.
> 
> What we experienced:
> 
> drivers/virtio/virtio_balloon.c: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
> 
> Turns out after having added the page to a local list when dequeuing,
> the page would suddenly be moved to an LRU list before we would free it
> via the local list, corrupting both lists. So a page we own and that is
> !LRU was moved to an LRU list.
> 
> In __unmap_and_move(), we lock the old and newpage and perform the
> migration. In case of vitio-balloon, the new page will become
> movable, the old page will no longer be movable.
> 
> However, after unlocking newpage, there is nothing stopping the newpage
> from getting dequeued and freed by virtio-balloon. This
> will result in the newpage
> 1. No longer having PageMovable()
> 2. Getting moved to the local list before finally freeing it (using
>    page->lru)
> 
> Back in the migration thread in __unmap_and_move(), we would after
> unlocking the newpage suddenly no longer have PageMovable(newpage) and
> will therefore call putback_lru_page(newpage), modifying page->lru
> although that list is still in use by virtio-balloon.
> 
> To summarize, we have a race between migrating the newpage and checking
> for PageMovable(newpage). Instead of checking PageMovable(newpage), we
> can simply rely on is_lru of the original page.
> 
> Looks like this was introduced by d6d86c0a7f8d ("mm/balloon_compaction:
> redesign ballooned pages management"), which was backported up to 3.12.
> Old compaction code used PageBalloon() via -_is_movable_balloon_page()
> instead of PageMovable(), however with the same semantics.
> 
> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> 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+
> Fixes: d6d86c0a7f8d ("mm/balloon_compaction: redesign ballooned pages management")
> Reported-by: Vratislav Bendel <vbendel@xxxxxxxxxx>
> Acked-by: Michal Hocko <mhocko@xxxxxxxx>
> Acked-by: Rafael Aquini <aquini@xxxxxxxxxx>
> Signed-off-by: David Hildenbrand <david@xxxxxxxxxx>
> ---
>  mm/migrate.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 4512afab46ac..31e002270b05 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1135,10 +1135,12 @@ static int __unmap_and_move(struct page *page, struct page *newpage,
>  	 * If migration is successful, decrease refcount of the newpage
>  	 * which will not free the page because new page owner increased
>  	 * refcounter. As well, if it is LRU page, add the page to LRU
> -	 * list in here.
> +	 * list in here. Don't rely on PageMovable(newpage), as that could
> +	 * already have changed after unlocking newpage (e.g.
> +	 * virtio-balloon deflation).
>  	 */
>  	if (rc == MIGRATEPAGE_SUCCESS) {
> -		if (unlikely(__PageMovable(newpage)))
> +		if (unlikely(!is_lru))
>  			put_page(newpage);
>  		else
>  			putback_lru_page(newpage);
> 

Vratislav just pointed out that this issue should not happen on upstream
as __PageMovable(newpage) will still return true even after
__ClearPageMovable(newpage). Only PageMovable(newpage) would actually
return false.

(not sure if I am happy about this, this is horribly confusing and
complicated)

I am not 100% sure yet, but I guess Vratislav is right. So it was
effectively fixed by

b1123ea6d3b3 ("mm: balloon: use general non-lru movable page feature"),
which checks for __PageMovable(newpage) instead of
__is_movable_balloon_page(newpage).

Anybody wanting to fix stable kernels either has to backport something
proposed in this patch or b1123ea6d3b3.

-- 

Thanks,

David / dhildenb