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@xxxxxxxxxxxxx> wrote:
>
>
>
> On 09/21/2018 04:25 PM, Souptick Joarder wrote:
> > On Fri, Sep 21, 2018 at 5:22 PM Andrey Ryabinin <aryabinin@xxxxxxxxxxxxx> wrote:
> >> On 09/21/2018 01:03 PM, Souptick Joarder wrote:
> >>> On Fri, Sep 21, 2018 at 3:06 PM Andrey Ryabinin <aryabinin@xxxxxxxxxxxxx> 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 ??