diff mbox series

[V2] riscv: Fixup race condition on PG_dcache_clean in flush_icache_pte

Message ID 20230127035306.1819561-1-guoren@kernel.org (mailing list archive)
State Accepted
Commit 950b879b7f0251317d26bae0687e72592d607532
Delegated to: Palmer Dabbelt
Headers show
Series [V2] riscv: Fixup race condition on PG_dcache_clean in flush_icache_pte | expand

Checks

Context Check Description
conchuod/cover_letter success Single patches do not need cover letters
conchuod/tree_selection success Guessed tree name to be fixes
conchuod/fixes_present success Fixes tag present in non-next series
conchuod/maintainers_pattern success MAINTAINERS pattern errors before the patch: 13 and now 13
conchuod/verify_signedoff success Signed-off-by tag matches author and committer
conchuod/kdoc success Errors and warnings before: 0 this patch: 0
conchuod/module_param success Was 0 now: 0
conchuod/build_rv64_gcc_allmodconfig success Errors and warnings before: 4 this patch: 4
conchuod/alphanumeric_selects success Out of order selects before the patch: 57 and now 57
conchuod/build_rv32_defconfig success Build OK
conchuod/dtb_warn_rv64 success Errors and warnings before: 2 this patch: 2
conchuod/header_inline success No static functions without inline keyword in header files
conchuod/checkpatch success total: 0 errors, 0 warnings, 0 checks, 11 lines checked
conchuod/source_inline success Was 0 now: 0
conchuod/build_rv64_nommu_k210_defconfig success Build OK
conchuod/verify_fixes success Fixes tag looks correct
conchuod/build_rv64_nommu_virt_defconfig success Build OK

Commit Message

Guo Ren Jan. 27, 2023, 3:53 a.m. UTC 
From: Guo Ren <guoren@linux.alibaba.com>

