Re: [PATCH v10 01/10] powerpc/mm: Implement set_memory() routines

Le 31/03/2021 à 13:16, Michael Ellerman a écrit :

Hi Jordan,

A few nits below ...

Jordan Niethe [off-list ref] writes:

quoted

From: Russell Currey <redacted>

The set_memory_{ro/rw/nx/x}() functions are required for STRICT_MODULE_RWX,
and are generally useful primitives to have.  This implementation is
designed to be completely generic across powerpc's many MMUs.

It's possible that this could be optimised to be faster for specific
MMUs, but the focus is on having a generic and safe implementation for
now.

This implementation does not handle cases where the caller is attempting
to change the mapping of the page it is executing from, or if another
CPU is concurrently using the page being altered.  These cases likely
shouldn't happen, but a more complex implementation with MMU-specific code
could safely handle them, so that is left as a TODO for now.

On hash the linear mapping is not kept in the linux pagetable, so this
will not change the protection if used on that range. Currently these
functions are not used on the linear map so just WARN for now.

These functions do nothing if STRICT_KERNEL_RWX is not enabled.

Reviewed-by: Daniel Axtens <redacted>
Signed-off-by: Russell Currey <redacted>
Signed-off-by: Christophe Leroy <redacted>
[jpn: -rebase on next plus "powerpc/mm/64s: Allow STRICT_KERNEL_RWX again"
       - WARN on hash linear map]
Signed-off-by: Jordan Niethe <redacted>
---
v10: WARN if trying to change the hash linear map
---

This ↓ should have a comment explaining what it's doing:

quoted

+#ifdef CONFIG_PPC_BOOK3S_64
+	if (WARN_ON_ONCE(!radix_enabled() &&
+		     get_region_id(addr) == LINEAR_MAP_REGION_ID)) {
+		return -1;
+	}
+#endif

Maybe:

	if (IS_ENABLED(CONFIG_PPC_BOOK3S_64) &&
	    WARN_ON_ONCE(!radix_enabled() && get_region_id(addr) == LINEAR_MAP_REGION_ID)) {
		return -1;
	}

get_region_id() only exists for book3s/64 at the time being, and LINEAR_MAP_REGION_ID as well.

But then Aneesh pointed out that we should also block VMEMMAP_REGION_ID.

It might be better to just check for the permitted regions.

	if (IS_ENABLED(CONFIG_PPC_BOOK3S_64) && !radix_enabled()) {
         	int region = get_region_id(addr);

	    	if (WARN_ON_ONCE(region != VMALLOC_REGION_ID && region != IO_REGION_ID))
                 	return -1;
	}

quoted

+
+	return apply_to_existing_page_range(&init_mm, start, sz,
+					    change_page_attr, (void *)action);
+}

cheers