"Aneesh Kumar K.V" [off-list ref] writes:

[ text/plain ]
Michael Ellerman [off-list ref] writes:

quoted

[ text/plain ]
On Fri, 2016-18-03 at 04:01:21 UTC, Andrew Donnellan wrote:

quoted

When handling page faults, cxl_handle_page_fault() checks whether the page
should be accessible by userspace and have its _PAGE_USER access bit set.
_PAGE_USER should be set if the context's kernel flag isn't set, or if the
page falls outside of kernel memory.

However, the check currently uses the wrong operator, causing it to always
evalute to true. As such, we always set the _PAGE_USER bit, even when it
should be restricted to the kernel.

Fix the check so that the _PAGE_USER bit is set only as intended.

..

quoted

diff --git a/drivers/misc/cxl/fault.c b/drivers/misc/cxl/fault.c
index 9a8650b..a76cb8a 100644
--- a/drivers/misc/cxl/fault.c
+++ b/drivers/misc/cxl/fault.c

@@ -152,7 +152,7 @@ static void cxl_handle_page_fault(struct cxl_context *ctx,
 	access = _PAGE_PRESENT;
 	if (dsisr & CXL_PSL_DSISR_An_S)
 		access |= _PAGE_RW;
-	if ((!ctx->kernel) || ~(dar & (1ULL << 63)))
+	if ((!ctx->kernel) || !(dar & (1ULL << 63)))
 		access |= _PAGE_USER;

I think you can (should) use is_kernel_addr() for the DAR check.

I'm also slightly worried by that logic in the case of a non-kernel context.

ie. if ctx->kernel is false, we get:

	if (true || !is_kernel_addr(dar))
 		access |= _PAGE_USER;

Which means we just add _PAGE_USER for any address. What am I missing here?

I noticed this when doing radix support and have a variant posted at

https://lists.ozlabs.org/pipermail/linuxppc-dev/2016-March/141036.html

My change also keep it similar to __hash_page.

	/*
	 * We need to set the _PAGE_USER bit if MSR_PR is set or if we are
	 * accessing a userspace segment (even from the kernel). We assume
	 * kernel addresses always have the high bit set.
	 */
	if ((msr & MSR_PR) || (REGION_ID(ea) == USER_REGION_ID))

If we need this change to go in this merge window i can respin my patch.

-aneesh