[PATCH] kernel/kcov: Replace vm_insert_page with vmf_insert_page

[PATCH] kernel/kcov: Replace vm_insert_page with vmf_insert_page

[Souptick Joarder]

There is a plan to replace vm_insert_page with new API
vmf_insert_page. As part of it, converting vm_insert_page
to use vmf_insert_page.

Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
---
 kernel/kcov.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/kernel/kcov.c b/kernel/kcov.c
index 3ebd09e..8900d8e 100644
--- a/kernel/kcov.c
+++ b/kernel/kcov.c
@@ -293,8 +293,9 @@ static int kcov_mmap(struct file *filep, struct vm_area_struct *vma)
 		spin_unlock(&kcov->lock);
 		for (off = 0; off < size; off += PAGE_SIZE) {
 			page = vmalloc_to_page(kcov->area + off);
-			if (vm_insert_page(vma, vma->vm_start + off, page))
-				WARN_ONCE(1, "vm_insert_page() failed");
+			if (vmf_insert_page(vma, vma->vm_start + off, page)
+					!= VM_FAULT_NOPAGE)
+				WARN_ONCE(1, "vmf_insert_page() failed");
 		}
 		return 0;
 	}
-- 
1.9.1

Re: [PATCH] kernel/kcov: Replace vm_insert_page with vmf_insert_page

[Andrey Ryabinin]

