Re: [PATCH 00/12] mm: free retracted page table by RCU
On Wed, 31 May 2023, Jann Horn wrote: > On Mon, May 29, 2023 at 8:11 AM Hugh Dickins wrote: > > Here is the third series of patches to mm (and a few architectures), based > > on v6.4-rc3 with the preceding two series applied: in which khugepaged > > takes advantage of pte_offset_map[_lock]() allowing for pmd transitions. > > To clarify: Part of the design here is that when you look up a user > page table with pte_offset_map_nolock() or pte_offset_map() without > holding mmap_lock in write mode, and you later lock the page table > yourself, you don't know whether you actually have the real page table > or a detached table that is currently in its RCU grace period, right? Right. (And I'd rather not assume anything of mmap_lock, but there are one or two or three places that may still do so.) > And detached tables are supposed to consist of only zeroed entries, > and we assume that no relevant codepath will do anything bad if one of > these functions spuriously returns a pointer to a page table full of > zeroed entries? (Nit that I expect you're well aware of: IIRC "zeroed" isn't 0 on s390.) If someone is using pte_offset_map() without lock, they must be prepared to accept page-table-like changes. The limits of pte_offset_map_nolock() with later spin_lock(ptl): I'm still exploring: there's certainly an argument that one ought to do a pmd_same() check before proceeding, but I don't think anywhere needs that at present. Whether the page table has to be full of zeroed entries when detached: I believe it is always like that at present (by the end of the series, when the collapse_pte_offset_map() oddity is fixed), but whether it needs to be so I'm not sure. Quite likely it will need to be; but I'm open to the possibility that all it needs is to be still a page table, with perhaps new entries from a new usage in it. The most obvious vital thing (in the split ptlock case) is that it remains a struct page with a usable ptl spinlock embedded in it. The question becomes more urgent when/if extending to replacing the pagetable pmd by huge pmd in one go, without any mmap_lock: powerpc wants to deposit the page table for later use even in the shmem/file case (and all arches in the anon case): I did work out the details once before, but I'm not sure whether I would still agree with myself; and was glad to leave replacement out of this series, to revisit some time later. > > So in particular, in handle_pte_fault() we can reach the "if > (unlikely(!pte_same(*vmf->pte, entry)))" with vmf->pte pointing to a > detached zeroed page table, but we're okay with that because in that > case we know that !pte_none(vmf->orig_pte)&&pte_none(*vmf->pte) , > which implies !pte_same(*vmf->pte, entry) , which means we'll bail > out? There is no current (even at end of series) circumstance in which we could be pointing to a detached page table there; but yes, I want to allow for that, and yes I agree with your analysis. But with the interesting unanswered question for the future, of what if the same value could be found there: would that imply it's safe to proceed, or would some further prevention be needed? > > If that's the intent, it might be good to add some comments, because > at least to me that's not very obvious. That's a very fair request; but I shall have difficulty deciding where to place such comments. I shall have to try, then you redirect me. And I think we approach this in opposite ways: my nature is to put some infrastructure in place, and then look at it to see what we can get away with; whereas your nature is to define upfront what the possibilities are. We can expect some tussles! Thanks, Hugh
Re: [PATCH 00/12] mm: free retracted page table by RCU
On Fri, Jun 2, 2023 at 6:37 AM Hugh Dickins wrote: > On Wed, 31 May 2023, Jann Horn wrote: > > On Mon, May 29, 2023 at 8:11 AM Hugh Dickins wrote: > > > Here is the third series of patches to mm (and a few architectures), based > > > on v6.4-rc3 with the preceding two series applied: in which khugepaged > > > takes advantage of pte_offset_map[_lock]() allowing for pmd transitions. > > > > To clarify: Part of the design here is that when you look up a user > > page table with pte_offset_map_nolock() or pte_offset_map() without > > holding mmap_lock in write mode, and you later lock the page table > > yourself, you don't know whether you actually have the real page table > > or a detached table that is currently in its RCU grace period, right? > > Right. (And I'd rather not assume anything of mmap_lock, but there are > one or two or three places that may still do so.) > > > And detached tables are supposed to consist of only zeroed entries, > > and we assume that no relevant codepath will do anything bad if one of > > these functions spuriously returns a pointer to a page table full of > > zeroed entries? > > (Nit that I expect you're well aware of: IIRC "zeroed" isn't 0 on s390.) I was not aware, thanks. I only knew that on Intel's Knights Landing CPUs, the A/D bits are ignored by pte_none() due to some erratum. > If someone is using pte_offset_map() without lock, they must be prepared > to accept page-table-like changes. The limits of pte_offset_map_nolock() > with later spin_lock(ptl): I'm still exploring: there's certainly an > argument that one ought to do a pmd_same() check before proceeding, > but I don't think anywhere needs that at present. > > Whether the page table has to be full of zeroed entries when detached: > I believe it is always like that at present (by the end of the series, > when the collapse_pte_offset_map() oddity is fixed), but whether it needs > to be so I'm not sure. Quite likely it will need to be; but I'm open to > the possibility that all it needs is to be still a page table, with > perhaps new entries from a new usage in it. My understanding is that at least handle_pte_fault(), the way it is currently written, would do bad things in that case: // assume we enter with mmap_lock in read mode, // for a write fault on a shared writable VMA without a page_mkwrite handler static vm_fault_t handle_pte_fault(struct vm_fault *vmf) { pte_t entry; if (unlikely(pmd_none(*vmf->pmd))) { [ not executed ] } else { /* * A regular pmd is established and it can't morph into a huge * pmd by anon khugepaged, since that takes mmap_lock in write * mode; but shmem or file collapse to THP could still morph * it into a huge pmd: just retry later if so. */ vmf->pte = pte_offset_map_nolock(vmf->vma->vm_mm, vmf->pmd, vmf->address, &vmf->ptl); if (unlikely(!vmf->pte)) [not executed] [assume that at this point, a concurrent THP collapse operation removes the page table, and the page table has now been reused and contains a read-only PTE] // this reads page table contents protected solely by RCU vmf->orig_pte = ptep_get_lockless(vmf->pte); vmf->flags |= FAULT_FLAG_ORIG_PTE_VALID; if (pte_none(vmf->orig_pte)) { pte_unmap(vmf->pte); vmf->pte = NULL; } } if (!vmf->pte) [not executed] if (!pte_present(vmf->orig_pte)) [not executed] if (pte_protnone(vmf->orig_pte) && vma_is_accessible(vmf->vma)) [not executed] spin_lock(vmf->ptl); entry = vmf->orig_pte; if (unlikely(!pte_same(*vmf->pte, entry))) { [not executed] } if (vmf->flags & (FAULT_FLAG_WRITE|FAULT_FLAG_UNSHARE)) { if (!pte_write(entry)) // This will go into wp_page_shared(), // which will call wp_page_reuse(), // which will upgrade the page to writable return do_wp_page(vmf); [ not executed ] } [ not executed ] } That looks like we could end up racing such that a read-only PTE in the reused page table gets upgraded to writable, which would probably be a security bug. But I guess if you added bailout checks to every page table lock operation, it'd be a different story, maybe? > The most obvious vital thing (in the split ptlock case) is that it > remains a struct page with a usable ptl spinlock embedded in it. > > The question becomes more urgent when/if extending to replacing the > pagetable pmd by huge pmd in one go, without any mmap_lock: powerpc > wants to deposit the page table for later use even in the shmem/file > case (and all arches in the anon case): I did work out the details once > before, but I'm not sure whether I would still agree with myself; and was > glad to leave replacement out of this series, to revisit some time later. > > > > > So in particular, in handle_pte_fault() we can reach the "if > > (unlikely(!pte_same(*vmf->pte, entry)))" with vmf->pte pointing to a > > detached zeroed page table, but we're okay with that because in that > > case
Re: [PATCH 00/12] mm: free retracted page table by RCU
On Fri, 2 Jun 2023, Jann Horn wrote: > On Fri, Jun 2, 2023 at 6:37 AM Hugh Dickins wrote: > > > The most obvious vital thing (in the split ptlock case) is that it > > remains a struct page with a usable ptl spinlock embedded in it. > > > > The question becomes more urgent when/if extending to replacing the > > pagetable pmd by huge pmd in one go, without any mmap_lock: powerpc > > wants to deposit the page table for later use even in the shmem/file > > case (and all arches in the anon case): I did work out the details once > > before, but I'm not sure whether I would still agree with myself; and was > > glad to leave replacement out of this series, to revisit some time later. > > > > > > > > So in particular, in handle_pte_fault() we can reach the "if > > > (unlikely(!pte_same(*vmf->pte, entry)))" with vmf->pte pointing to a > > > detached zeroed page table, but we're okay with that because in that > > > case we know that !pte_none(vmf->orig_pte)&&pte_none(*vmf->pte) , > > > which implies !pte_same(*vmf->pte, entry) , which means we'll bail > > > out? > > > > There is no current (even at end of series) circumstance in which we > > could be pointing to a detached page table there; but yes, I want to > > allow for that, and yes I agree with your analysis. > > Hmm, what am I missing here? I spent quite a while trying to reconstruct what I had been thinking, what meaning of "detached" or "there" I had in mind when I asserted so confidently "There is no current (even at end of series) circumstance in which we could be pointing to a detached page table there". But had to give up and get on with more useful work. Of course you are right, and that is what this series is about. Hugh > > static vm_fault_t handle_pte_fault(struct vm_fault *vmf) > { > pte_t entry; > > if (unlikely(pmd_none(*vmf->pmd))) { > [not executed] > } else { > /* > * A regular pmd is established and it can't morph into a huge > * pmd by anon khugepaged, since that takes mmap_lock in write > * mode; but shmem or file collapse to THP could still morph > * it into a huge pmd: just retry later if so. > */ > vmf->pte = pte_offset_map_nolock(vmf->vma->vm_mm, vmf->pmd, > vmf->address, &vmf->ptl); > if (unlikely(!vmf->pte)) > [not executed] > // this reads a present readonly PTE > vmf->orig_pte = ptep_get_lockless(vmf->pte); > vmf->flags |= FAULT_FLAG_ORIG_PTE_VALID; > > if (pte_none(vmf->orig_pte)) { > [not executed] > } > } > > [at this point, a concurrent THP collapse operation detaches the page table] > // vmf->pte now points into a detached page table > > if (!vmf->pte) > [not executed] > > if (!pte_present(vmf->orig_pte)) > [not executed] > > if (pte_protnone(vmf->orig_pte) && vma_is_accessible(vmf->vma)) > [not executed] > > spin_lock(vmf->ptl); > entry = vmf->orig_pte; > // vmf->pte still points into a detached page table > if (unlikely(!pte_same(*vmf->pte, entry))) { > update_mmu_tlb(vmf->vma, vmf->address, vmf->pte); > goto unlock; > } > [...] > }
Re: [PATCH 00/12] mm: free retracted page table by RCU
On Mon, May 29, 2023 at 8:11 AM Hugh Dickins wrote: > Here is the third series of patches to mm (and a few architectures), based > on v6.4-rc3 with the preceding two series applied: in which khugepaged > takes advantage of pte_offset_map[_lock]() allowing for pmd transitions. To clarify: Part of the design here is that when you look up a user page table with pte_offset_map_nolock() or pte_offset_map() without holding mmap_lock in write mode, and you later lock the page table yourself, you don't know whether you actually have the real page table or a detached table that is currently in its RCU grace period, right? And detached tables are supposed to consist of only zeroed entries, and we assume that no relevant codepath will do anything bad if one of these functions spuriously returns a pointer to a page table full of zeroed entries? So in particular, in handle_pte_fault() we can reach the "if (unlikely(!pte_same(*vmf->pte, entry)))" with vmf->pte pointing to a detached zeroed page table, but we're okay with that because in that case we know that !pte_none(vmf->orig_pte)&&pte_none(*vmf->pte) , which implies !pte_same(*vmf->pte, entry) , which means we'll bail out? If that's the intent, it might be good to add some comments, because at least to me that's not very obvious.