On Wed, 2023-03-08 at 16:27 +0100, Michal Suchánek wrote:

Hello,

On Wed, Aug 31, 2022 at 11:13:59PM +1000, Michael Ellerman wrote:

quoted

On Wed, 17 Aug 2022 15:06:39 +1000, Russell Currey wrote:

quoted

Add support for execute-only memory (XOM) for the Radix MMU by
using an
execute-only mapping, as opposed to the RX mapping used by
powerpc's
other MMUs.

The Hash MMU already supports XOM through the execute-only pkey,
which is a separate mechanism shared with x86.  A PROT_EXEC-only
mapping
will map to RX, and then the pkey will be applied on top of it.

[...]

Applied to powerpc/next.

[1/2] powerpc/mm: Support execute-only memory on the Radix MMU
     
https://git.kernel.org/powerpc/c/395cac7752b905318ae454a8b859d4c190485510

This breaks libaio tests (on POWER9 hash PowerVM):
https://pagure.io/libaio/blob/master/f/harness/cases/5.t#_43

cases/5.p
expect   512: (w), res =   512 [Success]
expect   512: (r), res =   512 [Success]
expect   512: (r), res =   512 [Success]
expect   512: (w), res =   512 [Success]
expect   512: (w), res =   512 [Success]
expect   -14: (r), res =   -14 [Bad address]
expect   512: (r), res =   512 [Success]
expect   512: (w), res =   512 [Success]
test cases/5.t completed PASSED.

cases/5.p
expect   512: (w), res =   512 [Success]
expect   512: (r), res =   512 [Success]
expect   512: (r), res =   512 [Success]
expect   512: (w), res =   512 [Success]
expect   512: (w), res =   512 [Success]
expect   -14: (r), res =   -14 [Bad address]
expect   512: (r), res =   512 [Success]
expect   -14: (w), res =   512 [Success] -- FAILED
test cases/5.t completed FAILED.

Can you have a look if that test assumption is OK?

Hi Michal, thanks for the report.

This wasn't an intended behaviour change, so it is a bug.  I have no
idea why we hit the fault in write() but not in io_submit(), though. 
The same issue applies under Radix.

What's happening here is that we're taking a page fault and calling
into access_error() and returning true when we shouldn't.  Previously
we didn't check for read faults and only checked for PROT_NONE.  My
patch checks the vma flags to see if they lack VM_READ after we check
for exec and write, which ignores that VM_WRITE implies read 

This means we're mishandling faults for write-only mappings by assuming
that the lack of VM_READ means we're faulting from read, when that
should only be possible under a PROT_EXEC-only mapping.

I think the correct behaviour is

	if (unlikely(!(vma->vm_flags & (VM_READ | VM_WRITE))))

in access_error().

Will do some more testing and send a patch soon.  I also need to verify
that write implying read is true for all powerpc platforms.

- Russell

Thanks

Michal