linux-kernel - Re: [PATCH v2 5/5] mm/mprotect: do not flush on permission promotion

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]

Date:   Mon, 25 Oct 2021 13:12:28 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     Nadav Amit <nadav.amit@...il.com>
Cc:     linux-mm@...ck.org, linux-kernel@...r.kernel.org,
        Nadav Amit <namit@...are.com>,
        Andrea Arcangeli <aarcange@...hat.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Andy Lutomirski <luto@...nel.org>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Peter Xu <peterx@...hat.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Will Deacon <will@...nel.org>, Yu Zhao <yuzhao@...gle.com>,
        Nick Piggin <npiggin@...il.com>, x86@...nel.org
Subject: Re: [PATCH v2 5/5] mm/mprotect: do not flush on permission promotion

On Thu, Oct 21, 2021 at 05:21:12AM -0700, Nadav Amit wrote:
> +/*
> + * pte_may_need_flush() checks whether permissions were demoted and require a
> + * flush. It should only be used for userspace PTEs.
> + */
> +static inline bool pte_may_need_flush(pte_t oldpte, pte_t newpte)
> +{
> +	/* new is non-present: need only if old is present */
> +	if (!pte_present(newpte))
> +		return pte_present(oldpte);
> +
> +	/* old is not present: no need for flush */
> +	if (!pte_present(oldpte))
> +		return false;

Would it not be clearer to write the above like:

	/* !PRESENT -> * ; no need for flush */
	if (!pte_present(oldpte))
		return false;

	/* PRESENT -> !PRESENT ; needs flush */
	if (!pte_present(newpte))
		return true;

?


> diff --git a/mm/mprotect.c b/mm/mprotect.c
> index 0f5c87af5c60..6179c82ea72d 100644
> --- a/mm/mprotect.c
> +++ b/mm/mprotect.c
> @@ -141,7 +141,8 @@ static unsigned long change_pte_range(struct mmu_gather *tlb,
>  				ptent = pte_mkwrite(ptent);
>  			}
>  			ptep_modify_prot_commit(vma, addr, pte, oldpte, ptent);
> -			tlb_flush_pte_range(tlb, addr, PAGE_SIZE);
> +			if (pte_may_need_flush(oldpte, ptent))
> +				tlb_flush_pte_range(tlb, addr, PAGE_SIZE);
>  			pages++;
>  		} else if (is_swap_pte(oldpte)) {
>  			swp_entry_t entry = pte_to_swp_entry(oldpte);

One question on naming, "may_need" sounds a bit washy to me, either it
does or it does not. I suppose you're trying to convey the fact that we
ought to err towards too many TLBi rather than too few, but that's
always true.

That is, would "needs" not be a better name?