On 04/07/2015 04:26 PM, Kirill A. Shutemov wrote:
> On Tue, Apr 07, 2015 at 04:17:00PM +0300, Kirill A. Shutemov wrote:
>> On Tue, Apr 07, 2015 at 03:57:32PM +0300, Boaz Harrosh wrote:
>>> +/*
>>> + * Handle write page faults for VM_MIXEDMAP or VM_PFNMAP for a VM_SHARED
>>> + * mapping
>>> + */
>>> +static int wp_pfn_shared(struct mm_struct *mm,
>>> + struct vm_area_struct *vma, unsigned long address,
>>> + pte_t *page_table, spinlock_t *ptl, pte_t orig_pte,
>>> + pmd_t *pmd)
>>> +{
>>> + if (vma->vm_ops && vma->vm_ops->pfn_mkwrite) {
>>> + struct vm_fault vmf = {
>>> + .page = NULL,
>>> + .pgoff = linear_page_index(vma, address),
>>> + .virtual_address = (void __user *)(address & PAGE_MASK),
>>> + .flags = FAULT_FLAG_WRITE | FAULT_FLAG_MKWRITE,
>>> + };
>>> + int ret;
>>> +
>>> + pte_unmap_unlock(page_table, ptl);
>>> + ret = vma->vm_ops->pfn_mkwrite(vma, &vmf);
>>> + if (ret & VM_FAULT_ERROR)
>>> + return ret;
>>> + page_table = pte_offset_map_lock(mm, pmd, address, &ptl);
>>> + /* Did pfn_mkwrite already fixed up the pte */
>
> Oh. I guess you've missunderstood why we need pte_same() check below.
> It's not about ->pfn_mkwrite() changing the pte (generatlly, it should
> not). It's requited to address race with parallel page fault to the pte.
>
>>> + if (!pte_same(*page_table, orig_pte)) {
>>> + pte_unmap_unlock(page_table, ptl);
>>> + return ret;
>>
>> This should be "return 0;", shouldn't it?
>>
>> VM_FAULT_NOPAGE would imply you've installed new pte, but you did not.
Changing this to "return 0" would be very scary for me. Because I'm running
with this code for 1/2 a year now. And it is stable. You see since the original
code it was always doing just that pte_unmap_unlock && return ret. (See the patch
based on 4.0)
I did not understand if you want that I keep it "return ret".
I gather that you would like the comment changed, about the changed pte.
Both here and at Documentation/.../locking.
I'll send a new patch just tell me if you want the reurn thing
Thank you
Boaz
--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html