[PATCH] riscv: mm: Fix concurrency in mark_new_valid_map()

[PATCH] riscv: mm: Fix concurrency in mark_new_valid_map()

[Vivian Wang]

Turns out, the concurrency concerns [1] were justified - BOSC reported a
spurious fault in KFENCE that still triggers despite previous fixes,
which KFENCE reports as a false-positive.

Fix the concurrency problems in mark_new_valid_map():

- Add smp_wmb() before filling the bitmap, to make sure page table
  writes are "received".
- Use WRITE_ONCE() to fill the bitmap.

This fixes the KFENCE false positives in internal testing.

Also update comments in the assembly exception handler code to match.

Fixes: 26c171fc4853 ("riscv: mm: Use the bitmap API for new_valid_map_cpus")
Reported-by: Yaxing Guo <guoyaxing@bosc.ac.cn>
Suggested-by: David Hildenbrand (Arm) <david@kernel.org>
Link: https://lore.kernel.org/linux-riscv/da19ffcf-8042-4f96-9c2d-649468dc6a0a@kernel.org/ # [1]
Signed-off-by: Vivian Wang <wangruikang@iscas.ac.cn>
---
 arch/riscv/include/asm/cacheflush.h | 15 ++++++++++++++-
 arch/riscv/kernel/entry.S           | 11 ++++-------
 2 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/arch/riscv/include/asm/cacheflush.h b/arch/riscv/include/asm/cacheflush.h
index 8cfe59483a8f..1f191cf801a2 100644
--- a/arch/riscv/include/asm/cacheflush.h
+++ b/arch/riscv/include/asm/cacheflush.h
@@ -6,7 +6,9 @@
 #ifndef _ASM_RISCV_CACHEFLUSH_H
 #define _ASM_RISCV_CACHEFLUSH_H
 
+#include <linux/array_size.h>
 #include <linux/mm.h>
