Re: [PATCH 1/4] arm64: compat: Alloc separate pages for vectors and sigpage

[Vincenzo Frascino <vincenzo.frascino@arm.com>]

On 01/04/2019 15:27, Catalin Marinas wrote:
> On Mon, Apr 01, 2019 at 12:20:22PM +0100, Vincenzo Frascino wrote:
>> diff --git a/arch/arm64/kernel/vdso.c b/arch/arm64/kernel/vdso.c
>> index 2d419006ad43..9556ad2036ef 100644
>> --- a/arch/arm64/kernel/vdso.c
>> +++ b/arch/arm64/kernel/vdso.c
>> @@ -1,5 +1,7 @@
>>  /*
>> - * VDSO implementation for AArch64 and vector page setup for AArch32.
>> + * VDSO implementation for AArch64 and for AArch32:
>> + * AArch64: vDSO implementation contains pages setup and data page update.
>> + * AArch32: vDSO implementation contains sigreturn and kuser pages setup.
>>   *
>>   * Copyright (C) 2012 ARM Limited
>>   *
>> @@ -53,61 +55,117 @@ struct vdso_data *vdso_data = &vdso_data_store.data;
>>  /*
>>   * Create and map the vectors page for AArch32 tasks.
>>   */
>> -static struct page *vectors_page[1] __ro_after_init;
>> +/*
>> + * aarch32_vdso_pages:
>> + * 0 - kuser helpers
>> + * 1 - sigreturn code
>> + */
>> +static struct page *aarch32_vdso_pages[2] __ro_after_init;
> 
> More of a nitpick, the code may be easier to follow if we had two
> separate variables. Does the array buy us anything?
>

Even though it does not make much difference right now, it simplifies the
implementation of the compat vdso going forward.

But I agree with you, we can always make the code more readable hence I will
introduce some meaningful defines in v2 (instead of 0 and 1 indexes).

>> +static const struct vm_special_mapping aarch32_vdso_spec[2] = {
>> +	{
>> +		/* Must be named [vectors] for compatibility with arm. */
>> +		.name	= "[vectors]",
>> +		.pages	= &aarch32_vdso_pages[0],
>> +	},
>> +	{
>> +		/* Must be named [sigpage] for compatibility with arm. */
>> +		.name	= "[sigpage]",
>> +		.pages	= &aarch32_vdso_pages[1],
>> +	},
>> +};
> [...]
>> -int aarch32_setup_vectors_page(struct linux_binprm *bprm, int uses_interp)
>> +static int aarch32_kuser_helpers_setup(struct mm_struct *mm)
>>  {
>> -	struct mm_struct *mm = current->mm;
>> -	unsigned long addr = AARCH32_VECTORS_BASE;
>> -	static const struct vm_special_mapping spec = {
>> -		.name	= "[vectors]",
>> -		.pages	= vectors_page,
>> +	void *ret;
>> +
>> +	/* The kuser helpers must be mapped at the ABI-defined high address */
>> +	ret = _install_special_mapping(mm, AARCH32_KUSER_BASE, PAGE_SIZE,
>> +				       VM_READ | VM_EXEC |
>> +				       VM_MAYREAD | VM_MAYEXEC,
>> +				       &aarch32_vdso_spec[0]);
>> +
>> +	return PTR_ERR_OR_ZERO(ret);
>> +}
>>  
>> -	};
>> +static int aarch32_sigreturn_setup(struct mm_struct *mm)
>> +{
>> +	unsigned long addr;
>>  	void *ret;
>>  
>> -	if (down_write_killable(&mm->mmap_sem))
>> -		return -EINTR;
>> -	current->mm->context.vdso = (void *)addr;
>> +	addr = get_unmapped_area(NULL, 0, PAGE_SIZE, 0, 0);
>> +	if (IS_ERR_VALUE(addr)) {
>> +		ret = ERR_PTR(addr);
>> +		goto out;
>> +	}
>>  
>> -	/* Map vectors page at the high address. */
>>  	ret = _install_special_mapping(mm, addr, PAGE_SIZE,
>> -				       VM_READ|VM_EXEC|VM_MAYREAD|VM_MAYEXEC,
>> -				       &spec);
>> +				       VM_READ | VM_EXEC | VM_MAYREAD |
>> +				       VM_MAYWRITE | VM_MAYEXEC,
>> +				       &aarch32_vdso_spec[1]);
> 
> Any reason for setting VM_MAYWRITE here?
> 

VM_MAYWRITE is required to allow gdb to Copy-on-Write and set breakpoints.

-- 
Regards,
Vincenzo