linux-kernel - Re: flush_dcache_page vs kunmap

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: <CAHk-=wjv--WRm9ay-D615xRwe+tUhZSg7dM0h32Rcf5TNMrD1g@mail.gmail.com>
Date:   Thu, 4 Nov 2021 11:23:56 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Catalin Marinas <catalin.marinas@....com>
Cc:     Matthew Wilcox <willy@...radead.org>,
        Christoph Hellwig <hch@....de>,
        linux-arch <linux-arch@...r.kernel.org>,
        Ira Weiny <ira.weiny@...el.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Russell King <linux@...linux.org.uk>
Subject: Re: flush_dcache_page vs kunmap_local

On Thu, Nov 4, 2021 at 11:04 AM Catalin Marinas <catalin.marinas@....com> wrote:
>
> Luckily I don't think we have a (working) SMP system with VIVT caches.
> On UP, looking at arm, for VIVT caches it flushes the D-cache before
> kunmap_local() (arch_kmap_local_pre_unmap()). So any new kmap_local()
> would see the correct data even if it's in a different location.

Ok, good.

Yeah, because kmap_local and SMP really would seem to be a "that can't
work with VIVT".

> We still have VIVT processors supported in the kernel and a few where
> the VIPT cache is aliasing (some ARMv6 CPUs). On these,
> flush_dcache_page() is still used to ensure the user aliases are
> coherent with the kernel one, so it's not just about the I/D-cache
> coherency.

Maybe we could try to split it up and make each function have more
well-defined rules? One of the issues with the flush_dcache thing is
that it's always been so ad-hoc and it's not been hugely clear.

For example, people seem to think it's purely about flushing writes.
But for the virtual aliasing issue and kmap, you may need to flush
purely between reads too - just to make sure that you don't have any
stale contents.

That's why kunmap needs to have some unconditional cache flush thing
for the virtual aliasing issue.

But hey, it's entirely possible that it should *not* have that
"flush_dcache_page()" thing, but something that is private to the
architecture.

So VIVT arm (and whoever else) would continue to do the cache flushing
at kunmap_local time (or kmap - I don't think it matters which one you
do, as long as you make sure there are no stale contents from the
previous use of that address).

And then we'd relegate flush_dcache_page() purely for uses where
somebody modifies the data and wants to make sure it ends up being
coherent with subsequent uses (whether kmap and VIVT or I$/D$
coherency issues)?

> The cachetlb.rst doc states the two cases where flush_dcache_page()
> should be called:
>
> 1. After writing to a page cache page (that's what we need on arm64 for
>    the I-cache).
>
> 2. Before reading from a page cache page and user mappings potentially
>    exist. I think arm32 ensures the D-cache user aliases are coherent
>    with the kernel one (added rmk to confirm).

I think the "kernel cache coherency" matters too. The PTE contents
thing seems relevant if we use kmap for that...

So I do think that the "page cache or user mapping" is not necessarily
the only case.

But personally I consider these situations so broken at a hardware
level that I can't find it in myself to care too deeply.

Because user space with non-coherent I$/D$ should do its own cache
flushing if it does "read()" to modify an executable range - exactly
the same way it has to do it for just doing regular stores to that
range.

It really shouldn't be the kernel that cares at all.

             Linus