| Message ID | 20240222080815.46291-1-zhengqi.arch@bytedance.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm: userfaultfd: fix unexpected change to src_folio when UFFDIO_MOVE fails | expand |
On 22.02.24 09:08, Qi Zheng wrote: > After ptep_clear_flush(), if we find that src_folio is pinned we will fail > UFFDIO_MOVE and put src_folio back to src_pte entry, but the change to > src_folio->{mapping,index} is not restored in this process. This is not > what we expected, so fix it. > > Fixes: adef440691ba ("userfaultfd: UFFDIO_MOVE uABI") > Signed-off-by: Qi Zheng <zhengqi.arch@bytedance.com> > --- > mm/userfaultfd.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c > index 4744d6a96f96..503ea77c81aa 100644 > --- a/mm/userfaultfd.c > +++ b/mm/userfaultfd.c > @@ -1008,9 +1008,6 @@ static int move_present_pte(struct mm_struct *mm, > goto out; > } > > - folio_move_anon_rmap(src_folio, dst_vma); > - WRITE_ONCE(src_folio->index, linear_page_index(dst_vma, dst_addr)); > - > orig_src_pte = ptep_clear_flush(src_vma, src_addr, src_pte); > /* Folio got pinned from under us. Put it back and fail the move. */ > if (folio_maybe_dma_pinned(src_folio)) { > @@ -1019,6 +1016,9 @@ static int move_present_pte(struct mm_struct *mm, > goto out; > } > > + folio_move_anon_rmap(src_folio, dst_vma); > + WRITE_ONCE(src_folio->index, linear_page_index(dst_vma, dst_addr)); > + > orig_dst_pte = mk_pte(&src_folio->page, dst_vma->vm_page_prot); > /* Follow mremap() behavior and treat the entry dirty after the move */ > orig_dst_pte = pte_mkwrite(pte_mkdirty(orig_dst_pte), dst_vma); Indeed, LGTM. Reviewed-by: David Hildenbrand <david@redhat.com>
On Thu, 22 Feb 2024 16:08:15 +0800 Qi Zheng <zhengqi.arch@bytedance.com> wrote: > After ptep_clear_flush(), if we find that src_folio is pinned we will fail > UFFDIO_MOVE and put src_folio back to src_pte entry, but the change to > src_folio->{mapping,index} is not restored in this process. This is not > what we expected, so fix it. > > Fixes: adef440691ba ("userfaultfd: UFFDIO_MOVE uABI") What are the expected worst-case userspace-visible runtime effects of this flaw?
On Thu, Feb 22, 2024 at 12:43 AM David Hildenbrand <david@redhat.com> wrote: > > On 22.02.24 09:08, Qi Zheng wrote: > > After ptep_clear_flush(), if we find that src_folio is pinned we will fail > > UFFDIO_MOVE and put src_folio back to src_pte entry, but the change to > > src_folio->{mapping,index} is not restored in this process. This is not > > what we expected, so fix it. > > > > Fixes: adef440691ba ("userfaultfd: UFFDIO_MOVE uABI") > > Signed-off-by: Qi Zheng <zhengqi.arch@bytedance.com> > > --- > > mm/userfaultfd.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c > > index 4744d6a96f96..503ea77c81aa 100644 > > --- a/mm/userfaultfd.c > > +++ b/mm/userfaultfd.c > > @@ -1008,9 +1008,6 @@ static int move_present_pte(struct mm_struct *mm, > > goto out; > > } > > > > - folio_move_anon_rmap(src_folio, dst_vma); > > - WRITE_ONCE(src_folio->index, linear_page_index(dst_vma, dst_addr)); > > - > > orig_src_pte = ptep_clear_flush(src_vma, src_addr, src_pte); > > /* Folio got pinned from under us. Put it back and fail the move. */ > > if (folio_maybe_dma_pinned(src_folio)) { > > @@ -1019,6 +1016,9 @@ static int move_present_pte(struct mm_struct *mm, > > goto out; > > } > > > > + folio_move_anon_rmap(src_folio, dst_vma); > > + WRITE_ONCE(src_folio->index, linear_page_index(dst_vma, dst_addr)); > > + > > orig_dst_pte = mk_pte(&src_folio->page, dst_vma->vm_page_prot); > > /* Follow mremap() behavior and treat the entry dirty after the move */ > > orig_dst_pte = pte_mkwrite(pte_mkdirty(orig_dst_pte), dst_vma); > > Indeed, LGTM. > > Reviewed-by: David Hildenbrand <david@redhat.com> Thanks for catching this! Makes total sense to check before modification. Reviewed-by: Suren Baghdasaryan <surenb@google.com> > > -- > Cheers, > > David / dhildenb >
On Thu, Feb 22, 2024 at 1:00 PM Andrew Morton <akpm@linux-foundation.org> wrote: > > On Thu, 22 Feb 2024 16:08:15 +0800 Qi Zheng <zhengqi.arch@bytedance.com> wrote: > > > After ptep_clear_flush(), if we find that src_folio is pinned we will fail > > UFFDIO_MOVE and put src_folio back to src_pte entry, but the change to > > src_folio->{mapping,index} is not restored in this process. This is not > > what we expected, so fix it. > > > > Fixes: adef440691ba ("userfaultfd: UFFDIO_MOVE uABI") > > What are the expected worst-case userspace-visible runtime effects of > this flaw? It can cause rmap for that page to be invalid. I guess memory corruption might be the visible effect?
On 22.02.24 22:56, Suren Baghdasaryan wrote: > On Thu, Feb 22, 2024 at 1:00 PM Andrew Morton <akpm@linux-foundation.org> wrote: >> >> On Thu, 22 Feb 2024 16:08:15 +0800 Qi Zheng <zhengqi.arch@bytedance.com> wrote: >> >>> After ptep_clear_flush(), if we find that src_folio is pinned we will fail >>> UFFDIO_MOVE and put src_folio back to src_pte entry, but the change to >>> src_folio->{mapping,index} is not restored in this process. This is not >>> what we expected, so fix it. >>> >>> Fixes: adef440691ba ("userfaultfd: UFFDIO_MOVE uABI") >> >> What are the expected worst-case userspace-visible runtime effects of >> this flaw? > > It can cause rmap for that page to be invalid. I guess memory > corruption might be the visible effect? At least swapout+migration would no longer work, because we might fail to locate the mappings of that folio. Memory corruption, not sure.
diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c index 4744d6a96f96..503ea77c81aa 100644 --- a/mm/userfaultfd.c +++ b/mm/userfaultfd.c @@ -1008,9 +1008,6 @@ static int move_present_pte(struct mm_struct *mm, goto out; } - folio_move_anon_rmap(src_folio, dst_vma); - WRITE_ONCE(src_folio->index, linear_page_index(dst_vma, dst_addr)); - orig_src_pte = ptep_clear_flush(src_vma, src_addr, src_pte); /* Folio got pinned from under us. Put it back and fail the move. */ if (folio_maybe_dma_pinned(src_folio)) { @@ -1019,6 +1016,9 @@ static int move_present_pte(struct mm_struct *mm, goto out; } + folio_move_anon_rmap(src_folio, dst_vma); + WRITE_ONCE(src_folio->index, linear_page_index(dst_vma, dst_addr)); + orig_dst_pte = mk_pte(&src_folio->page, dst_vma->vm_page_prot); /* Follow mremap() behavior and treat the entry dirty after the move */ orig_dst_pte = pte_mkwrite(pte_mkdirty(orig_dst_pte), dst_vma);
After ptep_clear_flush(), if we find that src_folio is pinned we will fail UFFDIO_MOVE and put src_folio back to src_pte entry, but the change to src_folio->{mapping,index} is not restored in this process. This is not what we expected, so fix it. Fixes: adef440691ba ("userfaultfd: UFFDIO_MOVE uABI") Signed-off-by: Qi Zheng <zhengqi.arch@bytedance.com> --- mm/userfaultfd.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)