[PATCH v2] x86/vdso: Change return type to vm_fault_t for fault handlers

[PATCH v2] x86/vdso: Change return type to vm_fault_t for fault handlers

[Souptick Joarder]

Use new return type vm_fault_t for both fault handler
vdso_fault() and vvar_fault(). Previously vm_insert_pfn()
returns err which has to mapped into VM_FAULT_* type.
The new function vmf_insert_pfn() will replace this
inefficiency by returning correct VM_FAULT_* type.

See the following commit:

  1c8f422059ae ("mm: change return type to vm_fault_t")
  b3ec9f33acb8 ("mm: change return type to vm_fault_t")

Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
---

v2: Updated the change log

 arch/x86/entry/vdso/vma.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/arch/x86/entry/vdso/vma.c b/arch/x86/entry/vdso/vma.c
index 5b8b556..3679885 100644
--- a/arch/x86/entry/vdso/vma.c
+++ b/arch/x86/entry/vdso/vma.c
@@ -39,7 +39,7 @@ void __init init_vdso_image(const struct vdso_image *image)
 
 struct linux_binprm;
 
-static int vdso_fault(const struct vm_special_mapping *sm,
+static vm_fault_t vdso_fault(const struct vm_special_mapping *sm,
 		      struct vm_area_struct *vma, struct vm_fault *vmf)
 {
 	const struct vdso_image *image = vma->vm_mm->context.vdso_image;
@@ -84,15 +84,15 @@ static int vdso_mremap(const struct vm_special_mapping *sm,
 	return 0;
 }
 
-static int vvar_fault(const struct vm_special_mapping *sm,
+static vm_fault_t vvar_fault(const struct vm_special_mapping *sm,
 		      struct vm_area_struct *vma, struct vm_fault *vmf)
 {
 	const struct vdso_image *image = vma->vm_mm->context.vdso_image;
 	long sym_offset;
-	int ret = -EFAULT;
+	vm_fault_t ret = VM_FAULT_SIGBUS;
 
 	if (!image)
-		return VM_FAULT_SIGBUS;
+		return ret;
 
 	sym_offset = (long)(vmf->pgoff << PAGE_SHIFT) +
 		image->sym_vvar_start;
@@ -105,10 +105,10 @@ static int vvar_fault(const struct vm_special_mapping *sm,
 	 * the page past the end of the vvar mapping.
 	 */
 	if (sym_offset == 0)
-		return VM_FAULT_SIGBUS;
+		return ret;
 
 	if (sym_offset == image->sym_vvar_page) {
-		ret = vm_insert_pfn(vma, vmf->address,
+		ret = vmf_insert_pfn(vma, vmf->address,
 				    __pa_symbol(&__vvar_page) >> PAGE_SHIFT);
 	} else if (sym_offset == image->sym_pvclock_page) {
 		struct pvclock_vsyscall_time_info *pvti =
@@ -124,14 +124,11 @@ static int vvar_fault(const struct vm_special_mapping *sm,
 		struct ms_hyperv_tsc_page *tsc_pg = hv_get_tsc_page();
 
 		if (tsc_pg && vclock_was_used(VCLOCK_HVCLOCK))
-			ret = vm_insert_pfn(vma, vmf->address,
+			ret = vmf_insert_pfn(vma, vmf->address,
 					    vmalloc_to_pfn(tsc_pg));
 	}
 
-	if (ret == 0 || ret == -EBUSY)
-		return VM_FAULT_NOPAGE;
-
-	return VM_FAULT_SIGBUS;
+	return ret;
 }
 
 static const struct vm_special_mapping vdso_mapping = {
-- 
1.9.1

Re: [PATCH v2] x86/vdso: Change return type to vm_fault_t for fault handlers

[Matthew Wilcox]

On Mon, Jun 25, 2018 at 11:27:37PM +0530, Souptick Joarder wrote:
> Use new return type vm_fault_t for both fault handler
> vdso_fault() and vvar_fault(). Previously vm_insert_pfn()
> returns err which has to mapped into VM_FAULT_* type.
> The new function vmf_insert_pfn() will replace this
> inefficiency by returning correct VM_FAULT_* type.

> @@ -105,10 +105,10 @@ static int vvar_fault(const struct vm_special_mapping *sm,
>  	 * the page past the end of the vvar mapping.
>  	 */
>  	if (sym_offset == 0)
> -		return VM_FAULT_SIGBUS;
> +		return ret;
>  
>  	if (sym_offset == image->sym_vvar_page) {
> -		ret = vm_insert_pfn(vma, vmf->address,
> +		ret = vmf_insert_pfn(vma, vmf->address,
>  				    __pa_symbol(&__vvar_page) >> PAGE_SHIFT);
>  	} else if (sym_offset == image->sym_pvclock_page) {
>  		struct pvclock_vsyscall_time_info *pvti =
> @@ -124,14 +124,11 @@ static int vvar_fault(const struct vm_special_mapping *sm,

Haven't you missed converting vm_insert_pfn_prot() at line 117?
Did you test-compile this?

Re: [PATCH v2] x86/vdso: Change return type to vm_fault_t for fault handlers

[Souptick Joarder]

On Tue, Jul 3, 2018 at 4:47 PM, Matthew Wilcox <willy@infradead.org> wrote:
> On Mon, Jun 25, 2018 at 11:27:37PM +0530, Souptick Joarder wrote:
>> Use new return type vm_fault_t for both fault handler
>> vdso_fault() and vvar_fault(). Previously vm_insert_pfn()
>> returns err which has to mapped into VM_FAULT_* type.
>> The new function vmf_insert_pfn() will replace this
>> inefficiency by returning correct VM_FAULT_* type.
>
>> @@ -105,10 +105,10 @@ static int vvar_fault(const struct vm_special_mapping *sm,
>>        * the page past the end of the vvar mapping.
>>        */
>>       if (sym_offset == 0)
>> -             return VM_FAULT_SIGBUS;
>> +             return ret;
>>
>>       if (sym_offset == image->sym_vvar_page) {
>> -             ret = vm_insert_pfn(vma, vmf->address,
>> +             ret = vmf_insert_pfn(vma, vmf->address,
>>                                   __pa_symbol(&__vvar_page) >> PAGE_SHIFT);
>>       } else if (sym_offset == image->sym_pvclock_page) {
>>               struct pvclock_vsyscall_time_info *pvti =
>> @@ -124,14 +124,11 @@ static int vvar_fault(const struct vm_special_mapping *sm,
>
> Haven't you missed converting vm_insert_pfn_prot() at line 117?
> Did you test-compile this?
>

I left it intentionally in this patch. When we will be replacing
vm_insert_foo() with new API vmf_insert_foo(), vm_insert_pfn_prot
need to be changed to return vm_fault_t type.

I will change the return type of vm_insert_pfn_prot() in that patch.

Re: [PATCH v2] x86/vdso: Change return type to vm_fault_t for fault handlers

[Matthew Wilcox]

On Tue, Jul 03, 2018 at 05:18:30PM +0530, Souptick Joarder wrote:
> On Tue, Jul 3, 2018 at 4:47 PM, Matthew Wilcox <willy@infradead.org> wrote:
> > On Mon, Jun 25, 2018 at 11:27:37PM +0530, Souptick Joarder wrote:
> >> Use new return type vm_fault_t for both fault handler
> >> vdso_fault() and vvar_fault(). Previously vm_insert_pfn()
> >> returns err which has to mapped into VM_FAULT_* type.
> >> The new function vmf_insert_pfn() will replace this
> >> inefficiency by returning correct VM_FAULT_* type.
> >
> >> @@ -105,10 +105,10 @@ static int vvar_fault(const struct vm_special_mapping *sm,
> >>        * the page past the end of the vvar mapping.
> >>        */
> >>       if (sym_offset == 0)
> >> -             return VM_FAULT_SIGBUS;
> >> +             return ret;
> >>
> >>       if (sym_offset == image->sym_vvar_page) {
> >> -             ret = vm_insert_pfn(vma, vmf->address,
> >> +             ret = vmf_insert_pfn(vma, vmf->address,
> >>                                   __pa_symbol(&__vvar_page) >> PAGE_SHIFT);
> >>       } else if (sym_offset == image->sym_pvclock_page) {
> >>               struct pvclock_vsyscall_time_info *pvti =
> >> @@ -124,14 +124,11 @@ static int vvar_fault(const struct vm_special_mapping *sm,
> >
> > Haven't you missed converting vm_insert_pfn_prot() at line 117?
> > Did you test-compile this?
> >
> 
> I left it intentionally in this patch. When we will be replacing
> vm_insert_foo() with new API vmf_insert_foo(), vm_insert_pfn_prot
> need to be changed to return vm_fault_t type.
> 
> I will change the return type of vm_insert_pfn_prot() in that patch.

What?!  That's just broken.  vm_insert_pfn_prot returns an errno, so
this patch introduces a bug where sometimes 'ret' contains a vm_fault_t
and sometimes contains an errno.  That's exactly the kind of thing this
patch series is supposed to be *preventing*, not introducing!

I'll send a replacement patch series to do this properly.

Re: [PATCH v2] x86/vdso: Change return type to vm_fault_t for fault handlers

[Souptick Joarder]

On Tue, Jul 3, 2018 at 5:41 PM, Matthew Wilcox <willy@infradead.org> wrote:
> On Tue, Jul 03, 2018 at 05:18:30PM +0530, Souptick Joarder wrote:
>> On Tue, Jul 3, 2018 at 4:47 PM, Matthew Wilcox <willy@infradead.org> wrote:
>> > On Mon, Jun 25, 2018 at 11:27:37PM +0530, Souptick Joarder wrote:
>> >> Use new return type vm_fault_t for both fault handler
>> >> vdso_fault() and vvar_fault(). Previously vm_insert_pfn()
>> >> returns err which has to mapped into VM_FAULT_* type.
>> >> The new function vmf_insert_pfn() will replace this
>> >> inefficiency by returning correct VM_FAULT_* type.
>> >
>> >> @@ -105,10 +105,10 @@ static int vvar_fault(const struct vm_special_mapping *sm,
>> >>        * the page past the end of the vvar mapping.
>> >>        */
>> >>       if (sym_offset == 0)
>> >> -             return VM_FAULT_SIGBUS;
>> >> +             return ret;
>> >>
>> >>       if (sym_offset == image->sym_vvar_page) {
>> >> -             ret = vm_insert_pfn(vma, vmf->address,
>> >> +             ret = vmf_insert_pfn(vma, vmf->address,
>> >>                                   __pa_symbol(&__vvar_page) >> PAGE_SHIFT);
>> >>       } else if (sym_offset == image->sym_pvclock_page) {
>> >>               struct pvclock_vsyscall_time_info *pvti =
>> >> @@ -124,14 +124,11 @@ static int vvar_fault(const struct vm_special_mapping *sm,
>> >
>> > Haven't you missed converting vm_insert_pfn_prot() at line 117?
>> > Did you test-compile this?
>> >
>>
>> I left it intentionally in this patch. When we will be replacing
>> vm_insert_foo() with new API vmf_insert_foo(), vm_insert_pfn_prot
>> need to be changed to return vm_fault_t type.
>>
>> I will change the return type of vm_insert_pfn_prot() in that patch.
>
> What?!  That's just broken.  vm_insert_pfn_prot returns an errno, so
> this patch introduces a bug where sometimes 'ret' contains a vm_fault_t
> and sometimes contains an errno.  That's exactly the kind of thing this
> patch series is supposed to be *preventing*, not introducing!
>

It was my wrong understanding which create this issue. Sorry about it.
I though to change the return type of vm_insert_pfn_prot() to vm_fault_t
type directly along with the caller of vm_insert_pfn_prot(), but in a separate
patch.

But introducing vmf_insert_pfn_prot() is much simpler approach here.

Sorry for making things complex.