Re: [PATCH v1] riscv: Fix lost icache flush requests in set_icache_stale_mask()

[Yury Norov <ynorov@nvidia.com>]

On Tue, Oct 13, 2026 at 01:12:37PM -0400, Nickolai Zeldovich wrote:
> set_icache_stale_mask() marks every other hart's icache as stale for the
> current mm by reading the current hart's bit, storing an all-ones mask and
> then writing the current hart's bit back as it was read:
> 
> 	stale_cpu = cpumask_test_cpu(cpu, mask);
> 	cpumask_setall(mask);
> 	__assign_cpu(cpu, mask, stale_cpu);
> 
> mm->context.icache_stale_mask is shared by all harts running the mm and
> is updated concurrently without a lock: flush_icache_mm() sets every bit
> with cpumask_setall() (a plain store) whenever a thread flushes the
> icache, prepare_sync_core_cmd() does the same for
> membarrier(MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE), and
> flush_icache_deferred() clears the current hart's bit with an atomic
> test-and-clear from switch_mm(). Those are safe with each other because
> each of them either only sets bits or atomically clears its own hart's
> bit, and a bit set one time too many only costs a redundant fence.i.
> set_icache_stale_mask() is the one writer that stores a value it read
> earlier: the current hart's bit as returned by cpumask_test_cpu(). If
> another hart sets that bit between the read and the write-back, the
> write-back erases it. The erased bit is a pending deferred icache flush
> for the current hart.
> 
> Consider a process with two threads on different harts:
> 
>   - Thread T1, on hart C, writes new instructions and calls
>     riscv_flush_icache(start, end, SYS_RISCV_FLUSH_ICACHE_LOCAL).
>     flush_icache_mm(mm, true) flushes hart C and only sets the other
>     harts' stale bits, without IPIs: hart A is to flush its icache the
>     next time it switches to this mm.
> 
>   - Thread T2, on hart A, calls prctl(PR_RISCV_SET_ICACHE_FLUSH_CTX,
>     PR_RISCV_CTX_SW_FENCEI_OFF, PR_RISCV_SCOPE_PER_THREAD) at the same
>     time. set_icache_stale_mask() reads hart A's bit as clear, T1's
>     cpumask_setall() sets it, and __assign_cpu() clears it again.
> 
> Hart A then runs another process, and T1 migrates to hart A. switch_mm()
> finds hart A's bit clear and does not flush, so T1 executes the old
> instructions still in hart A's icache. For a JIT this is a wrong result
> or a crash on a mix of old and new code. The guarantee of
> membarrier(SYNC_CORE), which relies on the same bits, is lost the same
> way. The window is the handful of instructions between the load and the
> store, but a thread that calls the prctl while another thread of the
> process flushes its icache hits it often, see the numbers below.
> 
> Set the other harts' bits one at a time with cpumask_set_cpu(), which
> only ever sets a bit, and leave the current hart's bit alone: it already
> holds the value the old code tried to preserve. Commit 6b9391b581fd
> ("riscv: Include riscv_set_icache_flush_ctx prctl") kept the current
> hart's bit to cope with concurrent callers of this function; that is
> preserved. Commit e5bf9a4b68fa ("riscv: switch set_icache_stale_mask()
> to using non-atomic assign_cpu()") noted that the sequence is not atomic
> as a whole; it does not need to be, it only must never clear a bit.
> 
> Reproduced on a StarFive JH7110 (VisionFive 2, non-coherent icache) with
> a test program that runs the sequence above in a loop: T1 executes the
> code on hart A to fill its icache, moves to hart C, rewrites the code and
> flushes with SYS_RISCV_FLUSH_ICACHE_LOCAL while T2 hammers the prctl on
> hart A, lets another process run on hart A, moves back to hart A and
> executes the code again. A result computed from the old code is a stale
> fetch. Each line is 50000 iterations on a pair of harts (A, C):
> 
>   v7.3-rc7 unpatched:               13177 and 14362 stale
>   v7.3-rc7 + 100 us delay in the
>     read-modify-write window:       49993 and 47675 stale
>   v7.3-rc7 + this patch:                0 and     0 stale
>   v7.3-rc7 + this patch + delay:        0 and     0 stale
> 
> Control runs with T2 idle never read stale code on any kernel, so the
> deferred flush itself works; only its request gets lost.
> 
> Found while formally verifying set_icache_stale_mask() against the
> kernel binary.
> 
> Fixes: 6b9391b581fd ("riscv: Include riscv_set_icache_flush_ctx prctl")
> Assisted-by: LLM
> Signed-off-by: Nickolai Zeldovich <nickolai@csail.mit.edu>
> ---
> 
> Notes:
>     The reproducer used for the numbers in the commit message is below (nolibc,
>     build with "make headers_install" done:
>     clang --target=riscv64-linux-gnu -Os -static -nostdlib -nostdinc
>       -I tools/include/nolibc -I usr/include -include nolibc.h
>       -o icache_stale_mask icache_stale_mask.c
>     and run as "icache_stale_mask <hartA> <hartC> 50000" for the race and with a
>     fifth argument for the control run). It needs hardware with a non-coherent
>     icache; QEMU TCG keeps translated code coherent and never shows the symptom.
>     
>     Build checks: riscv defconfig Image with clang, W=1 on cacheflush.o, and
>     allnoconfig (CONFIG_SMP=n, the function is not compiled), all clean.
>     checkpatch --strict reports only the missing Signed-off-by.
>     
>     /*
>      * icache_stale_mask.c: a thread's deferred icache flush is lost when
>      * prctl(PR_RISCV_SET_ICACHE_FLUSH_CTX, PR_RISCV_CTX_SW_FENCEI_OFF, ...) on
>      * one hart races with riscv_flush_icache(SYS_RISCV_FLUSH_ICACHE_LOCAL) on
>      * another hart of the same process.
>      *
>      * Build with the kernel's nolibc (after "make headers_install"):
>      *   clang --target=riscv64-linux-gnu -Os -static -nostdlib -nostdinc \
>      *     -I tools/include/nolibc -I usr/include -include nolibc.h \
>      *     -o icache_stale_mask icache_stale_mask.c
>      * Usage: icache_stale_mask <hartA> <hartC> [iterations] [control]
>      *
>      * Three actors:
>      *   T1 (the main thread): writes new code at P from hart C and calls
>      *      riscv_flush_icache(P, P + 4096, SYS_RISCV_FLUSH_ICACHE_LOCAL), which
>      *      flushes hart C and marks every other hart's icache stale for this mm.
>      *   T2 (a thread of the same process, pinned to hart A): calls the prctl in
>      *      a loop while T1 flushes.  Its set_icache_stale_mask() reads hart A's
>      *      stale bit (clear), then stores the whole mask back with the bit as
>      *      read, erasing the bit T1 set in between.
>      *   Q  (another process, pinned to hart A): runs on hart A after T2 blocks,
>      *      so that T1's arrival on hart A is a switch_mm() into the process,
>      *      the point where the kernel performs the deferred flush iff the bit
>      *      is set.
>      * Each iteration: T1 runs the old code on hart A (hart A's icache now holds
>      * it), moves to hart C, writes new code, flushes LOCAL while T2 hammers the
>      * prctl, lets Q run on hart A, moves back to hart A and runs the code.  A
>      * result computed from the old code is a stale fetch.  With "control" T2
>      * never runs: every iteration must then read the new code.
>      */
>     #define N 1000	/* li a0,0; N x addi a0,a0,v; ret: returns N*v */
>     #define PAGE 4096
>     
>     #ifndef SYS_RISCV_FLUSH_ICACHE_LOCAL
>     #define SYS_RISCV_FLUSH_ICACHE_LOCAL 1
>     #endif
>     #ifndef __NR_riscv_flush_icache
>     #define __NR_riscv_flush_icache 259
>     #endif
>     #ifndef PR_RISCV_SET_ICACHE_FLUSH_CTX
>     #define PR_RISCV_SET_ICACHE_FLUSH_CTX 71
>     #define PR_RISCV_CTX_SW_FENCEI_ON 0
>     #define PR_RISCV_CTX_SW_FENCEI_OFF 1
>     #define PR_RISCV_SCOPE_PER_PROCESS 0
>     #define PR_RISCV_SCOPE_PER_THREAD 1
>     #endif
>     
>     static unsigned int *rw;	/* the code page, writable mapping */
>     static unsigned int *rx;	/* the same page, executable mapping */
>     static volatile int go;		/* T2 hammers the prctl while set */
>     static int t1_to_t2[2], t2_to_t1[2], t1_to_q[2], q_to_t1[2];
>     static int hart_a, hart_c;
>     static char t2_stack[65536] __attribute__((aligned(16)));
>     
>     /* clone a thread (shared mm) running fn(arg) on the given stack */
>     long __clone_thread(long flags, void *stack_top, int (*fn)(void *), void *arg);
>     asm(".text\n"
>         ".globl __clone_thread\n"
>         "__clone_thread:\n"
>         "	addi	a1, a1, -16\n"
>         "	sd	a2, 0(a1)\n"		/* fn  */
>         "	sd	a3, 8(a1)\n"		/* arg */
>         "	li	a2, 0\n"
>         "	li	a3, 0\n"
>         "	li	a4, 0\n"
>         "	li	a7, 220\n"		/* __NR_clone */
>         "	ecall\n"
>         "	bnez	a0, 1f\n"
>         "	ld	a2, 0(sp)\n"
>         "	ld	a0, 8(sp)\n"
>         "	jalr	a2\n"
>         "	li	a7, 93\n"		/* __NR_exit */
>         "	ecall\n"
>         "1:	ret\n");
>     
>     static void pin(int cpu)
>     {
>     	unsigned long mask = 1UL << cpu;
>     
>     	if (syscall(__NR_sched_setaffinity, 0, sizeof(mask), &mask))
>     		printf("sched_setaffinity(%d) failed\n", cpu);
>     }
>     
>     static void write_text(unsigned int v)
>     {
>     	int i;
>     
>     	rw[0] = 0x00000513;
>     	for (i = 1; i <= N; i++)
>     		rw[i] = 0x00050513 | (v << 20);
>     	rw[N + 1] = 0x00008067;
>     }
>     
>     static long run_text(void)
>     {
>     	return ((long (*)(void))rx)();
>     }
>     
>     static int t2_main(void *arg)
>     {
>     	char c;
>     	long r;
>     
>     	pin(hart_a);
>     	while (read(t1_to_t2[0], &c, 1) == 1) {
>     		write(t2_to_t1[1], "", 1);	/* about to hammer the prctl */
>     		while (go) {
>     			r = syscall(__NR_prctl, PR_RISCV_SET_ICACHE_FLUSH_CTX,
>     				    PR_RISCV_CTX_SW_FENCEI_OFF,
>     				    PR_RISCV_SCOPE_PER_THREAD, 0, 0);
>     			if (r) {
>     				printf("prctl failed: %ld\n", r);
>     				break;
>     			}
>     		}
>     		write(t2_to_t1[1], "", 1);
>     	}
>     	return 0;
>     }
>     
>     int main(int argc, char **argv)
>     {
>     	int iters = argc > 3 ? atoi(argv[3]) : 1000;
>     	int control = argc > 4;
>     	int i, fd, stale = 0, old_only = 0, bad = 0;
>     	long got;
>     	char c;
>     
>     	if (argc < 3) {
>     		printf("usage: %s <hartA> <hartC> [iterations] [control]\n", argv[0]);
>     		return 2;
>     	}
>     	hart_a = atoi(argv[1]);
>     	hart_c = atoi(argv[2]);
>     
>     	fd = memfd_create("text", 0);
>     	ftruncate(fd, PAGE);
>     	rw = mmap(NULL, PAGE, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0);
>     	rx = mmap(NULL, PAGE, PROT_READ | PROT_EXEC, MAP_SHARED, fd, 0);
>     	pipe(t1_to_t2);
>     	pipe(t2_to_t1);
>     	pipe(t1_to_q);
>     	pipe(q_to_t1);
>     
>     	if (!fork()) {			/* Q: another mm, pinned to hart A */
>     		close(t1_to_q[1]);
>     		close(t1_to_t2[1]);
>     		pin(hart_a);
>     		while (read(t1_to_q[0], &c, 1) == 1)
>     			write(q_to_t1[1], "", 1);
>     		_exit(0);
>     	}
>     	__clone_thread(CLONE_VM | CLONE_FS | CLONE_FILES | CLONE_SIGHAND |
>     		       CLONE_THREAD | CLONE_SYSVSEM,
>     		       t2_stack + sizeof(t2_stack), t2_main, NULL);
>     
>     	for (i = 0; i < iters; i++) {
>     		unsigned int old = 1 + (i & 1), new = 2 - (i & 1);
>     
>     		pin(hart_a);
>     		write_text(old);
>     		syscall(__NR_riscv_flush_icache, 0, 0, 0);	/* all harts */
>     		got = run_text();		/* hart A caches the old code */
>     		if (got != (long)N * old) {
>     			bad++;
>     			continue;
>     		}
>     
>     		pin(hart_c);
>     		if (!control) {
>     			go = 1;
>     			write(t1_to_t2[1], "", 1);	/* T2 hammers the prctl on A */
>     			read(t2_to_t1[0], &c, 1);	/* ... from now on */
>     		}
>     		write_text(new);
>     		/* flush hart C now, mark hart A's icache stale for this mm */
>     		syscall(__NR_riscv_flush_icache, rx, (char *)rx + PAGE,
>     			SYS_RISCV_FLUSH_ICACHE_LOCAL);
>     		if (!control) {
>     			go = 0;
>     			read(t2_to_t1[0], &c, 1);	/* T2 stops and blocks */
>     		}
>     		write(t1_to_q[1], "", 1);	/* Q runs on hart A: A leaves our mm */
>     		read(q_to_t1[0], &c, 1);
>     
>     		pin(hart_a);			/* switch_mm into our mm on hart A */
>     		got = run_text();
>     		if (got != (long)N * new) {
>     			stale++;
>     			if (got == (long)N * old)
>     				old_only++;
>     		}
>     	}
>     	close(t1_to_t2[1]);
>     	close(t1_to_q[1]);
>     	wait(NULL);
>     	printf("harts A=%d C=%d %s: %d of %d iterations ran stale code (%d entirely old, %d warm-up failures)\n",
>     	       hart_a, hart_c, control ? "control" : "race", stale, iters,
>     	       old_only, bad);
>     	return !!stale;
>     }
> 
>  arch/riscv/mm/cacheflush.c | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/riscv/mm/cacheflush.c b/arch/riscv/mm/cacheflush.c
> index f8ead7cb7c7d6..22d6520a05417 100644
> --- a/arch/riscv/mm/cacheflush.c
> +++ b/arch/riscv/mm/cacheflush.c
> @@ -180,20 +180,19 @@ void __init riscv_init_cbo_blocksizes(void)
>  static void set_icache_stale_mask(void)
>  {
>  	int cpu = get_cpu();
> +	unsigned int other;
>  	cpumask_t *mask;
> -	bool stale_cpu;
>  
>  	/*
> -	 * Mark every other hart's icache as needing a flush for
> -	 * this MM. Maintain the previous value of the current
> -	 * cpu to handle the case when this function is called
> -	 * concurrently on different harts.
> +	 * Mark every other hart's icache as needing a flush for this MM.
> +	 * Set the bits one at a time: flush_icache_mm() and
> +	 * flush_icache_deferred() update this mask concurrently, and a
> +	 * read-modify-write of the whole mask would discard their updates.
>  	 */
>  	mask = &current->mm->context.icache_stale_mask;
> -	stale_cpu = cpumask_test_cpu(cpu, mask);
> -
> -	cpumask_setall(mask);
> -	__assign_cpu(cpu, mask, stale_cpu);
> +	for_each_possible_cpu(other)
> +		if (other != cpu)
> +			cpumask_set_cpu(other, mask);

Not judging the motivation, but the implementation should avoid the
for-loop:

        
	cpumask_setall(mask);
        __cpumask_clear_cpu(cpu);

Or I misunderstood your intention?

Thanks,
Yury

>  	put_cpu();
>  }
>  #endif
> 
> base-commit: 24a19b953e2a3d0ea5a8c4951a0ab8f32720c907
> -- 
> 2.56.0