missing data cache flush for signal trampoline on fork

missing data cache flush for signal trampoline on fork

[Atsushi Nemoto]

Hi.  The attached test program which is heavily using signal and fork
occasionally killed by SIGSEG, etc.  When it was killed, PC is always
near the stack pointer.

This would happen on CPUs without MIPS_CACHE_IC_F_DC.  D-cache
aliasing is irrelevant.

1. To handle the signal (SIGUSR1), signal-trampoline code are written
to the stack page.

2. They are flushed to memory immediately and I-cache are invalidated.

3. If other thread called fork() before the signal handler is
executed, all writable page (including the stack page) are marked as
COW page.

4. When the user signal handler is to write to the stack, the page
will be copied to new physical page by copy_user_page(), but not
flushed to main memory.  copy_user_page() use kernel virtual address
to copy the data.

5. Then flush_cache_page() is called for the stack page, but it uses
user virtual address and Hit_Invalidate_Writeback_D.  This does not
flush the cache written by copy_user_page().

6. When returned from the user signal handler, the signal trampoline
code might not be written to main memory.  Garbage code will be
executed and the program die.

Here is a test program.

#include <stdio.h>
#include <stdlib.h>
#include <signal.h>
#include <pthread.h>
#include <unistd.h>
#include <sys/types.h>

void sighandler(int sig)
{
	int a;
	*(volatile int *)&a = 0;
}

void *thread_func(void *arg)
{
	pid_t pid = getpid();
	struct sigaction act;
	memset(&act, 0, sizeof(act));
	act.sa_handler = sighandler;
	act.sa_flags = SA_NOMASK | SA_RESTART;
	sigaction(SIGUSR1, &act, 0);
	sig_count = 0;
	while (1)
		kill(pid, SIGUSR1);
}

int
main(int argc, char *argv[])
{
	int i;
	pid_t pid;
	pthread_t tid;
	for (i = 0; i < 4; i++)
		pthread_create(&tid, NULL, thread_func, NULL);
	for (i = 0; i < 1000; i++) {
		pid = fork();
		if (pid == -1) {
			perror("fork");
			exit(1);
		}
		if (pid)
			waitpid(pid, NULL, 0);
		else
			exit(0);
	}
	return 0;
}


If I used indexed-flush for executable page in flush_cache_page(), the
problem disappear.  Is this a right fix?


diff -u linux-mips/arch/mips/mm/c-r4k.c linux/arch/mips/mm/c-r4k.c
--- linux-mips/arch/mips/mm/c-r4k.c	2005-09-22 10:38:23.000000000 +0900
+++ linux/arch/mips/mm/c-r4k.c	2005-09-28 18:50:56.000000000 +0900
@@ -409,15 +409,11 @@
 	 * for every cache flush operation.  So we do indexed flushes
 	 * in that case, which doesn't overly flush the cache too much.
 	 */