In commit 588a513d3425 ("arm64: Fix race condition on PG_dcache_clean
in __sync_icache_dcache()"), we found RISC-V has the same issue as the
previous arm64. The previous implementation didn't guarantee the correct
sequence of operations, which means flush_icache_all() hasn't been
called when the PG_dcache_clean was set. That would cause a risk of page
synchronization.

Fixes: 08f051eda33b ("RISC-V: Flush I$ when making a dirty page executable")
Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
Signed-off-by: Guo Ren <guoren@kernel.org>
---
Changelog:
V2:
 - Optimize commit log
 - Rebase on riscv for-next (20230127)

V1:
https://lore.kernel.org/linux-riscv/20221023133205.3493564-2-guoren@kernel.org/
---
 arch/riscv/mm/cacheflush.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Andrew Jones Jan. 27, 2023, 11:34 a.m. UTC | #1 
On Thu, Jan 26, 2023 at 10:53:06PM -0500, guoren@kernel.org wrote:
> From: Guo Ren <guoren@linux.alibaba.com>
> 
> In commit 588a513d3425 ("arm64: Fix race condition on PG_dcache_clean
> in __sync_icache_dcache()"), we found RISC-V has the same issue as the
> previous arm64. The previous implementation didn't guarantee the correct
> sequence of operations, which means flush_icache_all() hasn't been
> called when the PG_dcache_clean was set. That would cause a risk of page
> synchronization.
> 
> Fixes: 08f051eda33b ("RISC-V: Flush I$ when making a dirty page executable")
> Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
> Signed-off-by: Guo Ren <guoren@kernel.org>
> ---
> Changelog:
> V2:
>  - Optimize commit log
>  - Rebase on riscv for-next (20230127)
> 
> V1:
> https://lore.kernel.org/linux-riscv/20221023133205.3493564-2-guoren@kernel.org/
> ---
>  arch/riscv/mm/cacheflush.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/riscv/mm/cacheflush.c b/arch/riscv/mm/cacheflush.c
> index 3cc07ed45aeb..fcd6145fbead 100644
> --- a/arch/riscv/mm/cacheflush.c
> +++ b/arch/riscv/mm/cacheflush.c
> @@ -90,8 +90,10 @@ void flush_icache_pte(pte_t pte)
>  	if (PageHuge(page))
>  		page = compound_head(page);
>  
> -	if (!test_and_set_bit(PG_dcache_clean, &page->flags))
> +	if (!test_bit(PG_dcache_clean, &page->flags)) {
>  		flush_icache_all();
> +		set_bit(PG_dcache_clean, &page->flags);
> +	}
>  }
>  #endif /* CONFIG_MMU */
>  
> -- 
> 2.36.1
>

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

Thanks,
drew
Conor Dooley Jan. 27, 2023, 11:36 p.m. UTC | #2 
Hey Guo Ren,

On Thu, Jan 26, 2023 at 10:53:06PM -0500, guoren@kernel.org wrote:
> From: Guo Ren <guoren@linux.alibaba.com>
> 
> In commit 588a513d3425 ("arm64: Fix race condition on PG_dcache_clean
> in __sync_icache_dcache()"), we found RISC-V has the same issue as the
> previous arm64. The previous implementation didn't guarantee the correct
> sequence of operations, which means flush_icache_all() hasn't been
> called when the PG_dcache_clean was set. That would cause a risk of page
> synchronization.
> 
> Fixes: 08f051eda33b ("RISC-V: Flush I$ when making a dirty page executable")
> Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
> Signed-off-by: Guo Ren <guoren@kernel.org>
> ---
> Changelog:
> V2:
>  - Optimize commit log

Probably would have benefited from providing the analysis that the arm64
commit did, for riscv, rather than referring to theirs.
But that's not really important and the diff itself seems sound, so:
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

Thanks,
Conor.

>  - Rebase on riscv for-next (20230127)
> 
> V1:
> https://lore.kernel.org/linux-riscv/20221023133205.3493564-2-guoren@kernel.org/
> ---
>  arch/riscv/mm/cacheflush.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/riscv/mm/cacheflush.c b/arch/riscv/mm/cacheflush.c
> index 3cc07ed45aeb..fcd6145fbead 100644
> --- a/arch/riscv/mm/cacheflush.c
> +++ b/arch/riscv/mm/cacheflush.c
> @@ -90,8 +90,10 @@ void flush_icache_pte(pte_t pte)
>  	if (PageHuge(page))
>  		page = compound_head(page);
>  
> -	if (!test_and_set_bit(PG_dcache_clean, &page->flags))
> +	if (!test_bit(PG_dcache_clean, &page->flags)) {
>  		flush_icache_all();
> +		set_bit(PG_dcache_clean, &page->flags);
> +	}
>  }
>  #endif /* CONFIG_MMU */
>  
> -- 
> 2.36.1
>
Guo Ren Jan. 28, 2023, 2:47 a.m. UTC | #3 
On Sat, Jan 28, 2023 at 7:36 AM Conor Dooley <conor@kernel.org> wrote:
>
> Hey Guo Ren,
>
> On Thu, Jan 26, 2023 at 10:53:06PM -0500, guoren@kernel.org wrote:
> > From: Guo Ren <guoren@linux.alibaba.com>
> >
> > In commit 588a513d3425 ("arm64: Fix race condition on PG_dcache_clean
> > in __sync_icache_dcache()"), we found RISC-V has the same issue as the
> > previous arm64. The previous implementation didn't guarantee the correct
> > sequence of operations, which means flush_icache_all() hasn't been
> > called when the PG_dcache_clean was set. That would cause a risk of page
> > synchronization.
> >
> > Fixes: 08f051eda33b ("RISC-V: Flush I$ when making a dirty page executable")
> > Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
> > Signed-off-by: Guo Ren <guoren@kernel.org>
> > ---
> > Changelog:
> > V2:
> >  - Optimize commit log
>
> Probably would have benefited from providing the analysis that the arm64
> commit did, for riscv, rather than referring to theirs.
Maybe you are right, but they are almost the same for this patch. So I
didn't want to duplicate the commit log but gave out the original
instead.

> But that's not really important and the diff itself seems sound, so:
> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
Thx.

>
> Thanks,
> Conor.
>
> >  - Rebase on riscv for-next (20230127)
> >
> > V1:
> > https://lore.kernel.org/linux-riscv/20221023133205.3493564-2-guoren@kernel.org/
> > ---
> >  arch/riscv/mm/cacheflush.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/riscv/mm/cacheflush.c b/arch/riscv/mm/cacheflush.c
> > index 3cc07ed45aeb..fcd6145fbead 100644
> > --- a/arch/riscv/mm/cacheflush.c
> > +++ b/arch/riscv/mm/cacheflush.c
> > @@ -90,8 +90,10 @@ void flush_icache_pte(pte_t pte)
> >       if (PageHuge(page))
> >               page = compound_head(page);
> >
> > -     if (!test_and_set_bit(PG_dcache_clean, &page->flags))
> > +     if (!test_bit(PG_dcache_clean, &page->flags)) {
> >               flush_icache_all();
> > +             set_bit(PG_dcache_clean, &page->flags);
> > +     }
> >  }
> >  #endif /* CONFIG_MMU */
> >
> > --
> > 2.36.1
> >
Palmer Dabbelt Feb. 9, 2023, 11:37 p.m. UTC | #4 
On Thu, 26 Jan 2023 22:53:06 -0500, guoren@kernel.org wrote:
> In commit 588a513d3425 ("arm64: Fix race condition on PG_dcache_clean
> in __sync_icache_dcache()"), we found RISC-V has the same issue as the
> previous arm64. The previous implementation didn't guarantee the correct
> sequence of operations, which means flush_icache_all() hasn't been
> called when the PG_dcache_clean was set. That would cause a risk of page
> synchronization.
> 
> [...]

Applied, thanks!

[1/1] riscv: Fixup race condition on PG_dcache_clean in flush_icache_pte
      https://git.kernel.org/palmer/c/950b879b7f02

Best regards,
patchwork-bot+linux-riscv@kernel.org Feb. 9, 2023, 11:40 p.m. UTC | #5 
Hello:

This patch was applied to riscv/linux.git (fixes)
by Palmer Dabbelt <palmer@rivosinc.com>:

On Thu, 26 Jan 2023 22:53:06 -0500 you wrote:
> From: Guo Ren <guoren@linux.alibaba.com>
> 
> In commit 588a513d3425 ("arm64: Fix race condition on PG_dcache_clean
> in __sync_icache_dcache()"), we found RISC-V has the same issue as the
> previous arm64. The previous implementation didn't guarantee the correct
> sequence of operations, which means flush_icache_all() hasn't been
> called when the PG_dcache_clean was set. That would cause a risk of page
> synchronization.
> 
> [...]

Here is the summary with links:
  - [V2] riscv: Fixup race condition on PG_dcache_clean in flush_icache_pte
    https://git.kernel.org/riscv/c/950b879b7f02

You are awesome, thank you!
diff mbox series

Patch

diff --git a/arch/riscv/mm/cacheflush.c b/arch/riscv/mm/cacheflush.c
index 3cc07ed45aeb..fcd6145fbead 100644
--- a/arch/riscv/mm/cacheflush.c
+++ b/arch/riscv/mm/cacheflush.c
@@ -90,8 +90,10 @@  void flush_icache_pte(pte_t pte)
 	if (PageHuge(page))
 		page = compound_head(page);
 
-	if (!test_and_set_bit(PG_dcache_clean, &page->flags))
+	if (!test_bit(PG_dcache_clean, &page->flags)) {
 		flush_icache_all();
+		set_bit(PG_dcache_clean, &page->flags);
+	}
 }
 #endif /* CONFIG_MMU */