+#include <asm/barrier.h>
 
 static inline void local_flush_icache_all(void)
 {
@@ -46,12 +48,23 @@ extern DECLARE_BITMAP(new_valid_map_cpus, NR_CPUS);
 extern char _end[];
 static inline void mark_new_valid_map(void)
 {
+	/*
+	 * Orders any previous page table writes before setting bits in
+	 * new_valid_map_cpus. Pairs with the sfence.vma in
+	 * new_valid_map_cpus_check.
+	 */
+	smp_wmb();
+
 	/*
 	 * We don't care if concurrently a cpu resets this value since
 	 * the only place this can happen is in handle_exception() where
 	 * an sfence.vma is emitted.
+	 *
+	 * Not memset() or bitmap_fill() to avoid any possible compiler
+	 * shenanigans.
 	 */
-	bitmap_fill(new_valid_map_cpus, NR_CPUS);
+	for (size_t i = 0; i < ARRAY_SIZE(new_valid_map_cpus); i++)
+		WRITE_ONCE(new_valid_map_cpus[i], -1UL);
 }
 #define flush_cache_vmap flush_cache_vmap
 static inline void flush_cache_vmap(unsigned long start, unsigned long end)
diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S
index c6988983cdf7..092f408edb7d 100644
--- a/arch/riscv/kernel/entry.S
+++ b/arch/riscv/kernel/entry.S
@@ -76,6 +76,8 @@
 	amoxor.d	a0, a1, (a0)
 
 	/*
+	 * Pairs with the smp_wmb() in mark_new_valid_map()
+	 *
 	 * A sfence.vma is required here. Even if we had Svvptc, there's no
 	 * guarantee that after returning we wouldn't just fault again.
 	 */
@@ -141,13 +143,8 @@ SYM_CODE_START(handle_exception)
 	/*
 	 * The RISC-V kernel does not flush TLBs on all CPUS after each new
 	 * vmalloc mapping or kfence_unprotect(), 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.
+	 * exceptions. In that case, we need to sfence.vma to "receive" the new
+	 * mappings and retry, whether or not we have Svvptc.
 	 */
 	new_valid_map_cpus_check
 #endif

---
base-commit: dc59e4fea9d83f03bad6bddf3fa2e52491777482
change-id: 20260629-riscv-mm-new-valid-map-ordering-007fa3841332

Best regards,
--  
Vivian Wang <wangruikang@iscas.ac.cn>

Re: [PATCH] riscv: mm: Fix concurrency in mark_new_valid_map()

[Paul Walmsley]

Hi Vivian,

On Mon, 29 Jun 2026, Vivian Wang wrote:

> Turns out, the concurrency concerns [1] were justified - BOSC reported a
> spurious fault in KFENCE that still triggers despite previous fixes,
> which KFENCE reports as a false-positive.
> 
> Fix the concurrency problems in mark_new_valid_map():
> 
> - Add smp_wmb() before filling the bitmap, to make sure page table
>   writes are "received".

Probably this expression should be clarified.  I guess the intent here is 
to ensure that the page table write is visible by other cores either 
before or simultaneously with the bitmap fill?  And the purpose of that is 
to avoid a situation where another core could execute an sfence.vma before 
the PTE write is observable by the other core?  If so, we should probably 
write something like this out in the commit message.

I wonder if there's still a window where spurious faults could be observed 
by kfence.  If another core "B" faults (due to its local TLB caching an 
invalid PTE) after the PTE write is observable by B, but before the bitmap 
write is observable by B, core B won't sfence.vma.  If this possibility 
still exists, we should probably document it.  It might be possible to 
narrow that window further with a smp_mb() after the bitmap fill, but 
unclear that it's worth trying unless more spurious faults are seen.
  
...

Also, now that we're adding an smp_wmb(), we should probably expand the 
preemption-disabled section in kfence_protect_page() to cover the entire 
function, to eliminate any risk that the PTE writes execute on a separate 
CPU from the smp_wmb().


- Paul

Re: [PATCH] riscv: mm: Fix concurrency in mark_new_valid_map()

[Paul Walmsley]

Hi Vivian,

On Thu, 9 Jul 2026, Paul Walmsley wrote:

> On Mon, 29 Jun 2026, Vivian Wang wrote:
> 
> > Turns out, the concurrency concerns [1] were justified - BOSC reported a
> > spurious fault in KFENCE that still triggers despite previous fixes,
> > which KFENCE reports as a false-positive.
> > 
> > Fix the concurrency problems in mark_new_valid_map():
> > 
> > - Add smp_wmb() before filling the bitmap, to make sure page table
> >   writes are "received".
> 
> Probably this expression should be clarified.  I guess the intent here is 
> to ensure that the page table write is visible by other cores either 
> before or simultaneously with the bitmap fill?  And the purpose of that is 
> to avoid a situation where another core could execute an sfence.vma before 
> the PTE write is observable by the other core?  If so, we should probably 
> write something like this out in the commit message.
> 
> I wonder if there's still a window where spurious faults could be observed 
> by kfence.  If another core "B" faults (due to its local TLB caching an 
> invalid PTE) after the PTE write is observable by B, but before the bitmap 
> write is observable by B, core B won't sfence.vma.  If this possibility 
> still exists, we should probably document it.  It might be possible to 
> narrow that window further with a smp_mb() after the bitmap fill, but 
> unclear that it's worth trying unless more spurious faults are seen.
>   
> ...
> 
> Also, now that we're adding an smp_wmb(), we should probably expand the 
> preemption-disabled section in kfence_protect_page() to cover the entire 
> function, to eliminate any risk that the PTE writes execute on a separate 
> CPU from the smp_wmb().

Are you planning to update this one?


- Paul

Re: [PATCH] riscv: mm: Fix concurrency in mark_new_valid_map()

[Vivian Wang]

Hi Paul,

On 7/30/26 02:46, Paul Walmsley wrote:
> Hi Vivian,
>
> On Thu, 9 Jul 2026, Paul Walmsley wrote:
>
>> On Mon, 29 Jun 2026, Vivian Wang wrote:
>>
>>> Turns out, the concurrency concerns [1] were justified - BOSC reported a
>>> spurious fault in KFENCE that still triggers despite previous fixes,
>>> which KFENCE reports as a false-positive.
>>>
>>> Fix the concurrency problems in mark_new_valid_map():
>>>
>>> - Add smp_wmb() before filling the bitmap, to make sure page table
>>>   writes are "received".
>> Probably this expression should be clarified.  I guess the intent here is 
>> to ensure that the page table write is visible by other cores either 
>> before or simultaneously with the bitmap fill?  And the purpose of that is 
>> to avoid a situation where another core could execute an sfence.vma before 
>> the PTE write is observable by the other core?  If so, we should probably 
>> write something like this out in the commit message.
>>
>> I wonder if there's still a window where spurious faults could be observed 
>> by kfence.  If another core "B" faults (due to its local TLB caching an 
>> invalid PTE) after the PTE write is observable by B, but before the bitmap 
>> write is observable by B, core B won't sfence.vma.  If this possibility 
>> still exists, we should probably document it.  It might be possible to 
>> narrow that window further with a smp_mb() after the bitmap fill, but 
>> unclear that it's worth trying unless more spurious faults are seen.
>>   
>> ...
>>
>> Also, now that we're adding an smp_wmb(), we should probably expand the 
>> preemption-disabled section in kfence_protect_page() to cover the entire 
>> function, to eliminate any risk that the PTE writes execute on a separate 
>> CPU from the smp_wmb().
> Are you planning to update this one?

Apologies for the delay in response. I think your concerns make sense. I
have been thinking about this and discussing this with BOSC and had not
yet come up with a decisive conclusion. There is also some doubt raised
about what this smp_wmb() is actually fixing. Since this is AFAICT only
possibly known to occur on an internal, RTL-only core, I think it would
be better to come up with a better fix than to slap a random smp_wmb()
in here and calling it a day.

Moreover I will be away until August 10. I recognize that this mean this
patch will miss 7.2, but I think that's better than having, as said, a
random smp_wmb().

Again, apologies,
Vivian "dramforever" Wang

Re: [PATCH] riscv: mm: Fix concurrency in mark_new_valid_map()

[Vivian Wang]

On 7/10/26 08:59, Paul Walmsley wrote:
> Hi Vivian,
>
> On Mon, 29 Jun 2026, Vivian Wang wrote:
>
>> Turns out, the concurrency concerns [1] were justified - BOSC reported a
>> spurious fault in KFENCE that still triggers despite previous fixes,
>> which KFENCE reports as a false-positive.
>>
>> Fix the concurrency problems in mark_new_valid_map():
>>
>> - Add smp_wmb() before filling the bitmap, to make sure page table
>>   writes are "received".
> Probably this expression should be clarified.  I guess the intent here is 
> to ensure that the page table write is visible by other cores either 
> before or simultaneously with the bitmap fill?  And the purpose of that is 
> to avoid a situation where another core could execute an sfence.vma before 
> the PTE write is observable by the other core?  If so, we should probably 
> write something like this out in the commit message.

Apologies for the mega delay. I had been focused on the other stuff.

Yes, This is the intention. I agree that this would probably need a
better wording.

> I wonder if there's still a window where spurious faults could be observed 
> by kfence.  If another core "B" faults (due to its local TLB caching an 
> invalid PTE) after the PTE write is observable by B, but before the bitmap 
> write is observable by B, core B won't sfence.vma.  If this possibility 
> still exists, we should probably document it.  It might be possible to 
> narrow that window further with a smp_mb() after the bitmap fill, but 
> unclear that it's worth trying unless more spurious faults are seen.
>   

Having thought about this more, I'm not sure how this should be handled.
The expected use case of this is some "allocation" function, at which
point the allocated pointer would be passed on to some other thread.

Here's the iffy part, *in general*, I *expect* that before the allocated
pointer or whatever becomes accessible in another thread, there's a
smp_wmb(), or an unlock-then-lock, or something like that. Otherwise,
none of the data being "sent through" this pointer wouldn't make much
sense, right. But this would have to be handled by every allocating
function, or similar, right? And I don't know if I'm able to practically
understand all the mm stuff to see if it is *actually* how it works.

> ...
>
> Also, now that we're adding an smp_wmb(), we should probably expand the 
> preemption-disabled section in kfence_protect_page() to cover the entire 
> function, to eliminate any risk that the PTE writes execute on a separate 
> CPU from the smp_wmb().

I'm not sure that this is a problem if the previous point wasn't. If the
thread is preempted, then migrated to a different CPU, then the thread
migration should have done the equivalent of a smp_mb(), right?
Something like:

        CPU a                    |      CPU b
                                 |
... thread 1 ...                 |
                                 |
[ Preemption starts here         |
                                 |
smp_mb()                         |
                                 |
Some store release in sched      |
to send the task to CPU b        |
                                 |      Some load acquire in sched to
                                 |      grab the task
                                 |
                                 |      smp_mb()
                                 |
                                 |      preemption finishes ]
                                 |
                                 |      ... thread 1 continues ...

Otherwise, even single-threaded program-order behavior wouldn't be
preserved. Therefore I believe it would still be safe even if the thread
was migrated after the smp_wmb().

As such, I do think preemption shouldn't be a problem, or should be
rolled into the other problem, namely, "wmb before sending a pointer to
another thread".

At my wit's end, again,
Vivian "dramforever" Wang