[PATCH v3 1/4] powerpc/mm: Implement set_memory() routines

[Michael Ellerman]

Russell Currey <ruscur at russell.cc> writes:
> diff --git a/arch/powerpc/mm/pageattr.c b/arch/powerpc/mm/pageattr.c
> new file mode 100644
> index 000000000000..fe3ecbfb8e10
> --- /dev/null
> +++ b/arch/powerpc/mm/pageattr.c
> @@ -0,0 +1,60 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/*
> + * MMU-generic set_memory implementation for powerpc
> + *
> + * Author: Russell Currey <ruscur at russell.cc>

Please don't add email addresses in new files, they just risk
bit-rotting, they're in the git log anyway.

> + *
> + * Copyright 2019, IBM Corporation.
> + */
> +
> +#include <linux/mm.h>
> +#include <linux/set_memory.h>
> +
> +#include <asm/mmu.h>
> +#include <asm/page.h>
> +#include <asm/pgtable.h>
> +
> +static int change_page_attr(pte_t *ptep, unsigned long addr, void *data)
> +{
> +	int action = *((int *)data);
> +	pte_t pte_val;
> +
> +	// invalidate the PTE so it's safe to modify
> +	pte_val = ptep_get_and_clear(&init_mm, addr, ptep);
> +	flush_tlb_kernel_range(addr, addr + PAGE_SIZE);

This doesn't work if for example we're setting the text mapping we're
executing from read-only, which in principle should work.

Or if another CPU is concurrently reading from a mapping we're marking
read-only.

I /think/ that's acceptable for all the current users, but I don't know
that for sure and it's not documented anywhere AFAICS.

At the very least it needs a big comment, and to be mentioned in the
change log.


Also there's no locking here, or in apply_to_page_range() AFAICS.

And because we're doing clear/modify/write, two CPUs that race doing eg.
set_memory_ro() and set_memory_nx() will potentially result in some PTEs
being marked permanently invalid, if one CPU sees the other CPUs clear
of the PTE before the write.

Again I'm not sure any current callers do that, but it's a bit fragile.

I think we can fix the race at least by taking the init_mm
page_table_lock around the clear/modify/write.

> +	// modify the PTE bits as desired, then apply
> +	switch (action) {
> +	case SET_MEMORY_RO:
> +		pte_val = pte_wrprotect(pte_val);
> +		break;
> +	case SET_MEMORY_RW:
> +		pte_val = pte_mkwrite(pte_val);
> +		break;
> +	case SET_MEMORY_NX:
> +		pte_val = pte_exprotect(pte_val);
> +		break;
> +	case SET_MEMORY_X:
> +		pte_val = pte_mkexec(pte_val);
> +		break;
> +	default:
> +		WARN_ON(true);
> +		return -EINVAL;
> +	}
> +
> +	set_pte_at(&init_mm, addr, ptep, pte_val);
> +
> +	return 0;
> +}

cheers