On 08.03.22 14:20, Gerald Schaefer wrote:
> On Tue, 8 Mar 2022 13:24:19 +0100
> David Hildenbrand <david@xxxxxxxxxx> wrote:
>
> [...]
>>
>> From 1e51e8a93894f87c0a4d0e908391e0628ae56afe Mon Sep 17 00:00:00 2001
>> From: David Hildenbrand <david@xxxxxxxxxx>
>> Date: Tue, 8 Mar 2022 12:51:26 +0100
>> Subject: [PATCH] mm/gup: fix buffered I/O on s390x with pagefaults disabled
>>
>> On s390x, we actually need a pte_mkyoung() / pte_mkdirty() instead of
>> going via the page and leaving the PTE unmodified. E.g., if we only
>> mark the page accessed via mark_page_accessed() when doing a FOLL_TOUCH,
>> we'll miss to clear the HW invalid bit in the pte and subsequent accesses
>> via the MMU would still require a pagefault.
>>
>> Otherwise, buffered I/O will loop forever because it will keep stumling
>> over the set HW invalid bit, requiring a page fault.
>>
>> Reported-by: Andreas Gruenbacher <agruenba@xxxxxxxxxx>
>> Signed-off-by: David Hildenbrand <david@xxxxxxxxxx>
>> ---
>> mm/gup.c | 32 +++++++++++++++++++++++++-------
>> 1 file changed, 25 insertions(+), 7 deletions(-)
>>
>> diff --git a/mm/gup.c b/mm/gup.c
>> index a9d4d724aef7..de3311feb377 100644
>> --- a/mm/gup.c
>> +++ b/mm/gup.c
>> @@ -587,15 +587,33 @@ static struct page *follow_page_pte(struct vm_area_struct *vma,
>> }
>> }
>> if (flags & FOLL_TOUCH) {
>> - if ((flags & FOLL_WRITE) &&
>> - !pte_dirty(pte) && !PageDirty(page))
>> - set_page_dirty(page);
>> /*
>> - * pte_mkyoung() would be more correct here, but atomic care
>> - * is needed to avoid losing the dirty bit: it is easier to use
>> - * mark_page_accessed().
>> + * We have to be careful with updating the PTE on architectures
>> + * that have a HW dirty bit: while updating the PTE we might
>> + * lose that bit again and we'd need an atomic update: it is
>> + * easier to leave the PTE untouched for these architectures.
>> + *
>> + * s390x doesn't have a hw referenced / dirty bit and e.g., sets
>> + * the hw invalid bit in pte_mkold(), to catch further
>> + * references. We have to update the PTE here to e.g., clear the
>> + * invalid bit; otherwise, callers that rely on not requiring
>> + * an MMU fault once GUP(FOLL_TOUCH) succeeded will loop forever
>> + * because the page won't actually be accessible via the MMU.
>> */
>> - mark_page_accessed(page);
>> + if (IS_ENABLED(CONFIG_S390)) {
>> + pte = pte_mkyoung(pte);
>> + if (flags & FOLL_WRITE)
>> + pte = pte_mkdirty(pte);
>> + if (!pte_same(pte, *ptep)) {
>> + set_pte_at(vma->vm_mm, address, ptep, pte);
>> + update_mmu_cache(vma, address, ptep);
>> + }
>> + } else {
>> + if ((flags & FOLL_WRITE) &&
>> + !pte_dirty(pte) && !PageDirty(page))
>> + set_page_dirty(page);
>> + mark_page_accessed(page);
>> + }
>> }
>> if ((flags & FOLL_MLOCK) && (vma->vm_flags & VM_LOCKED)) {
>> /* Do not mlock pte-mapped THP */
>
> Thanks David, your analysis looks valid, at least it seems that you found
> a scenario where we would have HW invalid bit set due to pte_mkold() in
> ptep_clear_flush_young(), and still GUP would find and return that page, IIUC.
>
> I think pte handling should be similar to pmd handling in follow_trans_huge_pmd()
> -> touch_pmd(), or cow_user_page() (see comment on software "accessed" bits),
> which is more or less what your patch does.
>
> Some possible concerns:
> - set_page_dirty() would not be done any more for s390, is that intended and ok?
I strongly assume so, because the page is mapped via a PTE, which is
writable and dirty. This is similar to THP logic.
> - using set_pte_at() here seems a bit dangerous, as I'm not sure if this will
> always only operate on invalid PTEs. Using it on active valid PTEs could
> result in TLB issues because of missing flush. Also not sure about kvm impact.
> Using ptep_set_access_flags() seems safer, again similar to touch_pmd() and
> also cow_user_page().
Yeah, I sticked to what follow_pfn_pte() does for simplicity for now.
But I agree that following what touch_pmd() does looks saner --
ptep_set_access_flags().
>
> Looking at cow_user_page(), I also wonder if the arch_faults_on_old_pte()
> logic could be used here. I must admit that I did not really understand the
> "losing the dirty bit" part of the comment, but it seems that we might need
> to not only check for arch_faults_on_old_pte(), but also for something like
> "arch_faults_for_dirty_pte".
>
> Last but not least, IIUC, this issue should affect all archs that return
> true on arch_faults_on_old_pte(). After all, the basic problem seems to be
> that a pagefault is required for PTEs marked as old, in combination with
> GUP still returning a valid page. So maybe this should not be restricted
> to IS_ENABLED(CONFIG_S390).
Yeah, as raised, the IS_ENABLED(CONFIG_S390) part is just a quick hack
to see if this would fix the issue.
arch_faults_on_dirty_pte / arch_faults_on_old_pte might be a
replacement. We just would have to be careful for architectures that
e.g., have arch_faults_on_old_pte=true and
arch_faults_on_dirty_pte=false (i.e., hw dirty bit but no hw accessed
bit). Would have to think about how to handle that properly ...
Thanks!
--
Thanks,
David / dhildenb
