Re: [PATCH 1/2] MIPS: c-r4k: Sync icache when it fills from dcache

[James Hogan <james.hogan@imgtec.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2280 bytes --]

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@imgtec.com> 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@imgtec.com>
> > Cc: Ralf Baechle <ralf@linux-mips.org>
> > Cc: Leonid Yegoshin <leonid.yegoshin@imgtec.com>
> > Cc: Manuel Lauss <manuel.lauss@gmail.com>
> > Cc: linux-mips@linux-mips.org
> > ---
> >  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@gmail.com>

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

[-- Attachment #1.2: mprotect.c --]
[-- Type: text/x-c, Size: 6033 bytes --]

/*
 * Copyright (C) 2016 Imagination Technologies Ltd.
 * Author: James Hogan <james.hogan@imgtec.com>
 *
 * 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 #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]