[PATCH] powerpc/mm: Invalidate subpage_prot() system call on radix platforms

[PATCH] powerpc/mm: Invalidate subpage_prot() system call on radix platforms

[Anshuman Khandual]

Radix enabled platforms don't support subpage_prot() system calls. But
at present the system calls goes through without an error and fails
later on while validating expected subpage accesses. Lets not allow
the system call on powerpc radix platforms to begin with to prevent
this confusion in user space.

Signed-off-by: Anshuman Khandual <khandual@linux.vnet.ibm.com>
---
 arch/powerpc/mm/subpage-prot.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/powerpc/mm/subpage-prot.c b/arch/powerpc/mm/subpage-prot.c
index 781532d..4005468 100644
--- a/arch/powerpc/mm/subpage-prot.c
+++ b/arch/powerpc/mm/subpage-prot.c
@@ -195,6 +195,9 @@ long sys_subpage_prot(unsigned long addr, unsigned long len, u32 __user *map)
 	unsigned long next, limit;
 	int err;
 
+	if (radix_enabled())
+		return -ENOSYS;
+
 	/* Check parameters */
 	if ((addr & ~PAGE_MASK) || (len & ~PAGE_MASK) ||
 	    addr >= mm->task_size || len >= mm->task_size ||
-- 
1.8.5.2

Re: [PATCH] powerpc/mm: Invalidate subpage_prot() system call on radix platforms

[Michael Ellerman]

Anshuman Khandual <khandual@linux.vnet.ibm.com> writes:

> Radix enabled platforms don't support subpage_prot() system calls. But
> at present the system calls goes through without an error and fails
> later on while validating expected subpage accesses. Lets not allow
> the system call on powerpc radix platforms to begin with to prevent
> this confusion in user space.
>
> Signed-off-by: Anshuman Khandual <khandual@linux.vnet.ibm.com>
> ---
>  arch/powerpc/mm/subpage-prot.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/arch/powerpc/mm/subpage-prot.c b/arch/powerpc/mm/subpage-prot.c
> index 781532d..4005468 100644
> --- a/arch/powerpc/mm/subpage-prot.c
> +++ b/arch/powerpc/mm/subpage-prot.c
> @@ -195,6 +195,9 @@ long sys_subpage_prot(unsigned long addr, unsigned long len, u32 __user *map)
>  	unsigned long next, limit;
>  	int err;
>  
> +	if (radix_enabled())
> +		return -ENOSYS;

It's considered poor form for syscalls to return ENOSYS. That is meant
to mean one thing only, which is that the syscall does not exist.

See:
  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/asm-generic/errno.h#n10


Now arguably in this case ENOSYS does make some sense, because we'd
really like it if the syscall didn't exist when radix is enabled.

But I still don't like it, it means without changing the kernel version,
the syscall would appear or disappear based on which MMU mode you booted
into - which is weird.

So I think ENOENT would be better. It says "the thing you're trying to
access doesn't exist", where in this case "the thing" is "the subpage
protection map for this address range". It's better than EINVAL because
there's no implication that changing the arguments would result in
different behaviour.

cheers

Re: [PATCH] powerpc/mm: Invalidate subpage_prot() system call on radix platforms

[Aneesh Kumar K.V]

On 12/01/2017 04:01 PM, Michael Ellerman wrote:
> Anshuman Khandual <khandual@linux.vnet.ibm.com> writes:
> 
>> Radix enabled platforms don't support subpage_prot() system calls. But
>> at present the system calls goes through without an error and fails
>> later on while validating expected subpage accesses. Lets not allow
>> the system call on powerpc radix platforms to begin with to prevent
>> this confusion in user space.
>>
>> Signed-off-by: Anshuman Khandual <khandual@linux.vnet.ibm.com>
>> ---
>>   arch/powerpc/mm/subpage-prot.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/arch/powerpc/mm/subpage-prot.c b/arch/powerpc/mm/subpage-prot.c
>> index 781532d..4005468 100644
>> --- a/arch/powerpc/mm/subpage-prot.c
>> +++ b/arch/powerpc/mm/subpage-prot.c
>> @@ -195,6 +195,9 @@ long sys_subpage_prot(unsigned long addr, unsigned long len, u32 __user *map)
>>   	unsigned long next, limit;
>>   	int err;
>>   
>> +	if (radix_enabled())
>> +		return -ENOSYS;
> 
> It's considered poor form for syscalls to return ENOSYS. That is meant
> to mean one thing only, which is that the syscall does not exist.
> 
> See:
>    https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/asm-generic/errno.h#n10
> 
> 
> Now arguably in this case ENOSYS does make some sense, because we'd
> really like it if the syscall didn't exist when radix is enabled.
> 
> But I still don't like it, it means without changing the kernel version,
> the syscall would appear or disappear based on which MMU mode you booted
> into - which is weird.
> 
> So I think ENOENT would be better. It says "the thing you're trying to
> access doesn't exist", where in this case "the thing" is "the subpage
> protection map for this address range". It's better than EINVAL because
> there's no implication that changing the arguments would result in
> different behaviour.
> 

Why not ENOTSUPP?

-aneesh