On 09/20/2018 10:12 PM, Souptick Joarder wrote:
> There is a plan to replace vm_insert_page with new API
> vmf_insert_page. As part of it, converting vm_insert_page
> to use vmf_insert_page.
> 
> Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
> ---
>  kernel/kcov.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/kcov.c b/kernel/kcov.c
> index 3ebd09e..8900d8e 100644
> --- a/kernel/kcov.c
> +++ b/kernel/kcov.c
> @@ -293,8 +293,9 @@ static int kcov_mmap(struct file *filep, struct vm_area_struct *vma)
>  		spin_unlock(&kcov->lock);
>  		for (off = 0; off < size; off += PAGE_SIZE) {
>  			page = vmalloc_to_page(kcov->area + off);
> -			if (vm_insert_page(vma, vma->vm_start + off, page))
> -				WARN_ONCE(1, "vm_insert_page() failed");
> +			if (vmf_insert_page(vma, vma->vm_start + off, page)
> +					!= VM_FAULT_NOPAGE)
> +				WARN_ONCE(1, "vmf_insert_page() failed");

Nack, don't see the reason for such change, it only makes code worse.

Re: [PATCH] kernel/kcov: Replace vm_insert_page with vmf_insert_page

[Souptick Joarder]

On Fri, Sep 21, 2018 at 3:06 PM Andrey Ryabinin <aryabinin@virtuozzo.com> wrote:
>
> On 09/20/2018 10:12 PM, Souptick Joarder wrote:
> > There is a plan to replace vm_insert_page with new API
> > vmf_insert_page. As part of it, converting vm_insert_page
> > to use vmf_insert_page.
> >
> > Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
> > ---
> >  kernel/kcov.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/kernel/kcov.c b/kernel/kcov.c
> > index 3ebd09e..8900d8e 100644
> > --- a/kernel/kcov.c
> > +++ b/kernel/kcov.c
> > @@ -293,8 +293,9 @@ static int kcov_mmap(struct file *filep, struct vm_area_struct *vma)
> >               spin_unlock(&kcov->lock);
> >               for (off = 0; off < size; off += PAGE_SIZE) {
> >                       page = vmalloc_to_page(kcov->area + off);
> > -                     if (vm_insert_page(vma, vma->vm_start + off, page))
> > -                             WARN_ONCE(1, "vm_insert_page() failed");
> > +                     if (vmf_insert_page(vma, vma->vm_start + off, page)
> > +                                     != VM_FAULT_NOPAGE)
> > +                             WARN_ONCE(1, "vmf_insert_page() failed");
>
> Nack, don't see the reason for such change, it only makes code worse.

Yes, it needed. Going forward vm_insert_page will be converted to
vmf_insert_page. As part of it, this code has to be converted to use
vmf_insert_page().
please refer below commit on linus tree -
1c8f422059ae5da07db74

Re: [PATCH] kernel/kcov: Replace vm_insert_page with vmf_insert_page

[Andrey Ryabinin]

On 09/21/2018 01:03 PM, Souptick Joarder wrote:
> On Fri, Sep 21, 2018 at 3:06 PM Andrey Ryabinin <aryabinin@virtuozzo.com> wrote:
>>
>> On 09/20/2018 10:12 PM, Souptick Joarder wrote:
>>> There is a plan to replace vm_insert_page with new API
>>> vmf_insert_page. As part of it, converting vm_insert_page
>>> to use vmf_insert_page.
>>>
>>> Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
>>> ---
>>>  kernel/kcov.c | 5 +++--
>>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/kernel/kcov.c b/kernel/kcov.c
>>> index 3ebd09e..8900d8e 100644
>>> --- a/kernel/kcov.c
>>> +++ b/kernel/kcov.c
>>> @@ -293,8 +293,9 @@ static int kcov_mmap(struct file *filep, struct vm_area_struct *vma)
>>>               spin_unlock(&kcov->lock);
>>>               for (off = 0; off < size; off += PAGE_SIZE) {
>>>                       page = vmalloc_to_page(kcov->area + off);
>>> -                     if (vm_insert_page(vma, vma->vm_start + off, page))
>>> -                             WARN_ONCE(1, "vm_insert_page() failed");
>>> +                     if (vmf_insert_page(vma, vma->vm_start + off, page)
>>> +                                     != VM_FAULT_NOPAGE)
>>> +                             WARN_ONCE(1, "vmf_insert_page() failed");
>>
>> Nack, don't see the reason for such change, it only makes code worse.
> 
> Yes, it needed.

No, it's not needed. vm_insert_page() works perfectly fine here.

> Going forward vm_insert_page will be converted to
> vmf_insert_page. As part of it, this code has to be converted to use
> vmf_insert_page().

This doesn't explain why such conversion would make sense for kcov_mmap().

> please refer below commit on linus tree -
> 1c8f422059ae5da07db74

That the commit has nothing to deal with kcov, it doesn't explain why kcov_mmap()
can't keep using vm_insert_page() and/or why we should prefer vmf_insert_page()
over vm_insert_page() particularly in the case of kcov_mmap().
 

Re: [PATCH] kernel/kcov: Replace vm_insert_page with vmf_insert_page

[Souptick Joarder]

On Fri, Sep 21, 2018 at 5:22 PM Andrey Ryabinin <aryabinin@virtuozzo.com> wrote:
>
>
>
> On 09/21/2018 01:03 PM, Souptick Joarder wrote:
> > On Fri, Sep 21, 2018 at 3:06 PM Andrey Ryabinin <aryabinin@virtuozzo.com> wrote:
> >>
> >> On 09/20/2018 10:12 PM, Souptick Joarder wrote:
> >>> There is a plan to replace vm_insert_page with new API
> >>> vmf_insert_page. As part of it, converting vm_insert_page
> >>> to use vmf_insert_page.
> >>>
> >>> Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
> >>> ---
> >>>  kernel/kcov.c | 5 +++--
> >>>  1 file changed, 3 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/kernel/kcov.c b/kernel/kcov.c
> >>> index 3ebd09e..8900d8e 100644
> >>> --- a/kernel/kcov.c
> >>> +++ b/kernel/kcov.c
> >>> @@ -293,8 +293,9 @@ static int kcov_mmap(struct file *filep, struct vm_area_struct *vma)
> >>>               spin_unlock(&kcov->lock);
> >>>               for (off = 0; off < size; off += PAGE_SIZE) {
> >>>                       page = vmalloc_to_page(kcov->area + off);
> >>> -                     if (vm_insert_page(vma, vma->vm_start + off, page))
> >>> -                             WARN_ONCE(1, "vm_insert_page() failed");
> >>> +                     if (vmf_insert_page(vma, vma->vm_start + off, page)
> >>> +                                     != VM_FAULT_NOPAGE)
> >>> +                             WARN_ONCE(1, "vmf_insert_page() failed");
> >>
> >> Nack, don't see the reason for such change, it only makes code worse.
> >
> > Yes, it needed.
>
> No, it's not needed. vm_insert_page() works perfectly fine here.
>
> > Going forward vm_insert_page will be converted to
> > vmf_insert_page. As part of it, this code has to be converted to use
> > vmf_insert_page().
>
> This doesn't explain why such conversion would make sense for kcov_mmap().

vm_insert_page used to return errno which individual drivers have to
map to VM_FAULT_CODE.
There were also places where return value of vm_insert_page was
ignored and return a
TRUE value.

As part of vm_fault_t migration patches, we have identified and clean
up all these.
The plan was to introduce inline wrapper vmf_insert_page(), convert all
the page fault handlers to use it and then convert vm_insert_page to
vmf_insert_page. By now we have converted all the page fault handlers
to use this API.

Still their are few places where vm_insert_page is called outside fault handlers
context to map kernel page to user vma and kernel/kcov.c is one of them.

Now removing vm_insert_page from those few places one by one.

Going forward vm_insert_page will be removed from kernel permanently,
so that new drivers can't use this API and create new errno to VM_FAULT_CODE
mapping code.

>
> > please refer below commit on linus tree -
> > 1c8f422059ae5da07db74
>
> That the commit has nothing to deal with kcov, it doesn't explain why kcov_mmap()
> can't keep using vm_insert_page() and/or why we should prefer vmf_insert_page()
> over vm_insert_page() particularly in the case of kcov_mmap().
>

Re: [PATCH] kernel/kcov: Replace vm_insert_page with vmf_insert_page

[Andrey Ryabinin]

On 09/21/2018 04:25 PM, Souptick Joarder wrote:
> On Fri, Sep 21, 2018 at 5:22 PM Andrey Ryabinin <aryabinin@virtuozzo.com> wrote:
>> On 09/21/2018 01:03 PM, Souptick Joarder wrote:
>>> On Fri, Sep 21, 2018 at 3:06 PM Andrey Ryabinin <aryabinin@virtuozzo.com> wrote:
>>>>
>>>> On 09/20/2018 10:12 PM, Souptick Joarder wrote:
>>>>> @@ -293,8 +293,9 @@ static int kcov_mmap(struct file *filep, struct vm_area_struct *vma)
>>>>>               spin_unlock(&kcov->lock);
>>>>>               for (off = 0; off < size; off += PAGE_SIZE) {
>>>>>                       page = vmalloc_to_page(kcov->area + off);
>>>>> -                     if (vm_insert_page(vma, vma->vm_start + off, page))
>>>>> -                             WARN_ONCE(1, "vm_insert_page() failed");
>>>>> +                     if (vmf_insert_page(vma, vma->vm_start + off, page)
>>>>> +                                     != VM_FAULT_NOPAGE)
>>>>> +                             WARN_ONCE(1, "vmf_insert_page() failed");
>>>>
>>>> Nack, don't see the reason for such change, it only makes code worse.
>>>
>>> Yes, it needed.
>>
>> No, it's not needed. vm_insert_page() works perfectly fine here.
>>
>>> Going forward vm_insert_page will be converted to
>>> vmf_insert_page. As part of it, this code has to be converted to use
>>> vmf_insert_page().
>>
>> This doesn't explain why such conversion would make sense for kcov_mmap().
> 
> vm_insert_page used to return errno which individual drivers have to
> map to VM_FAULT_CODE.
> There were also places where return value of vm_insert_page was
> ignored and return a
> TRUE value.
> 
> As part of vm_fault_t migration patches, we have identified and clean
> up all these.
> The plan was to introduce inline wrapper vmf_insert_page(), convert all
> the page fault handlers to use it and then convert vm_insert_page to
> vmf_insert_page. By now we have converted all the page fault handlers
> to use this API.
> 
> Still their are few places where vm_insert_page is called outside fault handlers
> context to map kernel page to user vma and kernel/kcov.c is one of them.
> 
> Now removing vm_insert_page from those few places one by one.
> 
> Going forward vm_insert_page will be removed from kernel permanently,
> so that new drivers can't use this API and create new errno to VM_FAULT_CODE
> mapping code.
> 

So, to avoid addition of new code that converts errno->VM_FAULT_* you propose to add
new code that does the opposite - converts VM_FAULT_* to errno? How is that better?

And what function is going to be next? pte_none()? It is also used in fault handlers.
Are there going to be vmf_pte_none() and complete removal of pte_none() "so that new drivers
can't use pte_none() and create new pte_none() to VM_FAULT_CODE mapping code"?

Don't you see that how that doesn't make any sense?
Want vmf_insert_page() for easier error handling in fault handlers? - fine, use it in fault handlers.
But leave the vm_insert_page() for the code that has nothing to do with fault handling.

Re: [PATCH] kernel/kcov: Replace vm_insert_page with vmf_insert_page

[Souptick Joarder]

On Fri, Sep 21, 2018 at 8:14 PM Andrey Ryabinin <aryabinin@virtuozzo.com> wrote:
>
>
>
> On 09/21/2018 04:25 PM, Souptick Joarder wrote:
> > On Fri, Sep 21, 2018 at 5:22 PM Andrey Ryabinin <aryabinin@virtuozzo.com> wrote:
> >> On 09/21/2018 01:03 PM, Souptick Joarder wrote:
> >>> On Fri, Sep 21, 2018 at 3:06 PM Andrey Ryabinin <aryabinin@virtuozzo.com> wrote:
> >>>>
> >>>> On 09/20/2018 10:12 PM, Souptick Joarder wrote:
> >>>>> @@ -293,8 +293,9 @@ static int kcov_mmap(struct file *filep, struct vm_area_struct *vma)
> >>>>>               spin_unlock(&kcov->lock);
> >>>>>               for (off = 0; off < size; off += PAGE_SIZE) {
> >>>>>                       page = vmalloc_to_page(kcov->area + off);
> >>>>> -                     if (vm_insert_page(vma, vma->vm_start + off, page))
> >>>>> -                             WARN_ONCE(1, "vm_insert_page() failed");
> >>>>> +                     if (vmf_insert_page(vma, vma->vm_start + off, page)
> >>>>> +                                     != VM_FAULT_NOPAGE)
> >>>>> +                             WARN_ONCE(1, "vmf_insert_page() failed");
> >>>>
> >>>> Nack, don't see the reason for such change, it only makes code worse.
> >>>
> >>> Yes, it needed.
> >>
> >> No, it's not needed. vm_insert_page() works perfectly fine here.
> >>
> >>> Going forward vm_insert_page will be converted to
> >>> vmf_insert_page. As part of it, this code has to be converted to use
> >>> vmf_insert_page().
> >>
> >> This doesn't explain why such conversion would make sense for kcov_mmap().
> >
> > vm_insert_page used to return errno which individual drivers have to
> > map to VM_FAULT_CODE.
> > There were also places where return value of vm_insert_page was
> > ignored and return a
> > TRUE value.
> >
> > As part of vm_fault_t migration patches, we have identified and clean
> > up all these.
> > The plan was to introduce inline wrapper vmf_insert_page(), convert all
> > the page fault handlers to use it and then convert vm_insert_page to
> > vmf_insert_page. By now we have converted all the page fault handlers
> > to use this API.
> >
> > Still their are few places where vm_insert_page is called outside fault handlers
> > context to map kernel page to user vma and kernel/kcov.c is one of them.
> >
> > Now removing vm_insert_page from those few places one by one.
> >
> > Going forward vm_insert_page will be removed from kernel permanently,
> > so that new drivers can't use this API and create new errno to VM_FAULT_CODE
> > mapping code.
> >
>
> So, to avoid addition of new code that converts errno->VM_FAULT_* you propose to add
> new code that does the opposite - converts VM_FAULT_* to errno? How is that better?

I didn't propose that. It's not a better idea.
And from fault handlers prospective, with these changes, fault handlers code
will only return VM_FAULT_CODE not errno.

>
> And what function is going to be next? pte_none()? It is also used in fault handlers.
> Are there going to be vmf_pte_none() and complete removal of pte_none() "so that new drivers
> can't use pte_none() and create new pte_none() to VM_FAULT_CODE mapping code"?

vmf_insert_{pfn, page, mixed} are the functions which are planned to change.
vmf_insert_{pfn,mixed} are merged into linux-next.
There is no plan for any other functions.

>

> Don't you see that how that doesn't make any sense?
> Want vmf_insert_page() for easier error handling in fault handlers? - fine, use it in fault handlers.
> But leave the vm_insert_page() for the code that has nothing to do with fault handling.

If we leave vm_insert_page() for the code other than fault handling,
then drivers writer will
have choice to use this API in fault handlers context in future which
need to be blocked.

How can we block driver writer not to use vm_insert_page in fault
handlers context ??

Re: [PATCH] kernel/kcov: Replace vm_insert_page with vmf_insert_page

[Matthew Wilcox]

On Fri, Sep 21, 2018 at 12:42:54AM +0530, Souptick Joarder wrote:
>  		for (off = 0; off < size; off += PAGE_SIZE) {
>  			page = vmalloc_to_page(kcov->area + off);
> -			if (vm_insert_page(vma, vma->vm_start + off, page))
> -				WARN_ONCE(1, "vm_insert_page() failed");
> +			if (vmf_insert_page(vma, vma->vm_start + off, page)
> +					!= VM_FAULT_NOPAGE)
> +				WARN_ONCE(1, "vmf_insert_page() failed");
>  		}

I think this is the wrong approach (as well as being buggy).

We should have a vmalloc_insert_range() _or something similar_ that
replaces this entire loop.  That makes each _user_ simpler.  What you're
trying to do right now makes each user more complex, and that's the
wrong approach.

Re: [PATCH] kernel/kcov: Replace vm_insert_page with vmf_insert_page

[Souptick Joarder]

On Sat, Sep 22, 2018 at 7:52 PM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Fri, Sep 21, 2018 at 12:42:54AM +0530, Souptick Joarder wrote:
> >               for (off = 0; off < size; off += PAGE_SIZE) {
> >                       page = vmalloc_to_page(kcov->area + off);
> > -                     if (vm_insert_page(vma, vma->vm_start + off, page))
> > -                             WARN_ONCE(1, "vm_insert_page() failed");
> > +                     if (vmf_insert_page(vma, vma->vm_start + off, page)
> > +                                     != VM_FAULT_NOPAGE)
> > +                             WARN_ONCE(1, "vmf_insert_page() failed");
> >               }
>
> I think this is the wrong approach (as well as being buggy).
>
> We should have a vmalloc_insert_range() _or something similar_ that
> replaces this entire loop.  That makes each _user_ simpler.  What you're
> trying to do right now makes each user more complex, and that's the
> wrong approach.

There are two different opinions here.

Andrey mentioned, to use vmf_insert_page() in fault handlers context and
not to remove vm_insert_page in his review comments.

And your opinion is to have vmalloc_insert_range() _or something similar_
that will replace whole loop. with this vm_insert_page will be removed.

Which one is better to go with ??