On Fri, Jan 22, 2016 at 01:06:14PM +0100, Manuel Lauss wrote:
> Hi James,
>
> On Fri, Jan 22, 2016 at 11:58 AM, James Hogan <james.hogan@xxxxxxxxxx> wrote:
> > It is still necessary to handle icache coherency in flush_cache_range()
> > and copy_to_user_page() when the icache fills from the dcache, even
> > though the dcache does not need to be written back. However when this
> > handling was added in commit 2eaa7ec286db ("[MIPS] Handle I-cache
> > coherency in flush_cache_range()"), it did not do any icache flushing
> > when it fills from dcache.
> >
> > Therefore fix r4k_flush_cache_range() to run
> > local_r4k_flush_cache_range() without taking into account whether icache
> > fills from dcache, so that the icache coherency gets handled. Checks are
> > also added in local_r4k_flush_cache_range() so that the dcache blast
> > doesn't take place when icache fills from dcache.
> >
> > A test to mmap a page PROT_READ|PROT_WRITE, modify code in it, and
> > mprotect it to VM_READ|VM_EXEC (similar to case described in above
> > commit) can hit this case quite easily to verify the fix.
> >
> > A similar check was added in commit f8829caee311 ("[MIPS] Fix aliasing
> > bug in copy_to_user_page / copy_from_user_page"), so also fix
> > copy_to_user_page() similarly, to call flush_cache_page() without taking
> > into account whether icache fills from dcache, since flush_cache_page()
> > already takes that into account to avoid performing a dcache flush.
> >
> > Signed-off-by: James Hogan <james.hogan@xxxxxxxxxx>
> > Cc: Ralf Baechle <ralf@xxxxxxxxxxxxxx>
> > Cc: Leonid Yegoshin <leonid.yegoshin@xxxxxxxxxx>
> > Cc: Manuel Lauss <manuel.lauss@xxxxxxxxx>
> > Cc: linux-mips@xxxxxxxxxxxxxx
> > ---
> > arch/mips/mm/c-r4k.c | 11 +++++++++--
> > arch/mips/mm/init.c | 2 +-
> > 2 files changed, 10 insertions(+), 3 deletions(-)
>
>
> I did some light testing on Alchemy and see no problems so far.
> If it matters: Tested-by: Manuel Lauss <manuel.lauss@xxxxxxxxx>
Thanks Manuel.
FWIW, attached is the test program I mentioned, which hits the first
part of this patch (flush_cache_range) via mprotect(2) and checks if
icache seems to have been flushed (tested on mips64r6, but should be
portable).
Cheers
James
/*
* Copyright (C) 2016 Imagination Technologies Ltd.
* Author: James Hogan <james.hogan@xxxxxxxxxx>
*
* Test that mprotect keeps icache in sync.
* I.e.
* 1) mprotect of non-PROT_EXEC mmap() should sync icache.
* 2) later mprotect RW->RX should sync icache.
* 3) later mprotect RWX->RWX should sync icache.
*
* Linux man pages do not state what mprotect(2) does with caches.
*
* IRIX man pages suggest that its mprotect(2) causes cache flushing to take
* place to allow for execution.
*
* The MIPS behaviour was fixed in Linux kernel commit 2eaa7ec286db ("[MIPS]
* Handle I-cache coherency in flush_cache_range()"), to accomodate the MIPS16
* dynamic linker which would perform data relocations in a page also containing
* code, allocated with mmap VM_READ|VM_WRITE, and then use mprotect(2) to make
* it executable. Without the fix, stale icache lines could be used.
*/
#ifndef __mips__
#error This test only supports MIPS
#endif
#include <inttypes.h>
#include <stdbool.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/cachectl.h>
#include <sys/mman.h>
#include <unistd.h>
#define REG_ZERO 0
#define REG_V0 2
#define REG_RA 31
#define OPC_S 26
#define RS_S 21
#define RT_S 16
#define IMM_S 0
#define FUNC_S 0
#define SPECIAL (000u << OPC_S)
#define ADDIU (011u << OPC_S)
#define JALR (SPECIAL | (011U << FUNC_S))
#define MOV_V0(SIMM) (ADDIU | (REG_V0 << RT_S) | ((uint16_t)(SIMM) << IMM_S))
void usage(char *arg0, FILE *f)
{
fprintf(f, "Usage: %s <options>:\n"
" -h : show this help text\n"
" --[no-]loop1 : run/skip RW/RX mprotect loop (=run)\n"
" --[no-]loop2 : run/skip RWX mprotect loop (=skip)\n"
" --loops <num> : set number of loop iterations (=%u)\n",
arg0, 0x10000);
}
int main(int argc, char **argv)
{
const int pagesize = getpagesize();
uint32_t *buf;
int (*func)(void);
unsigned int i;
int ret;
bool loop1 = true;
bool loop2 = false;
unsigned int max = 0xffff;
for (i = 1; i < argc; ++i) {
if (!strcmp(argv[i], "--loop1")) {
loop1 = true;
} else if (!strcmp(argv[i], "--no-loop1")) {
loop1 = false;
} else if (!strcmp(argv[i], "--loop2")) {
loop2 = true;
} else if (!strcmp(argv[i], "--no-loop2")) {
loop2 = false;
} else if (!strcmp(argv[i], "--loops")) {
++i;
if (i >= argc) {
fprintf(stderr,
"--loops expects an argument\n\n");
goto bad_usage;
}
max = atoi(argv[i]) - 1;
if (max > 0xffff) {
fprintf(stderr,
"--loops must be >= 1 and <= %u\n\n",
0x10000);
goto bad_usage;
}
} else if (!strcmp(argv[i], "-h")) {
usage(argv[0], stdout);
return EXIT_SUCCESS;
} else {
bad_usage:
usage(argv[0], stderr);
return EXIT_FAILURE;
}
}
/* Map a page where we can generate some code. */
buf = mmap(NULL, pagesize, PROT_READ|PROT_WRITE,
MAP_PRIVATE|MAP_ANONYMOUS, 0, 0);
if (buf == MAP_FAILED) {
perror("mmap");
return EXIT_FAILURE;
}
func = (void *)buf;
/*
* Write a simple function returning a value:
* jr ra
* addiu v0, zero, -1
*/
buf[0] = JALR | (REG_RA << RS_S);
buf[1] = MOV_V0(0xffff);
/*
* Flush cache immediately, so we can fail more gracefully if mprotect
* doesn't work.
*/
ret = cacheflush(buf, sizeof(buf[0]) * 2, BCACHE);
if (ret < 0) {
perror("cacheflush");
return EXIT_FAILURE;
}
/* Check mprotect flushes icache. */
/* Make function return 0. */
buf[1] = MOV_V0(0);
/* Make the page executable. */
ret = mprotect(buf, pagesize, PROT_READ|PROT_EXEC);
if (ret < 0) {
perror("initial mprotect\n");
return EXIT_FAILURE;
}
/* Check the return value. */
ret = func();
if (ret != 0) {
fprintf(stderr,
"%s:%u: First mprotect(2) PROT_READ|PROT_EXEC didn't sync icache, ret=%x\n",
__FILE__, __LINE__, ret);
return EXIT_FAILURE;
}
printf("Initial mprotect SUCCESS\n");
if (loop1) {
/* Lets test mprotect(RW), modify, mprotect(RX) a bit more. */
for (i = 0; i <= max; ++i) {
/* Make the page writable, non-executable. */
ret = mprotect(buf, pagesize, PROT_READ|PROT_WRITE);
if (ret < 0) {
perror("mprotect PROT_READ|PROT_WRITE\n");
return EXIT_FAILURE;
}
/* Make the function return (int16_t)i. */
buf[1] = MOV_V0(i);
/* Make the page executable. */
ret = mprotect(buf, pagesize, PROT_READ|PROT_EXEC);
if (ret < 0) {
perror("mprotect PROT_READ|PROT_EXEC\n");
return EXIT_FAILURE;
}
/* Check the return value. */
ret = func();
if (ret != (int16_t)i) {
fprintf(stderr,
"%s:%u: Looped mprotect(2) PROT_READ|PROT_EXEC didn't sync icache, ret=%x, expected %x\n",
__FILE__, __LINE__, ret, (int16_t)i);
return EXIT_FAILURE;
}
}
printf("Looped { mprotect RW, modify, mprotect RX, test } SUCCESS\n");
}
/*
* This loop is disabled by default, because Linux detects that the
* flags haven't changed and does nothing.
*/
if (loop2) {
/* Make the page writable AND executable. */
ret = mprotect(buf, pagesize, PROT_READ|PROT_WRITE|PROT_EXEC);
if (ret < 0) {
perror("initial mprotect PROT_READ|PROT_WRITE|PROT_EXEC\n");
return EXIT_FAILURE;
}
/* Lets test modify while PROT_EXEC. */
for (i = 0; i <= max; ++i) {
/* Make the function return (int16_t)i. */
buf[1] = MOV_V0(i);
/* Remind kernel of executability. */
ret = mprotect(buf, pagesize,
PROT_READ|PROT_WRITE|PROT_EXEC);
if (ret < 0) {
perror("mprotect PROT_READ|PROT_WRITE|PROT_EXEC\n");
return EXIT_FAILURE;
}
/* Check the return value. */
ret = func();
if (ret != (int16_t)i) {
/* This is expected under Linux, see above. */
fprintf(stderr,
"%s:%u: Looped mprotect(2) PROT_READ|PROT_WRITE|PROT_EXEC didn't sync icache, ret=%x, expected %x\n",
__FILE__, __LINE__, ret, (int16_t)i);
return EXIT_FAILURE;
}
}
printf("Looped { modify, mprotect RWX, test } SUCCESS\n");
}
/* Clean up */
ret = munmap(buf, pagesize);
if (ret < 0) {
perror("munmap\n");
return EXIT_FAILURE;
}
return EXIT_SUCCESS;
}
Attachment:
signature.asc
Description: Digital signature
