[PATCH] riscv: pgtable: Enhance set_pte to prevent OoO risk

[PATCH] riscv: pgtable: Enhance set_pte to prevent OoO risk

[guoren]

From: Guo Ren <guoren@linux.alibaba.com>

When changing from an invalid pte to a valid one for a kernel page,
there is no need for tlb_flush. It's okay for the TSO memory model, but
there is an OoO risk for the Weak one. eg:

sd t0, (a0) // a0 = pte address, pteval is changed from invalid to valid
...
ld t1, (a1) // a1 = va of above pte

If the ld instruction is executed speculatively before the sd
instruction. Then it would bring an invalid entry into the TLB, and when
the ld instruction retired, a spurious page fault occurred. Because the
vmemmap has been ignored by vmalloc_fault, the spurious page fault would
cause kernel panic.

This patch was inspired by the commit: 7f0b1bf04511 ("arm64: Fix barriers
used for page table modifications"). For RISC-V, there is no requirement
in the spec to guarantee all tlb entries are valid and no requirement to
PTW filter out invalid entries. Of course, micro-arch could give a more
robust design, but here, use a software fence to guarantee.

Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
Signed-off-by: Guo Ren <guoren@kernel.org>
---
 arch/riscv/include/asm/pgtable.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
index 294044429e8e..2fae5a5438e0 100644
--- a/arch/riscv/include/asm/pgtable.h
+++ b/arch/riscv/include/asm/pgtable.h
@@ -511,6 +511,13 @@ static inline int pte_same(pte_t pte_a, pte_t pte_b)
 static inline void set_pte(pte_t *ptep, pte_t pteval)
 {
 	*ptep = pteval;
+
+	/*
+	 * Only if the new pte is present and kernel, otherwise TLB
+	 * maintenance or update_mmu_cache() have the necessary barriers.
+	 */
+	if (pte_val(pteval) & (_PAGE_PRESENT | _PAGE_GLOBAL))
+		RISCV_FENCE(rw,rw);
 }
 
 void flush_icache_pte(pte_t pte);
-- 
2.40.1

Re: [PATCH] riscv: pgtable: Enhance set_pte to prevent OoO risk

[Guo Ren]

On Fri, Dec 8, 2023 at 11:10 PM <guoren@kernel.org> wrote:
>
> From: Guo Ren <guoren@linux.alibaba.com>
>
> When changing from an invalid pte to a valid one for a kernel page,
> there is no need for tlb_flush. It's okay for the TSO memory model, but
> there is an OoO risk for the Weak one. eg:
Sorry, TSO has no Immunity. The above sentence should be rewritten with:

When changing from an invalid pte to a valid one for a kernel page,
there is no guarantee tlb_flush in Linux. So, there is an OoO risk for
riscv, e.g.:

>
> sd t0, (a0) // a0 = pte address, pteval is changed from invalid to valid
> ...
> ld t1, (a1) // a1 = va of above pte
>
> If the ld instruction is executed speculatively before the sd
> instruction. Then it would bring an invalid entry into the TLB, and when
> the ld instruction retired, a spurious page fault occurred. Because the
> vmemmap has been ignored by vmalloc_fault, the spurious page fault would
> cause kernel panic.
>
> This patch was inspired by the commit: 7f0b1bf04511 ("arm64: Fix barriers
> used for page table modifications"). For RISC-V, there is no requirement
> in the spec to guarantee all tlb entries are valid and no requirement to
> PTW filter out invalid entries. Of course, micro-arch could give a more
> robust design, but here, use a software fence to guarantee.
>
> Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
> Signed-off-by: Guo Ren <guoren@kernel.org>
> ---
>  arch/riscv/include/asm/pgtable.h | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
> index 294044429e8e..2fae5a5438e0 100644
> --- a/arch/riscv/include/asm/pgtable.h
> +++ b/arch/riscv/include/asm/pgtable.h
> @@ -511,6 +511,13 @@ static inline int pte_same(pte_t pte_a, pte_t pte_b)
>  static inline void set_pte(pte_t *ptep, pte_t pteval)
>  {
>         *ptep = pteval;
> +
> +       /*
> +        * Only if the new pte is present and kernel, otherwise TLB
> +        * maintenance or update_mmu_cache() have the necessary barriers.
> +        */
> +       if (pte_val(pteval) & (_PAGE_PRESENT | _PAGE_GLOBAL))
> +               RISCV_FENCE(rw,rw);
>  }
>
>  void flush_icache_pte(pte_t pte);
> --
> 2.40.1
>


-- 
Best Regards
 Guo Ren

Re: [PATCH] riscv: pgtable: Enhance set_pte to prevent OoO risk

[Alexandre Ghiti]

Hi Guo,

On Fri, Dec 8, 2023 at 4:10 PM <guoren@kernel.org> wrote:
>
> From: Guo Ren <guoren@linux.alibaba.com>
>
> When changing from an invalid pte to a valid one for a kernel page,
> there is no need for tlb_flush. It's okay for the TSO memory model, but
> there is an OoO risk for the Weak one. eg:
>
> sd t0, (a0) // a0 = pte address, pteval is changed from invalid to valid
> ...
> ld t1, (a1) // a1 = va of above pte
>
> If the ld instruction is executed speculatively before the sd
> instruction. Then it would bring an invalid entry into the TLB, and when
> the ld instruction retired, a spurious page fault occurred. Because the
> vmemmap has been ignored by vmalloc_fault, the spurious page fault would
> cause kernel panic.
>
> This patch was inspired by the commit: 7f0b1bf04511 ("arm64: Fix barriers
> used for page table modifications"). For RISC-V, there is no requirement
> in the spec to guarantee all tlb entries are valid and no requirement to
> PTW filter out invalid entries. Of course, micro-arch could give a more
> robust design, but here, use a software fence to guarantee.
>
> Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
> Signed-off-by: Guo Ren <guoren@kernel.org>
> ---
>  arch/riscv/include/asm/pgtable.h | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
> index 294044429e8e..2fae5a5438e0 100644
> --- a/arch/riscv/include/asm/pgtable.h
> +++ b/arch/riscv/include/asm/pgtable.h
> @@ -511,6 +511,13 @@ static inline int pte_same(pte_t pte_a, pte_t pte_b)
>  static inline void set_pte(pte_t *ptep, pte_t pteval)
>  {
>         *ptep = pteval;
> +
> +       /*
> +        * Only if the new pte is present and kernel, otherwise TLB
> +        * maintenance or update_mmu_cache() have the necessary barriers.
> +        */
> +       if (pte_val(pteval) & (_PAGE_PRESENT | _PAGE_GLOBAL))
> +               RISCV_FENCE(rw,rw);

Only a sfence.vma can guarantee that the PTW actually sees a new
mapping, a fence is not enough. That being said, new kernel mappings
(vmalloc ones) are correctly handled in the kernel by using
flush_cache_vmap(). Did you observe something that this patch fixes?

Thanks,

Alex

>  }
>
>  void flush_icache_pte(pte_t pte);
> --
> 2.40.1
>

Re: [PATCH] riscv: pgtable: Enhance set_pte to prevent OoO risk

[Guo Ren]

On Mon, Dec 11, 2023 at 1:52 PM Alexandre Ghiti <alexghiti@rivosinc.com> wrote:
>
> Hi Guo,
>
> On Fri, Dec 8, 2023 at 4:10 PM <guoren@kernel.org> wrote:
> >
> > From: Guo Ren <guoren@linux.alibaba.com>
> >
> > When changing from an invalid pte to a valid one for a kernel page,
> > there is no need for tlb_flush. It's okay for the TSO memory model, but
> > there is an OoO risk for the Weak one. eg:
> >
> > sd t0, (a0) // a0 = pte address, pteval is changed from invalid to valid
> > ...
> > ld t1, (a1) // a1 = va of above pte
> >
> > If the ld instruction is executed speculatively before the sd
> > instruction. Then it would bring an invalid entry into the TLB, and when
> > the ld instruction retired, a spurious page fault occurred. Because the
> > vmemmap has been ignored by vmalloc_fault, the spurious page fault would
> > cause kernel panic.
> >
> > This patch was inspired by the commit: 7f0b1bf04511 ("arm64: Fix barriers
> > used for page table modifications"). For RISC-V, there is no requirement
> > in the spec to guarantee all tlb entries are valid and no requirement to
> > PTW filter out invalid entries. Of course, micro-arch could give a more
> > robust design, but here, use a software fence to guarantee.
> >
> > Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
> > Signed-off-by: Guo Ren <guoren@kernel.org>
> > ---
> >  arch/riscv/include/asm/pgtable.h | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
> > index 294044429e8e..2fae5a5438e0 100644
> > --- a/arch/riscv/include/asm/pgtable.h
> > +++ b/arch/riscv/include/asm/pgtable.h
> > @@ -511,6 +511,13 @@ static inline int pte_same(pte_t pte_a, pte_t pte_b)
> >  static inline void set_pte(pte_t *ptep, pte_t pteval)
> >  {
> >         *ptep = pteval;
> > +
> > +       /*
> > +        * Only if the new pte is present and kernel, otherwise TLB
> > +        * maintenance or update_mmu_cache() have the necessary barriers.
> > +        */
> > +       if (pte_val(pteval) & (_PAGE_PRESENT | _PAGE_GLOBAL))
> > +               RISCV_FENCE(rw,rw);
>
> Only a sfence.vma can guarantee that the PTW actually sees a new
> mapping, a fence is not enough. That being said, new kernel mappings
> (vmalloc ones) are correctly handled in the kernel by using
> flush_cache_vmap(). Did you observe something that this patch fixes?
Thx for the reply!

The sfence.vma is too expensive, so the situation is tricky. See the
arm64 commit: 7f0b1bf04511 ("arm64: Fix barriers used for page table
modifications"), which is similar. That is, linux assumes invalid pte
won't get into TLB. Think about memory hotplug:

mm/sparse.c: sparse_add_section() {
...
        memmap = section_activate(nid, start_pfn, nr_pages, altmap, pgmap);
        if (IS_ERR(memmap))
                return PTR_ERR(memmap);

        /*
         * Poison uninitialized struct pages in order to catch invalid flags
         * combinations.
         */
        page_init_poison(memmap, sizeof(struct page) * nr_pages);
...
}
The section_activate would use set_pte to setup vmemmap, and
page_init_poison would access these pages' struct.

That means:
sd t0, (a0) // a0 = struct page's pte address, pteval is changed from
invalid to valid
 ...
lw/sw t1, (a1) // a1 = va of struct page

If the lw/sw instruction is executed speculatively before the set_pte,
we need a fence to prevent this.

>
> Thanks,
>
> Alex
>
> >  }
> >
> >  void flush_icache_pte(pte_t pte);
> > --
> > 2.40.1
> >



-- 
Best Regards
 Guo Ren

Re: [PATCH] riscv: pgtable: Enhance set_pte to prevent OoO risk

[Alexandre Ghiti]

On Mon, Dec 11, 2023 at 9:41 AM Guo Ren <guoren@kernel.org> wrote:
>
> On Mon, Dec 11, 2023 at 1:52 PM Alexandre Ghiti <alexghiti@rivosinc.com> wrote:
> >
> > Hi Guo,
> >
> > On Fri, Dec 8, 2023 at 4:10 PM <guoren@kernel.org> wrote:
> > >
> > > From: Guo Ren <guoren@linux.alibaba.com>
> > >
> > > When changing from an invalid pte to a valid one for a kernel page,
> > > there is no need for tlb_flush. It's okay for the TSO memory model, but
> > > there is an OoO risk for the Weak one. eg:
> > >
> > > sd t0, (a0) // a0 = pte address, pteval is changed from invalid to valid
> > > ...
> > > ld t1, (a1) // a1 = va of above pte
> > >
> > > If the ld instruction is executed speculatively before the sd
> > > instruction. Then it would bring an invalid entry into the TLB, and when
> > > the ld instruction retired, a spurious page fault occurred. Because the
> > > vmemmap has been ignored by vmalloc_fault, the spurious page fault would
> > > cause kernel panic.
> > >
> > > This patch was inspired by the commit: 7f0b1bf04511 ("arm64: Fix barriers
> > > used for page table modifications"). For RISC-V, there is no requirement
> > > in the spec to guarantee all tlb entries are valid and no requirement to
> > > PTW filter out invalid entries. Of course, micro-arch could give a more
> > > robust design, but here, use a software fence to guarantee.
> > >
> > > Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
> > > Signed-off-by: Guo Ren <guoren@kernel.org>
> > > ---
> > >  arch/riscv/include/asm/pgtable.h | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > >
> > > diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
> > > index 294044429e8e..2fae5a5438e0 100644
> > > --- a/arch/riscv/include/asm/pgtable.h
> > > +++ b/arch/riscv/include/asm/pgtable.h
> > > @@ -511,6 +511,13 @@ static inline int pte_same(pte_t pte_a, pte_t pte_b)
> > >  static inline void set_pte(pte_t *ptep, pte_t pteval)
> > >  {
> > >         *ptep = pteval;
> > > +
> > > +       /*
> > > +        * Only if the new pte is present and kernel, otherwise TLB
> > > +        * maintenance or update_mmu_cache() have the necessary barriers.
> > > +        */
> > > +       if (pte_val(pteval) & (_PAGE_PRESENT | _PAGE_GLOBAL))
> > > +               RISCV_FENCE(rw,rw);
> >
> > Only a sfence.vma can guarantee that the PTW actually sees a new
> > mapping, a fence is not enough. That being said, new kernel mappings
> > (vmalloc ones) are correctly handled in the kernel by using
> > flush_cache_vmap(). Did you observe something that this patch fixes?
> Thx for the reply!
>
> The sfence.vma is too expensive, so the situation is tricky. See the
> arm64 commit: 7f0b1bf04511 ("arm64: Fix barriers used for page table
> modifications"), which is similar. That is, linux assumes invalid pte
> won't get into TLB. Think about memory hotplug:
>
> mm/sparse.c: sparse_add_section() {
> ...
>         memmap = section_activate(nid, start_pfn, nr_pages, altmap, pgmap);
>         if (IS_ERR(memmap))
>                 return PTR_ERR(memmap);
>
>         /*
>          * Poison uninitialized struct pages in order to catch invalid flags
>          * combinations.
>          */
>         page_init_poison(memmap, sizeof(struct page) * nr_pages);
> ...
> }
> The section_activate would use set_pte to setup vmemmap, and
> page_init_poison would access these pages' struct.

So I think the generic code must be fixed by adding a
flush_cache_vmap() in vmemmap_populate_range() or similar: several
architectures implement flush_cache_vmap() because they need to do
"something" after a new mapping is established, so vmemmap should not
be any different.

>
> That means:
> sd t0, (a0) // a0 = struct page's pte address, pteval is changed from
> invalid to valid
>  ...
> lw/sw t1, (a1) // a1 = va of struct page
>
> If the lw/sw instruction is executed speculatively before the set_pte,
> we need a fence to prevent this.

Yes I agree, but to me we need the fence property of sfence.vma to
make sure the PTW sees the new pte, unless I'm mistaken and something
in the privileged specification states that a fence is enough?

>
> >
> > Thanks,
> >
> > Alex
> >
> > >  }
> > >
> > >  void flush_icache_pte(pte_t pte);
> > > --
> > > 2.40.1
> > >
>
>
>
> --
> Best Regards
>  Guo Ren

Re: [PATCH] riscv: pgtable: Enhance set_pte to prevent OoO risk

[Guo Ren]

On Mon, Dec 11, 2023 at 5:04 PM Alexandre Ghiti <alexghiti@rivosinc.com> wrote:
>
> On Mon, Dec 11, 2023 at 9:41 AM Guo Ren <guoren@kernel.org> wrote:
> >
> > On Mon, Dec 11, 2023 at 1:52 PM Alexandre Ghiti <alexghiti@rivosinc.com> wrote:
> > >
> > > Hi Guo,
> > >
> > > On Fri, Dec 8, 2023 at 4:10 PM <guoren@kernel.org> wrote:
> > > >
> > > > From: Guo Ren <guoren@linux.alibaba.com>
> > > >
> > > > When changing from an invalid pte to a valid one for a kernel page,
> > > > there is no need for tlb_flush. It's okay for the TSO memory model, but
> > > > there is an OoO risk for the Weak one. eg:
> > > >
> > > > sd t0, (a0) // a0 = pte address, pteval is changed from invalid to valid
> > > > ...
> > > > ld t1, (a1) // a1 = va of above pte
> > > >
> > > > If the ld instruction is executed speculatively before the sd
> > > > instruction. Then it would bring an invalid entry into the TLB, and when
> > > > the ld instruction retired, a spurious page fault occurred. Because the
> > > > vmemmap has been ignored by vmalloc_fault, the spurious page fault would
> > > > cause kernel panic.
> > > >
> > > > This patch was inspired by the commit: 7f0b1bf04511 ("arm64: Fix barriers
> > > > used for page table modifications"). For RISC-V, there is no requirement
> > > > in the spec to guarantee all tlb entries are valid and no requirement to
> > > > PTW filter out invalid entries. Of course, micro-arch could give a more
> > > > robust design, but here, use a software fence to guarantee.
> > > >
> > > > Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
> > > > Signed-off-by: Guo Ren <guoren@kernel.org>
> > > > ---
> > > >  arch/riscv/include/asm/pgtable.h | 7 +++++++
> > > >  1 file changed, 7 insertions(+)
> > > >
> > > > diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
> > > > index 294044429e8e..2fae5a5438e0 100644
> > > > --- a/arch/riscv/include/asm/pgtable.h
> > > > +++ b/arch/riscv/include/asm/pgtable.h
> > > > @@ -511,6 +511,13 @@ static inline int pte_same(pte_t pte_a, pte_t pte_b)
> > > >  static inline void set_pte(pte_t *ptep, pte_t pteval)
> > > >  {
> > > >         *ptep = pteval;
> > > > +
> > > > +       /*
> > > > +        * Only if the new pte is present and kernel, otherwise TLB
> > > > +        * maintenance or update_mmu_cache() have the necessary barriers.
> > > > +        */
> > > > +       if (pte_val(pteval) & (_PAGE_PRESENT | _PAGE_GLOBAL))
> > > > +               RISCV_FENCE(rw,rw);
> > >
> > > Only a sfence.vma can guarantee that the PTW actually sees a new
> > > mapping, a fence is not enough. That being said, new kernel mappings
> > > (vmalloc ones) are correctly handled in the kernel by using
> > > flush_cache_vmap(). Did you observe something that this patch fixes?
> > Thx for the reply!
> >
> > The sfence.vma is too expensive, so the situation is tricky. See the
> > arm64 commit: 7f0b1bf04511 ("arm64: Fix barriers used for page table
> > modifications"), which is similar. That is, linux assumes invalid pte
> > won't get into TLB. Think about memory hotplug:
> >
> > mm/sparse.c: sparse_add_section() {
> > ...
> >         memmap = section_activate(nid, start_pfn, nr_pages, altmap, pgmap);
> >         if (IS_ERR(memmap))
> >                 return PTR_ERR(memmap);
> >
> >         /*
> >          * Poison uninitialized struct pages in order to catch invalid flags
> >          * combinations.
> >          */
> >         page_init_poison(memmap, sizeof(struct page) * nr_pages);
> > ...
> > }
> > The section_activate would use set_pte to setup vmemmap, and
> > page_init_poison would access these pages' struct.
>
> So I think the generic code must be fixed by adding a
> flush_cache_vmap() in vmemmap_populate_range() or similar: several
> architectures implement flush_cache_vmap() because they need to do
> "something" after a new mapping is established, so vmemmap should not
> be any different.
Perhaps generic code assumes TLB won't contain invalid entries. When
invalid -> valid, Linux won't do any tlb_flush, ref:

* Use set_p*_safe(), and elide TLB flushing, when confident that *no*
 * TLB flush will be required as a result of the "set". For example, use
 * in scenarios where it is known ahead of time that the routine is
 * setting non-present entries, or re-setting an existing entry to the
 * same value. Otherwise, use the typical "set" helpers and flush the
 * TLB.

>
> >
> > That means:
> > sd t0, (a0) // a0 = struct page's pte address, pteval is changed from
> > invalid to valid
> >  ...
> > lw/sw t1, (a1) // a1 = va of struct page
> >
> > If the lw/sw instruction is executed speculatively before the set_pte,
> > we need a fence to prevent this.
>
> Yes I agree, but to me we need the fence property of sfence.vma to
> make sure the PTW sees the new pte, unless I'm mistaken and something
> in the privileged specification states that a fence is enough?
All PTW are triggered by IFU & load/store. For the "set" scenarios, we
just need to prevent the access va before the set_pte. So:
 - Don't worry about IFU, which fetches the code sequentially.
 - Use a fence prevent load/store before set_pte.

Sfence.vma is used for invalidate TLB, not for invalid -> valid.

>
> >
> > >
> > > Thanks,
> > >
> > > Alex
> > >
> > > >  }
> > > >
> > > >  void flush_icache_pte(pte_t pte);
> > > > --
> > > > 2.40.1
> > > >
> >
> >
> >
> > --
> > Best Regards
> >  Guo Ren



-- 
Best Regards
 Guo Ren

Re: [PATCH] riscv: pgtable: Enhance set_pte to prevent OoO risk

[Samuel Holland]

On 2023-12-11 5:36 AM, Guo Ren wrote:
> On Mon, Dec 11, 2023 at 5:04 PM Alexandre Ghiti <alexghiti@rivosinc.com> wrote:
>>
>> On Mon, Dec 11, 2023 at 9:41 AM Guo Ren <guoren@kernel.org> wrote:
>>>
>>> On Mon, Dec 11, 2023 at 1:52 PM Alexandre Ghiti <alexghiti@rivosinc.com> wrote:
>>>>
>>>> Hi Guo,
>>>>
>>>> On Fri, Dec 8, 2023 at 4:10 PM <guoren@kernel.org> wrote:
>>>>>
>>>>> From: Guo Ren <guoren@linux.alibaba.com>
>>>>>
>>>>> When changing from an invalid pte to a valid one for a kernel page,
>>>>> there is no need for tlb_flush. It's okay for the TSO memory model, but
>>>>> there is an OoO risk for the Weak one. eg:
>>>>>
>>>>> sd t0, (a0) // a0 = pte address, pteval is changed from invalid to valid
>>>>> ...
>>>>> ld t1, (a1) // a1 = va of above pte
>>>>>
>>>>> If the ld instruction is executed speculatively before the sd
>>>>> instruction. Then it would bring an invalid entry into the TLB, and when
>>>>> the ld instruction retired, a spurious page fault occurred. Because the
>>>>> vmemmap has been ignored by vmalloc_fault, the spurious page fault would
>>>>> cause kernel panic.
>>>>>
>>>>> This patch was inspired by the commit: 7f0b1bf04511 ("arm64: Fix barriers
>>>>> used for page table modifications"). For RISC-V, there is no requirement
>>>>> in the spec to guarantee all tlb entries are valid and no requirement to
>>>>> PTW filter out invalid entries. Of course, micro-arch could give a more
>>>>> robust design, but here, use a software fence to guarantee.
>>>>>
>>>>> Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
>>>>> Signed-off-by: Guo Ren <guoren@kernel.org>
>>>>> ---
>>>>>  arch/riscv/include/asm/pgtable.h | 7 +++++++
>>>>>  1 file changed, 7 insertions(+)
>>>>>
>>>>> diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
>>>>> index 294044429e8e..2fae5a5438e0 100644
>>>>> --- a/arch/riscv/include/asm/pgtable.h
>>>>> +++ b/arch/riscv/include/asm/pgtable.h
>>>>> @@ -511,6 +511,13 @@ static inline int pte_same(pte_t pte_a, pte_t pte_b)
>>>>>  static inline void set_pte(pte_t *ptep, pte_t pteval)
>>>>>  {
>>>>>         *ptep = pteval;
>>>>> +
>>>>> +       /*
>>>>> +        * Only if the new pte is present and kernel, otherwise TLB
>>>>> +        * maintenance or update_mmu_cache() have the necessary barriers.
>>>>> +        */
>>>>> +       if (pte_val(pteval) & (_PAGE_PRESENT | _PAGE_GLOBAL))
>>>>> +               RISCV_FENCE(rw,rw);
>>>>
>>>> Only a sfence.vma can guarantee that the PTW actually sees a new
>>>> mapping, a fence is not enough. That being said, new kernel mappings
>>>> (vmalloc ones) are correctly handled in the kernel by using
>>>> flush_cache_vmap(). Did you observe something that this patch fixes?
>>> Thx for the reply!
>>>
>>> The sfence.vma is too expensive, so the situation is tricky. See the
>>> arm64 commit: 7f0b1bf04511 ("arm64: Fix barriers used for page table
>>> modifications"), which is similar. That is, linux assumes invalid pte
>>> won't get into TLB. Think about memory hotplug:
>>>
>>> mm/sparse.c: sparse_add_section() {
>>> ...
>>>         memmap = section_activate(nid, start_pfn, nr_pages, altmap, pgmap);
>>>         if (IS_ERR(memmap))
>>>                 return PTR_ERR(memmap);
>>>
>>>         /*
>>>          * Poison uninitialized struct pages in order to catch invalid flags
>>>          * combinations.
>>>          */
>>>         page_init_poison(memmap, sizeof(struct page) * nr_pages);
>>> ...
>>> }
>>> The section_activate would use set_pte to setup vmemmap, and
>>> page_init_poison would access these pages' struct.
>>
>> So I think the generic code must be fixed by adding a
>> flush_cache_vmap() in vmemmap_populate_range() or similar: several
>> architectures implement flush_cache_vmap() because they need to do
>> "something" after a new mapping is established, so vmemmap should not
>> be any different.
> Perhaps generic code assumes TLB won't contain invalid entries. When
> invalid -> valid, Linux won't do any tlb_flush, ref:
> 
> * Use set_p*_safe(), and elide TLB flushing, when confident that *no*
>  * TLB flush will be required as a result of the "set". For example, use
>  * in scenarios where it is known ahead of time that the routine is
>  * setting non-present entries, or re-setting an existing entry to the
>  * same value. Otherwise, use the typical "set" helpers and flush the
>  * TLB.
> 
>>
>>>
>>> That means:
>>> sd t0, (a0) // a0 = struct page's pte address, pteval is changed from
>>> invalid to valid
>>>  ...
>>> lw/sw t1, (a1) // a1 = va of struct page
>>>
>>> If the lw/sw instruction is executed speculatively before the set_pte,
>>> we need a fence to prevent this.
>>
>> Yes I agree, but to me we need the fence property of sfence.vma to
>> make sure the PTW sees the new pte, unless I'm mistaken and something
>> in the privileged specification states that a fence is enough?
> All PTW are triggered by IFU & load/store. For the "set" scenarios, we
> just need to prevent the access va before the set_pte. So:
>  - Don't worry about IFU, which fetches the code sequentially.
>  - Use a fence prevent load/store before set_pte.
> 
> Sfence.vma is used for invalidate TLB, not for invalid -> valid.

I think the problem is that, architecturally, you can't prevent a PTW by
preventing access to the virtual address. The RISC-V privileged spec allows
caching the results of PTWs from speculative execution, and it allows caching
invalid PTEs. So effectively, as soon as satp is written, software must be able
to handle _any_ virtual address being in the TLB.

To avoid the sfence.vma in the invalid->valid case, you need to handle the
possible page fault, like in Alex's series here:

https://lore.kernel.org/linux-riscv/20231207150348.82096-1-alexghiti@rivosinc.com/

Regards,
Samuel

Re: [PATCH] riscv: pgtable: Enhance set_pte to prevent OoO risk

[Guo Ren]

On Mon, Dec 11, 2023 at 11:27 PM Samuel Holland
<samuel.holland@sifive.com> wrote:
>
> On 2023-12-11 5:36 AM, Guo Ren wrote:
> > On Mon, Dec 11, 2023 at 5:04 PM Alexandre Ghiti <alexghiti@rivosinc.com> wrote:
> >>
> >> On Mon, Dec 11, 2023 at 9:41 AM Guo Ren <guoren@kernel.org> wrote:
> >>>
> >>> On Mon, Dec 11, 2023 at 1:52 PM Alexandre Ghiti <alexghiti@rivosinc.com> wrote:
> >>>>
> >>>> Hi Guo,
> >>>>
> >>>> On Fri, Dec 8, 2023 at 4:10 PM <guoren@kernel.org> wrote:
> >>>>>
> >>>>> From: Guo Ren <guoren@linux.alibaba.com>
> >>>>>
> >>>>> When changing from an invalid pte to a valid one for a kernel page,
> >>>>> there is no need for tlb_flush. It's okay for the TSO memory model, but
> >>>>> there is an OoO risk for the Weak one. eg:
> >>>>>
> >>>>> sd t0, (a0) // a0 = pte address, pteval is changed from invalid to valid
> >>>>> ...
> >>>>> ld t1, (a1) // a1 = va of above pte
> >>>>>
> >>>>> If the ld instruction is executed speculatively before the sd
> >>>>> instruction. Then it would bring an invalid entry into the TLB, and when
> >>>>> the ld instruction retired, a spurious page fault occurred. Because the
> >>>>> vmemmap has been ignored by vmalloc_fault, the spurious page fault would
> >>>>> cause kernel panic.
> >>>>>
> >>>>> This patch was inspired by the commit: 7f0b1bf04511 ("arm64: Fix barriers
> >>>>> used for page table modifications"). For RISC-V, there is no requirement
> >>>>> in the spec to guarantee all tlb entries are valid and no requirement to
> >>>>> PTW filter out invalid entries. Of course, micro-arch could give a more
> >>>>> robust design, but here, use a software fence to guarantee.
> >>>>>
> >>>>> Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
> >>>>> Signed-off-by: Guo Ren <guoren@kernel.org>
> >>>>> ---
> >>>>>  arch/riscv/include/asm/pgtable.h | 7 +++++++
> >>>>>  1 file changed, 7 insertions(+)
> >>>>>
> >>>>> diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
> >>>>> index 294044429e8e..2fae5a5438e0 100644
> >>>>> --- a/arch/riscv/include/asm/pgtable.h
> >>>>> +++ b/arch/riscv/include/asm/pgtable.h
> >>>>> @@ -511,6 +511,13 @@ static inline int pte_same(pte_t pte_a, pte_t pte_b)
> >>>>>  static inline void set_pte(pte_t *ptep, pte_t pteval)
> >>>>>  {
> >>>>>         *ptep = pteval;
> >>>>> +
> >>>>> +       /*
> >>>>> +        * Only if the new pte is present and kernel, otherwise TLB
> >>>>> +        * maintenance or update_mmu_cache() have the necessary barriers.
> >>>>> +        */
> >>>>> +       if (pte_val(pteval) & (_PAGE_PRESENT | _PAGE_GLOBAL))
> >>>>> +               RISCV_FENCE(rw,rw);
> >>>>
> >>>> Only a sfence.vma can guarantee that the PTW actually sees a new
> >>>> mapping, a fence is not enough. That being said, new kernel mappings
> >>>> (vmalloc ones) are correctly handled in the kernel by using
> >>>> flush_cache_vmap(). Did you observe something that this patch fixes?
> >>> Thx for the reply!
> >>>
> >>> The sfence.vma is too expensive, so the situation is tricky. See the
> >>> arm64 commit: 7f0b1bf04511 ("arm64: Fix barriers used for page table
> >>> modifications"), which is similar. That is, linux assumes invalid pte
> >>> won't get into TLB. Think about memory hotplug:
> >>>
> >>> mm/sparse.c: sparse_add_section() {
> >>> ...
> >>>         memmap = section_activate(nid, start_pfn, nr_pages, altmap, pgmap);
> >>>         if (IS_ERR(memmap))
> >>>                 return PTR_ERR(memmap);
> >>>
> >>>         /*
> >>>          * Poison uninitialized struct pages in order to catch invalid flags
> >>>          * combinations.
> >>>          */
> >>>         page_init_poison(memmap, sizeof(struct page) * nr_pages);
> >>> ...
> >>> }
> >>> The section_activate would use set_pte to setup vmemmap, and
> >>> page_init_poison would access these pages' struct.
> >>
> >> So I think the generic code must be fixed by adding a
> >> flush_cache_vmap() in vmemmap_populate_range() or similar: several
> >> architectures implement flush_cache_vmap() because they need to do
> >> "something" after a new mapping is established, so vmemmap should not
> >> be any different.
> > Perhaps generic code assumes TLB won't contain invalid entries. When
> > invalid -> valid, Linux won't do any tlb_flush, ref:
> >
> > * Use set_p*_safe(), and elide TLB flushing, when confident that *no*
> >  * TLB flush will be required as a result of the "set". For example, use
> >  * in scenarios where it is known ahead of time that the routine is
> >  * setting non-present entries, or re-setting an existing entry to the
> >  * same value. Otherwise, use the typical "set" helpers and flush the
> >  * TLB.
> >
> >>
> >>>
> >>> That means:
> >>> sd t0, (a0) // a0 = struct page's pte address, pteval is changed from
> >>> invalid to valid
> >>>  ...
> >>> lw/sw t1, (a1) // a1 = va of struct page
> >>>
> >>> If the lw/sw instruction is executed speculatively before the set_pte,
> >>> we need a fence to prevent this.
> >>
> >> Yes I agree, but to me we need the fence property of sfence.vma to
> >> make sure the PTW sees the new pte, unless I'm mistaken and something
> >> in the privileged specification states that a fence is enough?
> > All PTW are triggered by IFU & load/store. For the "set" scenarios, we
> > just need to prevent the access va before the set_pte. So:
> >  - Don't worry about IFU, which fetches the code sequentially.
> >  - Use a fence prevent load/store before set_pte.
> >
> > Sfence.vma is used for invalidate TLB, not for invalid -> valid.
>
> I think the problem is that, architecturally, you can't prevent a PTW by
> preventing access to the virtual address. The RISC-V privileged spec allows
> caching the results of PTWs from speculative execution, and it allows caching
> invalid PTEs. So effectively, as soon as satp is written, software must be able
> to handle _any_ virtual address being in the TLB.
>
> To avoid the sfence.vma in the invalid->valid case, you need to handle the
> possible page fault, like in Alex's series here:
>
> https://lore.kernel.org/linux-riscv/20231207150348.82096-1-alexghiti@rivosinc.com/
Just as this patch series said:

+ * The RISC-V kernel does not eagerly emit a sfence.vma after each
+ * new vmalloc mapping, which may result in exceptions:
+ * - if the uarch caches invalid entries, the new mapping would not be
+ *   observed by the page table walker and an invalidation is needed.
+ * - if the uarch does not cache invalid entries, a reordered access
+ *   could "miss" the new mapping and traps: in that case, we only need
+ *   to retry the access, no sfence.vma is required.

I'm talking about "uarch does not cache invalid entries, a reordered
access could "miss" the new mapping and traps".
Using a fence in set_pte is another solution, and better than
retrying. Of course the premise is that the fence is not expensive for
micro-arch.

 - Arm64 used this way: commit: 7f0b1bf04511 ("arm64: Fix barriers
 - X86 is similar, because it's TSO, so any load + store instructions
between set_pte & next access would give a barrier, eg:
   set_pte va
   load (acquire)
   store (release)
   load/store va


Another topic is about "retry the access", this is about kernel
virtual address space spurious page fault right? And Alex is
preventing that in the riscv linux kernel and it would cause a lot of
side effects.

>
> Regards,
> Samuel
>


-- 
Best Regards
 Guo Ren