[v7,7/9] mm: Add address parameter to arch_validate_prot()

Message ID 43c120f0cbbebd1398997b9521013ced664e5053.1502219353.git.khalid.aziz@oracle.com
State Changes Requested
Delegated to: David Miller
Headers show

Commit Message

Khalid Aziz Aug. 9, 2017, 9:26 p.m.
A protection flag may not be valid across entire address space and
hence arch_validate_prot() might need the address a protection bit is
being set on to ensure it is a valid protection flag. For example, sparc
processors support memory corruption detection (as part of ADI feature)
flag on memory addresses mapped on to physical RAM but not on PFN mapped
pages or addresses mapped on to devices. This patch adds address to the
parameters being passed to arch_validate_prot() so protection bits can
be validated in the relevant context.

Signed-off-by: Khalid Aziz <khalid.aziz@oracle.com>
Cc: Khalid Aziz <khalid@gonehiking.org>
---
v7:
	- new patch

 arch/powerpc/include/asm/mman.h | 2 +-
 arch/powerpc/kernel/syscalls.c  | 2 +-
 include/linux/mman.h            | 2 +-
 mm/mprotect.c                   | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

Comments

Michael Ellerman Aug. 10, 2017, 1:20 p.m. | #1
Khalid Aziz <khalid.aziz@oracle.com> writes:

> A protection flag may not be valid across entire address space and
> hence arch_validate_prot() might need the address a protection bit is
> being set on to ensure it is a valid protection flag. For example, sparc
> processors support memory corruption detection (as part of ADI feature)
> flag on memory addresses mapped on to physical RAM but not on PFN mapped
> pages or addresses mapped on to devices. This patch adds address to the
> parameters being passed to arch_validate_prot() so protection bits can
> be validated in the relevant context.
>
> Signed-off-by: Khalid Aziz <khalid.aziz@oracle.com>
> Cc: Khalid Aziz <khalid@gonehiking.org>
> ---
> v7:
> 	- new patch
>
>  arch/powerpc/include/asm/mman.h | 2 +-
>  arch/powerpc/kernel/syscalls.c  | 2 +-
>  include/linux/mman.h            | 2 +-
>  mm/mprotect.c                   | 2 +-
>  4 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/mman.h b/arch/powerpc/include/asm/mman.h
> index 30922f699341..bc74074304a2 100644
> --- a/arch/powerpc/include/asm/mman.h
> +++ b/arch/powerpc/include/asm/mman.h
> @@ -40,7 +40,7 @@ static inline bool arch_validate_prot(unsigned long prot)
>  		return false;
>  	return true;
>  }
> -#define arch_validate_prot(prot) arch_validate_prot(prot)
> +#define arch_validate_prot(prot, addr) arch_validate_prot(prot)

This can be simpler, as just:

#define arch_validate_prot arch_validate_prot

cheers
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Khalid Aziz Aug. 10, 2017, 2:41 p.m. | #2
On 08/10/2017 07:20 AM, Michael Ellerman wrote:
> Khalid Aziz <khalid.aziz@oracle.com> writes:
> 
>> A protection flag may not be valid across entire address space and
>> hence arch_validate_prot() might need the address a protection bit is
>> being set on to ensure it is a valid protection flag. For example, sparc
>> processors support memory corruption detection (as part of ADI feature)
>> flag on memory addresses mapped on to physical RAM but not on PFN mapped
>> pages or addresses mapped on to devices. This patch adds address to the
>> parameters being passed to arch_validate_prot() so protection bits can
>> be validated in the relevant context.
>>
>> Signed-off-by: Khalid Aziz <khalid.aziz@oracle.com>
>> Cc: Khalid Aziz <khalid@gonehiking.org>
>> ---
>> v7:
>> 	- new patch
>>
>>   arch/powerpc/include/asm/mman.h | 2 +-
>>   arch/powerpc/kernel/syscalls.c  | 2 +-
>>   include/linux/mman.h            | 2 +-
>>   mm/mprotect.c                   | 2 +-
>>   4 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/mman.h b/arch/powerpc/include/asm/mman.h
>> index 30922f699341..bc74074304a2 100644
>> --- a/arch/powerpc/include/asm/mman.h
>> +++ b/arch/powerpc/include/asm/mman.h
>> @@ -40,7 +40,7 @@ static inline bool arch_validate_prot(unsigned long prot)
>>   		return false;
>>   	return true;
>>   }
>> -#define arch_validate_prot(prot) arch_validate_prot(prot)
>> +#define arch_validate_prot(prot, addr) arch_validate_prot(prot)
> 
> This can be simpler, as just:
> 
> #define arch_validate_prot arch_validate_prot
> 

