linux-kernel - Re: [PATCH v1 2/4] ioremap: Invalidate TLB after huge mappings

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: <20180314114814.mzgwlzkwzxijiv54@lakrids.cambridge.arm.com>
Date:   Wed, 14 Mar 2018 11:48:15 +0000
From:   Mark Rutland <mark.rutland@....com>
To:     Chintan Pandya <cpandya@...eaurora.org>
Cc:     catalin.marinas@....com, will.deacon@....com, arnd@...db.de,
        ard.biesheuvel@...aro.org, marc.zyngier@....com,
        james.morse@....com, kristina.martsenko@....com,
        takahiro.akashi@...aro.org, gregkh@...uxfoundation.org,
        tglx@...utronix.de, linux-arm-kernel@...ts.infradead.org,
        linux-kernel@...r.kernel.org, linux-arch@...r.kernel.org,
        akpm@...ux-foundation.org, toshi.kani@....com
Subject: Re: [PATCH v1 2/4] ioremap: Invalidate TLB after huge mappings

On Wed, Mar 14, 2018 at 04:50:35PM +0530, Chintan Pandya wrote:
> On 3/14/2018 4:18 PM, Mark Rutland wrote:
> > On Wed, Mar 14, 2018 at 02:18:23PM +0530, Chintan Pandya wrote:
> > As has been noted in previous threads, the ARM architecture requires a
> > Break-Before-Make sequence when changing an entry from a table to a
> > block, as is the case here.
> > 
> > The means the necessary sequence is:
> > 
> > 	1. Make the entry invalid
> > 	2. Invalidate relevant TLB entries
> > 	3. Write the new entry
> > 
> We do this for PTEs. I don't see this applicable to PMDs.

The architecture requires this for *all* levels of page table, when
certain changes are made. Switching an entry from a table to block (or
vice versa) is one of those changes, and this definitely applies to
PMDs.

> Because,
> 
> 1) To mark any PMD invalid, we need to be sure that next level page
>    table (I mean all the 512 PTEs) should be zero. That requires us
>    to scan entire last level page. A big perf hit !

This is in ioremap code. Under what workload does this constitute a perf
hit?

Regardless, so long as we mark the pmd entry invalid before the TLB
invalidation, we don't need to touch the next level table at all. We
just require a sequence like:

	pmd_clear(*pmdp);
	flush_tlb_kernel_range(pmd_start_addr, pmd_end_addr);
	pmd_set_huge(*pmdp, phys, prot);

> 2) We need to perform step 1 for every unmap as we never know which
>    unmap will make last level page table empty.

Sorry, I don't follow. Could you elaborate on the problem?

> Moreover, problem comes only when 4K mapping was followed by 2M
> mapping. In all other cases, retaining valid PMD has obvious perf
> gain. That's what walk-cache is supposed to be introduced for.

Retaining a valid PMD in the TLB that *differs* from a valid PMD in the
page tables is a big problem.

The architecture requires BBM, as this permits CPUs to allocate PMDs
into TLBs at *any* time, even if there's already PMD in the TLB for a
given address.

Thus, CPUs can allocate *both* valid PMDs into the TLBs. When this
happens, a TLB lookup can:

1) return either of the PMDs.

2) raise a TLB conflict abort.

3) return an amalgamation of the two entries (e.g. provide an erroneous
   address).

Note that (3) is particularly scary:

* The CPU could raise an SError if the amalgamated entry is junk.

* If a memory access hits an amalgamated entry, it may use the wrong
  physical address, attributes, or permissions, resulting in a number of
  potential problems.

* If the amalgamated entry looks like a partial walk, the TLB might try
  to perform a walk starting at the physical address in the amalgamated
  entry. This would cause page table walks to access bogus addresses,
  allocating junk into TLBs, and may result in SErrors or other aborts.

> > Whereas above, the sequence is
> > 
> > 	1. Write the new entry
> > 	2. invalidate relevant TLB entries
> > 
> > Which is insufficient, and will lead to a number of problems.
> I couldn't think of new problems with this approach. Could you share
> any problematic scenarios ?

Please see above.

> Also, my test-case runs fine with these patches for 10+ hours.

While this may happen to work on particular platforms, it is not
guaranteed per the architecture, and will fail on others.

Thanks,
Mark.