flush_cache_vmap() must be called after new vmalloc mappings are
installed in the page table in order to allow architectures to make sure
the new mapping is visible.

Fixes: 3e9a9e256b1e ("mm: add a vmap_pfn function")
Reported-by: Dylan Jhong <dylan@andestech.com>
Closes: https://lore.kernel.org/linux-riscv/ZMytNY2J8iyjbPPy@atctrx.andestech.com/
Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
---
 mm/vmalloc.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index 93cf99aba335..228a4a5312f2 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -2979,6 +2979,10 @@ void *vmap_pfn(unsigned long *pfns, unsigned int count, pgprot_t prot)
 		free_vm_area(area);
 		return NULL;
 	}
+
+	flush_cache_vmap((unsigned long)area->addr,
+			 (unsigned long)area->addr + count * PAGE_SIZE);
+
 	return area->addr;
 }
 EXPORT_SYMBOL_GPL(vmap_pfn);
-- 
2.39.2

On Wed, Aug 09, 2023 at 06:46:33PM +0200, Alexandre Ghiti wrote:
> flush_cache_vmap() must be called after new vmalloc mappings are
> installed in the page table in order to allow architectures to make sure
> the new mapping is visible.
> 
> Fixes: 3e9a9e256b1e ("mm: add a vmap_pfn function")
> Reported-by: Dylan Jhong <dylan@andestech.com>
> Closes: https://lore.kernel.org/linux-riscv/ZMytNY2J8iyjbPPy@atctrx.andestech.com/
> Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
> ---
>  mm/vmalloc.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index 93cf99aba335..228a4a5312f2 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -2979,6 +2979,10 @@ void *vmap_pfn(unsigned long *pfns, unsigned int count, pgprot_t prot)
>  		free_vm_area(area);
>  		return NULL;
>  	}
> +
> +	flush_cache_vmap((unsigned long)area->addr,
> +			 (unsigned long)area->addr + count * PAGE_SIZE);
> +
>  	return area->addr;
>  }
>  EXPORT_SYMBOL_GPL(vmap_pfn);
> -- 
> 2.39.2
> 

Hi Alex,

Looks good to me. Thanks.
Reviewed-by: Dylan Jhong <dylan@andestech.com>

Best regards,
Dylan Jhong

On Wed, Aug 09, 2023 at 06:46:33PM +0200, Alexandre Ghiti wrote:
> flush_cache_vmap() must be called after new vmalloc mappings are
> installed in the page table in order to allow architectures to make sure
> the new mapping is visible.

Looks good.  I somehow vaguely remember seing a patch like this floating
around before as part of a series, but if that didn't make it it
certainly should now.

Reviewed-by: Christoph Hellwig <hch@lst.de>

On Wed, 09 Aug 2023 15:25:19 PDT (-0700), Christoph Hellwig wrote:
> On Wed, Aug 09, 2023 at 06:46:33PM +0200, Alexandre Ghiti wrote:
>> flush_cache_vmap() must be called after new vmalloc mappings are
>> installed in the page table in order to allow architectures to make sure
>> the new mapping is visible.
>
> Looks good.  I somehow vaguely remember seing a patch like this floating
> around before as part of a series, but if that didn't make it it
> certainly should now.
>
> Reviewed-by: Christoph Hellwig <hch@lst.de>

I think we're likely to end up with performance problems around here, 
but at least it's correct.  If someone has performance

Dylan: this fixes your breakage as well, right?

I've queued it up for testing, but I doubt QEMU would find any issues 
here.  My build box has been slow lately, but it should end up in fixes 
later today.

On Thu, 10 Aug 2023 08:13:56 PDT (-0700), Palmer Dabbelt wrote:
> On Wed, 09 Aug 2023 15:25:19 PDT (-0700), Christoph Hellwig wrote:
>> On Wed, Aug 09, 2023 at 06:46:33PM +0200, Alexandre Ghiti wrote:
>>> flush_cache_vmap() must be called after new vmalloc mappings are
>>> installed in the page table in order to allow architectures to make sure
>>> the new mapping is visible.
>>
>> Looks good.  I somehow vaguely remember seing a patch like this floating
>> around before as part of a series, but if that didn't make it it
>> certainly should now.
>>
>> Reviewed-by: Christoph Hellwig <hch@lst.de>
>
> I think we're likely to end up with performance problems around here,
> but at least it's correct.  If someone has performance
>
> Dylan: this fixes your breakage as well, right?
>
> I've queued it up for testing, but I doubt QEMU would find any issues
> here.  My build box has been slow lately, but it should end up in fixes
> later today.

Sorry about that, I'm in the wrong thread -- I meant to be over here 
<https://lore.kernel.org/all/20230725132246.817726-1-alexghiti@rivosinc.com/>.

Reviewed-by: Palmer Dabbelt <palmer@rivosinc.com>
Acked-by: Palmer Dabbelt <palmer@rivosinc.com>

but I'm not taking this via the RISC-V tree unless someone asks.

On Wed,  9 Aug 2023 18:46:33 +0200 Alexandre Ghiti <alexghiti@rivosinc.com> wrote:

> flush_cache_vmap() must be called after new vmalloc mappings are
> installed in the page table in order to allow architectures to make sure
> the new mapping is visible.

Thanks.  What are the user-visible effects of this bug?

Hi Andrew,

On Wed, Aug 9, 2023 at 8:46 PM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Wed,  9 Aug 2023 18:46:33 +0200 Alexandre Ghiti <alexghiti@rivosinc.com> wrote:
>
> > flush_cache_vmap() must be called after new vmalloc mappings are
> > installed in the page table in order to allow architectures to make sure
> > the new mapping is visible.
>
> Thanks.  What are the user-visible effects of this bug?

It could lead to a panic since on some architectures (like powerpc),
the page table walker could see the wrong pte value and trigger a
spurious page fault that can not be resolved (see commit f1cb8f9beba8
("powerpc/64s/radix: avoid ptesync after set_pte and
ptep_set_access_flags")).

But actually the patch is aiming at riscv: the riscv specification
allows the caching of invalid entries in the TLB, and since we
recently removed the vmalloc page fault handling, we now need to emit
a tlb shootdown whenever a new vmalloc mapping is emitted
(https://lore.kernel.org/linux-riscv/20230725132246.817726-1-alexghiti@rivosinc.com/).
That's a temporary solution, there are ways to avoid that :)

Thanks,

Alex