Hi Michael,

Thanks for reviewing!

My patch expands parameter list for arch_validate_prot() from one to two 
parameters. Existing powerpc version of arch_validate_prot() is written 
with one parameter. If I use the above #define, compilation fails with:

mm/mprotect.c: In function ‘do_mprotect_pkey’:
mm/mprotect.c:399: error: too many arguments to function 
‘arch_validate_prot’

Another way to solve it would be to add the new addr parameter to 
powerpc version of arch_validate_prot() but I chose the less disruptive 
solution of tackling it through #define and expanded the existing 
#define to include the new parameter. Make sense?

Thanks,
Khalid
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Michael Ellerman Aug. 15, 2017, 5:02 a.m. | #3
Khalid Aziz <khalid.aziz@oracle.com> writes:

> On 08/10/2017 07:20 AM, Michael Ellerman wrote:
>> Khalid Aziz <khalid.aziz@oracle.com> writes:
>> 
>>> A protection flag may not be valid across entire address space and
>>> hence arch_validate_prot() might need the address a protection bit is
>>> being set on to ensure it is a valid protection flag. For example, sparc
>>> processors support memory corruption detection (as part of ADI feature)
>>> flag on memory addresses mapped on to physical RAM but not on PFN mapped
>>> pages or addresses mapped on to devices. This patch adds address to the
>>> parameters being passed to arch_validate_prot() so protection bits can
>>> be validated in the relevant context.
>>>
>>> Signed-off-by: Khalid Aziz <khalid.aziz@oracle.com>
>>> Cc: Khalid Aziz <khalid@gonehiking.org>
>>> ---
>>> v7:
>>> 	- new patch
>>>
>>>   arch/powerpc/include/asm/mman.h | 2 +-
>>>   arch/powerpc/kernel/syscalls.c  | 2 +-
>>>   include/linux/mman.h            | 2 +-
>>>   mm/mprotect.c                   | 2 +-
>>>   4 files changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/arch/powerpc/include/asm/mman.h b/arch/powerpc/include/asm/mman.h
>>> index 30922f699341..bc74074304a2 100644
>>> --- a/arch/powerpc/include/asm/mman.h
>>> +++ b/arch/powerpc/include/asm/mman.h
>>> @@ -40,7 +40,7 @@ static inline bool arch_validate_prot(unsigned long prot)
>>>   		return false;
>>>   	return true;
>>>   }
>>> -#define arch_validate_prot(prot) arch_validate_prot(prot)
>>> +#define arch_validate_prot(prot, addr) arch_validate_prot(prot)
>> 
>> This can be simpler, as just:
>> 
>> #define arch_validate_prot arch_validate_prot
>> 
>
> Hi Michael,
>
> Thanks for reviewing!
>
> My patch expands parameter list for arch_validate_prot() from one to two 
> parameters. Existing powerpc version of arch_validate_prot() is written 
> with one parameter. If I use the above #define, compilation fails with:
>
> mm/mprotect.c: In function ‘do_mprotect_pkey’:
> mm/mprotect.c:399: error: too many arguments to function 
> ‘arch_validate_prot’
>
> Another way to solve it would be to add the new addr parameter to 
> powerpc version of arch_validate_prot() but I chose the less disruptive 
> solution of tackling it through #define and expanded the existing 
> #define to include the new parameter. Make sense?

Yes, it makes sense. But it's a bit gross.

At first glance it looks like our arch_validate_prot() has an incorrect
signature.

I'd prefer you just updated it to have the correct signature, I think
you'll have to change one more line in do_mmap2(). So it's not very
intrusive.