-	if ((mm == current->active_mm) && (pte_val(*ptep) & _PAGE_VALID)) {
-		if (cpu_has_dc_aliases || (exec && !cpu_has_ic_fills_f_dc)) {
+	if ((mm == current->active_mm) && (pte_val(*ptep) & _PAGE_VALID) &&
+	    !(exec && !cpu_has_ic_fills_f_dc)) {
+		if (cpu_has_dc_aliases) {
 			r4k_blast_dcache_page(page);
-			if (exec && !cpu_icache_snoops_remote_store)
-				r4k_blast_scache_page(page);
 		}
-		if (exec)
-			r4k_blast_icache_page(page);
-
 		return;
 	}
 
diff -u linux-mips/arch/mips/mm/c-tx39.c linux/arch/mips/mm/c-tx39.c
--- linux-mips/arch/mips/mm/c-tx39.c	2005-09-05 10:16:59.000000000 +0900
+++ linux/arch/mips/mm/c-tx39.c	2005-09-28 18:51:43.000000000 +0900
@@ -213,12 +213,10 @@
 	 * for every cache flush operation.  So we do indexed flushes
 	 * in that case, which doesn't overly flush the cache too much.
 	 */
-	if ((mm == current->active_mm) && (pte_val(*ptep) & _PAGE_VALID)) {
-		if (cpu_has_dc_aliases || exec)
+	if ((mm == current->active_mm) && (pte_val(*ptep) & _PAGE_VALID) &&
+	    !exec) {
+		if (cpu_has_dc_aliases)
 			tx39_blast_dcache_page(page);
-		if (exec)
-			tx39_blast_icache_page(page);
-
 		return;
 	}
 

---
Atsushi Nemoto

Re: missing data cache flush for signal trampoline on fork

[Atsushi Nemoto]

>>>>> On Wed, 28 Sep 2005 20:34:29 +0900 (JST), Atsushi Nemoto <anemo@mba.ocn.ne.jp> said:
anemo> If I used indexed-flush for executable page in
anemo> flush_cache_page(), the problem disappear.  Is this a right
anemo> fix?

Sorry, this would corrupt cpu_has_ic_fills_f_dc case.  Revised.

diff -u linux-mips/arch/mips/mm/c-r4k.c linux/arch/mips/mm/c-r4k.c
--- linux-mips/arch/mips/mm/c-r4k.c	2005-09-22 10:38:23.000000000 +0900
+++ linux/arch/mips/mm/c-r4k.c	2005-09-28 20:55:55.000000000 +0900
@@ -409,8 +409,9 @@
 	 * for every cache flush operation.  So we do indexed flushes
 	 * in that case, which doesn't overly flush the cache too much.
 	 */
-	if ((mm == current->active_mm) && (pte_val(*ptep) & _PAGE_VALID)) {
-		if (cpu_has_dc_aliases || (exec && !cpu_has_ic_fills_f_dc)) {
+	if ((mm == current->active_mm) && (pte_val(*ptep) & _PAGE_VALID) &&
+	    !(exec && !cpu_has_ic_fills_f_dc)) {
+		if (cpu_has_dc_aliases) {
 			r4k_blast_dcache_page(page);
 			if (exec && !cpu_icache_snoops_remote_store)
 				r4k_blast_scache_page(page);
diff -u linux-mips/arch/mips/mm/c-tx39.c linux/arch/mips/mm/c-tx39.c
--- linux-mips/arch/mips/mm/c-tx39.c	2005-09-05 10:16:59.000000000 +0900
+++ linux/arch/mips/mm/c-tx39.c	2005-09-28 18:51:43.000000000 +0900
@@ -213,12 +213,10 @@
 	 * for every cache flush operation.  So we do indexed flushes
 	 * in that case, which doesn't overly flush the cache too much.
 	 */
-	if ((mm == current->active_mm) && (pte_val(*ptep) & _PAGE_VALID)) {
-		if (cpu_has_dc_aliases || exec)
+	if ((mm == current->active_mm) && (pte_val(*ptep) & _PAGE_VALID) &&
+	    !exec) {
+		if (cpu_has_dc_aliases)
 			tx39_blast_dcache_page(page);
-		if (exec)
-			tx39_blast_icache_page(page);
-
 		return;
 	}
 

---
Atsushi Nemoto

Re: missing data cache flush for signal trampoline on fork

[Atsushi Nemoto]

>>>>> On Wed, 28 Sep 2005 20:34:29 +0900 (JST), Atsushi Nemoto <anemo@mba.ocn.ne.jp> said:

anemo> 5. Then flush_cache_page() is called for the stack page, but it
anemo> uses user virtual address and Hit_Invalidate_Writeback_D.  This
anemo> does not flush the cache written by copy_user_page().

This was somewhat wrong.  The flush_cache_page() would flush old data
on the page allocated for the stack.  Anyway this does not flush the
cache written by copy_user_page() because new PTE is not established
yet.

---
Atsushi Nemoto

Re: missing data cache flush for signal trampoline on fork

[Atsushi Nemoto]

>>>>> On Wed, 28 Sep 2005 20:57:58 +0900 (JST), Atsushi Nemoto <anemo@mba.ocn.ne.jp> said:
anemo> Sorry, this would corrupt cpu_has_ic_fills_f_dc case.  Revised.

The patch was overkill.  The indexed-flush is required only for
d-cache.  Revised.

diff -u linux-mips/arch/mips/mm/c-r4k.c linux/arch/mips/mm/c-r4k.c
--- linux-mips/arch/mips/mm/c-r4k.c	2005-09-22 10:38:23.000000000 +0900
+++ linux/arch/mips/mm/c-r4k.c	2005-09-30 12:25:14.000000000 +0900
@@ -410,7 +410,11 @@
 	 * in that case, which doesn't overly flush the cache too much.
 	 */
 	if ((mm == current->active_mm) && (pte_val(*ptep) & _PAGE_VALID)) {
-		if (cpu_has_dc_aliases || (exec && !cpu_has_ic_fills_f_dc)) {
+		if (exec && !cpu_has_ic_fills_f_dc) {
+			r4k_blast_dcache_page_indexed(page);
+			if (!cpu_icache_snoops_remote_store)
+				r4k_blast_scache_page_indexed(page);
+		} else if (cpu_has_dc_aliases) {
 			r4k_blast_dcache_page(page);
 			if (exec && !cpu_icache_snoops_remote_store)
 				r4k_blast_scache_page(page);
diff -u linux-mips/arch/mips/mm/c-tx39.c linux/arch/mips/mm/c-tx39.c
--- linux-mips/arch/mips/mm/c-tx39.c	2005-09-05 10:16:59.000000000 +0900
+++ linux/arch/mips/mm/c-tx39.c	2005-09-30 12:26:23.000000000 +0900
@@ -214,7 +214,9 @@
 	 * in that case, which doesn't overly flush the cache too much.
 	 */
 	if ((mm == current->active_mm) && (pte_val(*ptep) & _PAGE_VALID)) {
-		if (cpu_has_dc_aliases || exec)
+		if (exec)
+			tx39_blast_dcache_page_indexed(page);
+		else if (cpu_has_dc_aliases)
 			tx39_blast_dcache_page(page);
 		if (exec)
 			tx39_blast_icache_page(page);

---
Atsushi Nemoto

Re: missing data cache flush for signal trampoline on fork

[Ralf Baechle]

On Fri, Sep 30, 2005 at 12:32:41PM +0900, Atsushi Nemoto wrote:
> Date:	Fri, 30 Sep 2005 12:32:41 +0900 (JST)
> To:	linux-mips@linux-mips.org
> Cc:	ralf@linux-mips.org
> Subject: Re: missing data cache flush for signal trampoline on fork
> From:	Atsushi Nemoto <anemo@mba.ocn.ne.jp>
> Content-Type: Text/Plain; charset=us-ascii
> 
> >>>>> On Wed, 28 Sep 2005 20:57:58 +0900 (JST), Atsushi Nemoto <anemo@mba.ocn.ne.jp> said:
> anemo> Sorry, this would corrupt cpu_has_ic_fills_f_dc case.  Revised.
> 
> The patch was overkill.  The indexed-flush is required only for
> d-cache.  Revised.

Hmm...  Your patch may be right for the time being but I think this should
the whole flushing biz should actually be handled via update_mmu_cache by
adding something along the lines of:

...
	if (vma->flags & VM_EXEC)
		do_flush_icache_page(addr);
...

What do you think?

  Ralf