Date:   Wed, 1 Mar 2023 10:19:31 -0500
From:   Peter Xu <peterx@redhat.com>
To:     Muhammad Usama Anjum <usama.anjum@collabora.com>
Cc:     linux-kernel@vger.kernel.org, linux-mm@kvack.org,
        Andrea Arcangeli <aarcange@redhat.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Mike Rapoport <rppt@linux.vnet.ibm.com>,
        Axel Rasmussen <axelrasmussen@google.com>,
        Nadav Amit <nadav.amit@gmail.com>,
        David Hildenbrand <david@redhat.com>,
        "kernel@collabora.com" <kernel@collabora.com>
Subject: Re: [PATCH v2] mm/uffd: UFFD_FEATURE_WP_UNPOPULATED
Message-ID: <Y/9tA2EVglOJ0Ap1@x1n>
References: <20230227230044.1596744-1-peterx@redhat.com>
 <Y/1Mh5uivFt+zWKM@x1n>
 <e1e0ed70-76df-647f-30ac-0bb6ae8dc05c@collabora.com>
 <Y/4kjnom6MQqh9iA@x1n>
 <fb7ec372-2b16-14e1-a8cd-a90f4449661f@collabora.com>
 <640319be-ddb6-d74f-b731-eee5ceab3d01@collabora.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <640319be-ddb6-d74f-b731-eee5ceab3d01@collabora.com>
Precedence: bulk

On Wed, Mar 01, 2023 at 12:55:51PM +0500, Muhammad Usama Anjum wrote:
> Hi Peter,

Hi, Muhammad,

> While using WP_UNPOPULATED, we get stuck if newly allocated memory is read
> without initialization. This can be reproduced by either of the following
> statements:
>     printf("%c", buffer[0]);
>     buffer[0]++;
> 
> This bug has start to appear on this patch. How are you handling reading
> newly allocated memory when WP_UNPOPULATED is defined?

Yes it's a bug, thanks for the reproducer. You're right I missed a trivial
but important detail.  Could you try apply below on top?

---8<---
diff --git a/mm/memory.c b/mm/memory.c
index 46934133bd0b..2f4b3892948b 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -4062,7 +4062,7 @@ static vm_fault_t do_anonymous_page(struct vm_fault *vmf)
                                                vma->vm_page_prot));
                vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd,
                                vmf->address, &vmf->ptl);
-               if (!pte_none(*vmf->pte)) {
+               if (vmf_pte_changed(vmf)) {
                        update_mmu_tlb(vma, vmf->address, vmf->pte);
                        goto unlock;
                }
---8<---

I can send a new version after you confirmed it at least works on your
side. I'll also add some more test to cover that in the next version.

The current smoke test within this patch is really light; I somehow rely on
you on this patch on the testing side, and thanks for that.

> Running my pagemap_ioctl selftest as benchmark in a VM:
> without zeropage / wp_unpopulated (decide from pte_none() if page is dirty
> or not, buggy and wrong implementation, just for reference)
> 26.608 seconds
> with zeropage
> 39.203 seconds
> with wp_unpopulated
> 62.907 seconds
> 
> 136% worse performance overall
> 60% worse performance of unpopulated than zeropage

Yes this is unfortunate, because we're protecting more things than before
when with WP_ZEROPAGE / WP_UNPOPULATED but that's what it is for (when we
want to make sure that accuracy on the holes).

I didn't look closer to your whole test suite yet, but my pure test on
protection above should mean that it's still much better for such a use
case than either (1) pre-read or (2) MADV_POPULATE_READ.

Again, I hope the performance result is not a concern to you.  If it is,
please let us know.

Thanks,

-- 
Peter Xu