cheers
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Khalid Aziz Aug. 15, 2017, 2:32 p.m. | #4
On 08/14/2017 11:02 PM, Michael Ellerman wrote:
> Khalid Aziz <khalid.aziz@oracle.com> writes:
> 
>> On 08/10/2017 07:20 AM, Michael Ellerman wrote:
>>> Khalid Aziz <khalid.aziz@oracle.com> writes:
>>>
>>>> A protection flag may not be valid across entire address space and
>>>> hence arch_validate_prot() might need the address a protection bit is
>>>> being set on to ensure it is a valid protection flag. For example, sparc
>>>> processors support memory corruption detection (as part of ADI feature)
>>>> flag on memory addresses mapped on to physical RAM but not on PFN mapped
>>>> pages or addresses mapped on to devices. This patch adds address to the
>>>> parameters being passed to arch_validate_prot() so protection bits can
>>>> be validated in the relevant context.
>>>>
>>>> Signed-off-by: Khalid Aziz <khalid.aziz@oracle.com>
>>>> Cc: Khalid Aziz <khalid@gonehiking.org>
>>>> ---
>>>> v7:
>>>> 	- new patch
>>>>
>>>>    arch/powerpc/include/asm/mman.h | 2 +-
>>>>    arch/powerpc/kernel/syscalls.c  | 2 +-
>>>>    include/linux/mman.h            | 2 +-
>>>>    mm/mprotect.c                   | 2 +-
>>>>    4 files changed, 4 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/arch/powerpc/include/asm/mman.h b/arch/powerpc/include/asm/mman.h
>>>> index 30922f699341..bc74074304a2 100644
>>>> --- a/arch/powerpc/include/asm/mman.h
>>>> +++ b/arch/powerpc/include/asm/mman.h
>>>> @@ -40,7 +40,7 @@ static inline bool arch_validate_prot(unsigned long prot)
>>>>    		return false;
>>>>    	return true;
>>>>    }
>>>> -#define arch_validate_prot(prot) arch_validate_prot(prot)
>>>> +#define arch_validate_prot(prot, addr) arch_validate_prot(prot)
>>>
>>> This can be simpler, as just:
>>>
>>> #define arch_validate_prot arch_validate_prot
>>>
>>
>> Hi Michael,
>>
>> Thanks for reviewing!
>>
>> My patch expands parameter list for arch_validate_prot() from one to two
>> parameters. Existing powerpc version of arch_validate_prot() is written
>> with one parameter. If I use the above #define, compilation fails with:
>>
>> mm/mprotect.c: In function ‘do_mprotect_pkey’:
>> mm/mprotect.c:399: error: too many arguments to function
>> ‘arch_validate_prot’
>>
>> Another way to solve it would be to add the new addr parameter to
>> powerpc version of arch_validate_prot() but I chose the less disruptive
>> solution of tackling it through #define and expanded the existing
>> #define to include the new parameter. Make sense?
> 
> Yes, it makes sense. But it's a bit gross.
> 
> At first glance it looks like our arch_validate_prot() has an incorrect
> signature.
> 
> I'd prefer you just updated it to have the correct signature, I think
> you'll have to change one more line in do_mmap2(). So it's not very
> intrusive.

Thanks, Michael. I can do that.

--
Khalid

--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/arch/powerpc/include/asm/mman.h b/arch/powerpc/include/asm/mman.h
index 30922f699341..bc74074304a2 100644
--- a/arch/powerpc/include/asm/mman.h
+++ b/arch/powerpc/include/asm/mman.h
@@ -40,7 +40,7 @@  static inline bool arch_validate_prot(unsigned long prot)
 		return false;
 	return true;
 }
-#define arch_validate_prot(prot) arch_validate_prot(prot)
+#define arch_validate_prot(prot, addr) arch_validate_prot(prot)
 
 #endif /* CONFIG_PPC64 */
 #endif	/* _ASM_POWERPC_MMAN_H */
diff --git a/arch/powerpc/kernel/syscalls.c b/arch/powerpc/kernel/syscalls.c
index a877bf8269fe..6d90ddbd2d11 100644
--- a/arch/powerpc/kernel/syscalls.c
+++ b/arch/powerpc/kernel/syscalls.c
@@ -48,7 +48,7 @@  static inline long do_mmap2(unsigned long addr, size_t len,
 {
 	long ret = -EINVAL;
 
-	if (!arch_validate_prot(prot))
+	if (!arch_validate_prot(prot, addr))
 		goto out;
 
 	if (shift) {
diff --git a/include/linux/mman.h b/include/linux/mman.h
index 634c4c51fe3a..1693d95a88ee 100644
--- a/include/linux/mman.h
+++ b/include/linux/mman.h
@@ -49,7 +49,7 @@  static inline void vm_unacct_memory(long pages)
  *
  * Returns true if the prot flags are valid
  */
-static inline bool arch_validate_prot(unsigned long prot)
+static inline bool arch_validate_prot(unsigned long prot, unsigned long addr)
 {
 	return (prot & ~(PROT_READ | PROT_WRITE | PROT_EXEC | PROT_SEM)) == 0;
 }
diff --git a/mm/mprotect.c b/mm/mprotect.c
index 8edd0d576254..beac2dfbb5fa 100644
--- a/mm/mprotect.c
+++ b/mm/mprotect.c
@@ -396,7 +396,7 @@  static int do_mprotect_pkey(unsigned long start, size_t len,
 	end = start + len;
 	if (end <= start)
 		return -ENOMEM;
-	if (!arch_validate_prot(prot))
+	if (!arch_validate_prot(prot, start))
 		return -EINVAL;
 
 	reqprot = prot;