linux-kernel - Re: [PATCH v4 2/4] mm: Add function to support extra actions on swap in/out

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: <b31a3aef-26d9-6d3a-109b-c8453a3a2aef@linux.intel.com>
Date:   Wed, 11 Jan 2017 08:56:06 -0800
From:   Dave Hansen <dave.hansen@...ux.intel.com>
To:     Khalid Aziz <khalid.aziz@...cle.com>, akpm@...ux-foundation.org,
        davem@...emloft.net, arnd@...db.de
Cc:     kirill.shutemov@...ux.intel.com, mhocko@...e.com,
        jmarchan@...hat.com, vbabka@...e.cz, dan.j.williams@...el.com,
        lstoakes@...il.com, hannes@...xchg.org, mgorman@...e.de,
        hughd@...gle.com, vdavydov.dev@...il.com, minchan@...nel.org,
        namit@...are.com, linux-arch@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-mm@...ck.org,
        sparclinux@...r.kernel.org, Khalid Aziz <khalid@...ehiking.org>
Subject: Re: [PATCH v4 2/4] mm: Add function to support extra actions on swap
 in/out

On 01/11/2017 08:12 AM, Khalid Aziz wrote:
> +#ifndef set_swp_pte_at
> +#define set_swp_pte_at(mm, addr, ptep, pte, oldpte)	\
> +		set_pte_at(mm, addr, ptep, pte)
> +#endif

BTW, thanks for the *much* improved description of the series.  This is
way easier to understand.

I really don't think this is the interface we want, though.
set_swp_pte_at() is really doing *two* things:
1. Detecting _PAGE_MCD_4V and squirreling the MCD data away at swap-out
2. Reading back in the MCD data at swap-on

You're effectively using (!pte_none(pte) && !pte_present(pte)) to
determine whether you're at swap in or swap out time.  That's goofy, IMNHO.

It isn't obvious from the context, but this hunk is creating a migration
PTE.  Why is ADI tag manipulation needed?  We're just changing the
physical address of the underlying memory, but neither the
application-visible contents nor the tags are changing.

> @@ -1539,7 +1539,7 @@ static int try_to_unmap_one(struct page *page, struct vm_area_struct *vma,
>  		swp_pte = swp_entry_to_pte(entry);
>  		if (pte_soft_dirty(pteval))
>  			swp_pte = pte_swp_mksoft_dirty(swp_pte);
> -		set_pte_at(mm, address, pte, swp_pte);
> +		set_swp_pte_at(mm, address, pte, swp_pte, pteval);
>  	} else if (PageAnon(page)) {
>  		swp_entry_t entry = { .val = page_private(page) };
>  		pte_t swp_pte;

Which means you're down to a single call that does swap-out, and a
single call that does swap-in.  There's no reason to hide all your code
behind set_pte_at().

Just add a new arch-specific call that takes the VMA and the swap PTE
and stores the ADI bit in there, here:

> @@ -1572,7 +1572,7 @@ static int try_to_unmap_one(struct page *page, struct vm_area_struct *vma,
>  		swp_pte = swp_entry_to_pte(entry);
>  		if (pte_soft_dirty(pteval))
>  			swp_pte = pte_swp_mksoft_dirty(swp_pte);
> -		set_pte_at(mm, address, pte, swp_pte);
> +		set_swp_pte_at(mm, address, pte, swp_pte, pteval);
>  	} else

and in do_swap_page(), do the opposite with a second, new call.