riscv: Fixup race condition on PG_dcache_clean in flush_icache_pte

PATCH Applied [email protected] 5 participants 6 messages Jan 27, 2023 – Feb 9, 2023 RISC-V

lore

Share & tools

lore

[email protected]

author

2023-01-27 03:53 UTC #0 lore raw

PATCH v2 1/1 riscv: Fixup race condition on PG_dcache_clean in flush_icache_pte

From: Guo Ren <[email protected]>

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 <[email protected]>

---
Changelog:
V2:
- Optimize commit log
- Rebase on riscv for-next (20230127)

V1:
https://lore.kernel.org/linux-riscv/[email protected]/

---
 arch/riscv/mm/cacheflush.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

arch/riscv/mm/cacheflush.c +3 -1

index 3cc07ed45aeb..fcd6145fbead 100644
@@ -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 */

Signature · 1 line

2.36.1

Andrew Jones

Reviewed-by on v2 1/1

2023-01-27 11:40 UTC #1 lore raw

Quoted 43 lines · On Thu, Jan 26, 2023 at 10:53:06PM -0500, [email protected] wrote:

From: Guo Ren <[email protected]>
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 <[email protected]>
Signed-off-by Guo Ren <[email protected]>
---
Changelog:
V2:
- Optimize commit log
- Rebase on riscv for-next (20230127)
V1:
https://lore.kernel.org/linux-riscv/[email protected]/
---
 arch/riscv/mm/cacheflush.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
arch/riscv/mm/cacheflush.c +3 -1
index 3cc07ed45aeb..fcd6145fbead 100644
@@ -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 */
 
Signature · 1 line
2.36.1

Reviewed-by Andrew Jones <[email protected]>

Thanks,
drew

Conor Dooley

Reviewed-by on v2 1/1

2023-01-27 23:36 UTC #2 lore raw

In reply to [email protected]

Hey Guo Ren,

Quoted 9 lines · On Thu, Jan 26, 2023 at 10:53:06PM -0500, [email protected] wrote:

From: Guo Ren <[email protected]>
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.

Quoted 7 lines

Fixes 08f051eda33b ("RISC-V: Flush I$ when making a dirty page executable")
Signed-off-by Guo Ren <[email protected]>
Signed-off-by Guo Ren <[email protected]>
---
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 <[email protected]>

Thanks,
Conor.

Quoted 27 lines

- Rebase on riscv for-next (20230127)

V1:
https://lore.kernel.org/linux-riscv/[email protected]/

---
 arch/riscv/mm/cacheflush.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

arch/riscv/mm/cacheflush.c +3 -1

index 3cc07ed45aeb..fcd6145fbead 100644
@@ -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 */

Signature · 1 line

2.36.1

Guo Ren

author

2023-01-28 02:47 UTC #3 lore raw

Quoted 21 lines · Conor Dooley

Hey Guo Ren,
Quoted 9 lines · On Thu, Jan 26, 2023 at 10:53:06PM -0500, [email protected] wrote:
From: Guo Ren <[email protected]>
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.
Quoted 7 lines
Fixes 08f051eda33b ("RISC-V: Flush I$ when making a dirty page executable")
Signed-off-by Guo Ren <[email protected]>
Signed-off-by Guo Ren <[email protected]>
---
Changelog:
V2:
- Optimize commit log

Quoted 2 lines

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.

Quoted 2 lines

But that's not really important and the diff itself seems sound, so:
Reviewed-by Conor Dooley <[email protected]>

Thx.

Quoted 31 lines

Thanks,
Conor.

Quoted 27 lines

- Rebase on riscv for-next (20230127)

V1:
https://lore.kernel.org/linux-riscv/[email protected]/

---
 arch/riscv/mm/cacheflush.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

arch/riscv/mm/cacheflush.c +3 -2

index 3cc07ed45aeb..fcd6145fbead 100644
@@ -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

Signature · 2 lines

Best Regards
 Guo Ren

Palmer Dabbelt

Applied

2023-02-09 23:39 UTC #4 lore raw

In reply to [email protected]

Quoted 8 lines · On Thu, 26 Jan 2023 22:53:06 -0500, [email protected] 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,

Signature · 7 lines

Palmer Dabbelt <[email protected]>


_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv

[email protected]

CI Applied

2023-02-09 23:40 UTC #5 lore raw

In reply to [email protected]

This patch was applied to riscv/linux.git (fixes) by Palmer Dabbelt <[email protected]>:

Hello:

This patch was applied to riscv/linux.git (fixes)
by Palmer Dabbelt <[email protected]>:

Quoted 10 lines · On Thu, 26 Jan 2023 22:53:06 -0500 you wrote:

From: Guo Ren <[email protected]>
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!

Signature · 9 lines

Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html



_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv