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

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]
Message-ID: <e3247625-b25c-a18a-a494-f1e9a0148932@redhat.com>
Date:   Mon, 28 Jan 2019 21:02:52 +0100
From:   David Hildenbrand <david@...hat.com>
To:     linux-mm@...ck.org
Cc:     linux-kernel@...r.kernel.org,
        Andrew Morton <akpm@...ux-foundation.org>,
        Mel Gorman <mgorman@...hsingularity.net>,
        "Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>,
        Michal Hocko <mhocko@...e.com>,
        Naoya Horiguchi <n-horiguchi@...jp.nec.com>,
        Jan Kara <jack@...e.cz>,
        Andrea Arcangeli <aarcange@...hat.com>,
        Dominik Brodowski <linux@...inikbrodowski.net>,
        Matthew Wilcox <willy@...radead.org>,
        Vratislav Bendel <vbendel@...hat.com>,
        Rafael Aquini <aquini@...hat.com>,
        Konstantin Khlebnikov <k.khlebnikov@...sung.com>,
        Minchan Kim <minchan@...nel.org>, stable@...r.kernel.org
Subject: Re: [PATCH v1] mm: migrate: don't rely on PageMovable() of newpage
 after unlocking it

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@...ux-foundation.org>
> Cc: Mel Gorman <mgorman@...hsingularity.net>
> Cc: "Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>
> Cc: Michal Hocko <mhocko@...e.com>
> Cc: Naoya Horiguchi <n-horiguchi@...jp.nec.com>
> Cc: Jan Kara <jack@...e.cz>
> Cc: Andrea Arcangeli <aarcange@...hat.com>
> Cc: Dominik Brodowski <linux@...inikbrodowski.net>
> Cc: Matthew Wilcox <willy@...radead.org>
> Cc: Vratislav Bendel <vbendel@...hat.com>
> Cc: Rafael Aquini <aquini@...hat.com>
> Cc: Konstantin Khlebnikov <k.khlebnikov@...sung.com>
> Cc: Minchan Kim <minchan@...nel.org>
> Cc: stable@...r.kernel.org # 3.12+
> Fixes: d6d86c0a7f8d ("mm/balloon_compaction: redesign ballooned pages management")
> Reported-by: Vratislav Bendel <vbendel@...hat.com>
> Acked-by: Michal Hocko <mhocko@...e.com>
> Acked-by: Rafael Aquini <aquini@...hat.com>
> Signed-off-by: David Hildenbrand <david@...hat.com>
> ---
>  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