Re: [PATCH 1/2] mm: Allow architectures to request 'old' entries when prefaulting

[Linus Torvalds <torvalds@linux-foundation.org>]

[-- Attachment #1: Type: text/plain, Size: 1983 bytes --]

On Mon, Dec 28, 2020 at 4:53 AM Kirill A. Shutemov <kirill@shutemov.name> wrote:
>
> So far I only found one more pin leak and always-true check. I don't see
> how can it lead to crash or corruption. Keep looking.

Well, I noticed that the nommu.c version of filemap_map_pages() needs
fixing, but that's obviously not the case Hugh sees.

No,m I think the problem is the

        pte_unmap_unlock(vmf->pte, vmf->ptl);

at the end of filemap_map_pages().

Why?

Because we've been updating vmf->pte as we go along:

                vmf->pte += xas.xa_index - last_pgoff;

and I think that by the time we get to that "pte_unmap_unlock()",
vmf->pte potentially points to past the edge of the page directory.

I think that is the bug that Hugh sees - simply because we get
entirely confused about the page table locking. And it would match the
latest change, which was all about moving that unlock from the caller
to filemap_map_pages(), and now it's missing the pte fixup..

I personally think it's wrong to update vmf->pte at all. We should
just have a local 'ptep' pointer that we update as we walk along. But
that requires another change to the calling convention, namely to
"do_set_pte()".

Also, considering how complicated this patch is getting, I think it
might be good to try to split it up a bit.

In particular, I think the calling convention change for
"filemap_map_pages()" could be done first and independently. And then
as a second step, move the VM_FAULT_NOPAGE and "pte_unmap_lock()" from
the callers to filemap_map_pages().

And then only as the final step, do that nice re-organization with
first_map_page/next_map_page() and moving the locking from
alloc_set_pte() into filemap_map_pages()..

How does that sound?

Anyway, Hugh, if it's about overshooting the pte pointer, maybe this
absolutely horrendous and disgusting patch fixes it without the above
kinds of more extensive cleanups. Worth testing, perhaps, even if it's
too ugly for words?

               Linus

[-- Attachment #2: patch --]
[-- Type: application/octet-stream, Size: 1360 bytes --]

 mm/filemap.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/mm/filemap.c b/mm/filemap.c
index 42d7a58e3a14..74384f9f1776 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -3022,6 +3022,8 @@ vm_fault_t filemap_map_pages(struct vm_fault *vmf, unsigned long address,
 	struct page *head, *page;
 	unsigned int mmap_miss = READ_ONCE(file->f_ra.mmap_miss);
 	vm_fault_t ret = 0;
+	pte_t *orig_pte;
+	unsigned long orig_address;
 
 	rcu_read_lock();
 	head = first_map_page(vmf, &xas, end_pgoff);
@@ -3037,6 +3039,14 @@ vm_fault_t filemap_map_pages(struct vm_fault *vmf, unsigned long address,
 
 	vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd,
 				       vmf->address, &vmf->ptl);
+
+	/*
+	 * Disgusting - we should not update vmf->pte and ->address,
+	 * but do_set_pte() needs it
+	 */
+	orig_pte = vmf->pte;
+	orig_address = vmf->address;
+
 	do {
 		page = find_subpage(head, xas.xa_index);
 		if (PageHWPoison(page))
@@ -3066,6 +3076,10 @@ vm_fault_t filemap_map_pages(struct vm_fault *vmf, unsigned long address,
 		unlock_page(head);
 		put_page(head);
 	} while ((head = next_map_page(vmf, &xas, end_pgoff)) != NULL);
+
+	/* Hackety hack - reset the vmf state back */
+	vmf->pte = orig_pte;
+	vmf->address = orig_address;
 	pte_unmap_unlock(vmf->pte, vmf->ptl);
 	rcu_read_unlock();
 	WRITE_ONCE(file->f_ra.mmap_miss, mmap_miss);