LinuxPPC-Dev Archive on lore.kernel.org
 help / color / Atom feed
From: Michael Ellerman <mpe@ellerman.id.au>
To: Russell Currey <ruscur@russell.cc>, linuxppc-dev@lists.ozlabs.org
Cc: ajd@linux.ibm.com, npiggin@gmail.com, joel@jms.id.au,
	rashmica.g@gmail.com, dja@axtens.net
Subject: Re: [PATCH v3 1/4] powerpc/mm: Implement set_memory() routines
Date: Wed, 23 Oct 2019 13:56:41 +1100
Message-ID: <87pnio5fva.fsf@mpe.ellerman.id.au> (raw)
In-Reply-To: <20191004075050.73327-2-ruscur@russell.cc>

Russell Currey <ruscur@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@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


  reply index

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-04  7:50 [PATCH v3 0/4] Implement STRICT_MODULE_RWX for powerpc Russell Currey
2019-10-04  7:50 ` [PATCH v3 1/4] powerpc/mm: Implement set_memory() routines Russell Currey
2019-10-23  2:56   ` Michael Ellerman [this message]
2019-10-04  7:50 ` [PATCH v3 2/4] powerpc/kprobes: Mark newly allocated probes as RO Russell Currey
2019-10-14  2:22   ` Andrew Donnellan
2019-10-04  7:50 ` [PATCH v3 3/4] powerpc/mm/ptdump: debugfs handler for W+X checks at runtime Russell Currey
2019-10-08  1:59   ` Daniel Axtens
2019-10-08  6:21   ` Christophe Leroy
2019-10-14  2:36     ` Russell Currey
2019-10-04  7:50 ` [PATCH v3 4/4] powerpc: Enable STRICT_MODULE_RWX Russell Currey
2019-10-10 13:13   ` Daniel Axtens

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87pnio5fva.fsf@mpe.ellerman.id.au \
    --to=mpe@ellerman.id.au \
    --cc=ajd@linux.ibm.com \
    --cc=dja@axtens.net \
    --cc=joel@jms.id.au \
    --cc=linuxppc-dev@lists.ozlabs.org \
    --cc=npiggin@gmail.com \
    --cc=rashmica.g@gmail.com \
    --cc=ruscur@russell.cc \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

LinuxPPC-Dev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linuxppc-dev/0 linuxppc-dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linuxppc-dev linuxppc-dev/ https://lore.kernel.org/linuxppc-dev \
		linuxppc-dev@lists.ozlabs.org linuxppc-dev@ozlabs.org
	public-inbox-index linuxppc-dev

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.ozlabs.lists.linuxppc-dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git