Re: [PATCH 3/3] mm: unexport follow_pfn
On Thu, Apr 08, 2021 at 01:40:59PM +0200, Paolo Bonzini wrote: > On 08/04/21 12:05, Daniel Vetter wrote: > > On Mon, Mar 29, 2021 at 10:31:01AM -0300, Jason Gunthorpe wrote: > > > On Tue, Mar 16, 2021 at 04:33:03PM +0100, Daniel Vetter wrote: > > > > Both kvm (in bd2fae8da794 ("KVM: do not assume PTE is writable after > > > > follow_pfn")) and vfio (in 07956b6269d3 ("vfio/type1: Use > > > > follow_pte()")) have lost their callsites of follow_pfn(). All the > > > > other ones have been switched over to unsafe_follow_pfn because they > > > > cannot be fixed without breaking userspace api. > > > > > > > > Argueably the vfio code is still racy, but that's kinda a bigger > > > > > > vfio and kvm > > > > Hm I thought kvm is non-racy due to the mmu notifier catch races? > > No, but the plan is indeed to have some struct for each page that uses > follow_pfn and update it from the MMU notifiers. Thanks for clarifying, I've fixed the commit message to mention both vfio and kvm as Jason suggested. I didn't know that the follow_pte usage in kvm still has some gaps wrt what's invalidated with mmu notifiers. Thanks, Daniel > > Paolo > > > > > > > > picture. But since it does leak the pte beyond where it drops the pt > > > > lock, without anything else like an mmu notifier guaranteeing > > > > coherence, the problem is at least clearly visible in the vfio code. > > > > So good enough with me. > > > > > > > > I've decided to keep the explanation that after dropping the pt lock > > > > you must have an mmu notifier if you keep using the pte somehow by > > > > adjusting it and moving it into the kerneldoc for the new follow_pte() > > > > function. > > > > > > > > Cc: 3...@google.com > > > > Cc: Jann Horn > > > > Cc: Paolo Bonzini > > > > Cc: Jason Gunthorpe > > > > Cc: Cornelia Huck > > > > Cc: Peter Xu > > > > Cc: Alex Williamson > > > > Cc: linux...@kvack.org > > > > Cc: linux-arm-ker...@lists.infradead.org > > > > Cc: linux-samsung-...@vger.kernel.org > > > > Cc: linux-me...@vger.kernel.org > > > > Cc: k...@vger.kernel.org > > > > Signed-off-by: Daniel Vetter > > > > --- > > > > include/linux/mm.h | 2 -- > > > > mm/memory.c| 26 +- > > > > mm/nommu.c | 13 + > > > > 3 files changed, 6 insertions(+), 35 deletions(-) > > > > > > Reviewed-by: Jason Gunthorpe > > > > Thanks for your r-b tags, I'll add them. > > -Daniel > > > > > > > > Jason > > > > ___ > dri-devel mailing list > dri-de...@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Re: [PATCH 3/3] mm: unexport follow_pfn
On 08/04/21 12:05, Daniel Vetter wrote: On Mon, Mar 29, 2021 at 10:31:01AM -0300, Jason Gunthorpe wrote: On Tue, Mar 16, 2021 at 04:33:03PM +0100, Daniel Vetter wrote: Both kvm (in bd2fae8da794 ("KVM: do not assume PTE is writable after follow_pfn")) and vfio (in 07956b6269d3 ("vfio/type1: Use follow_pte()")) have lost their callsites of follow_pfn(). All the other ones have been switched over to unsafe_follow_pfn because they cannot be fixed without breaking userspace api. Argueably the vfio code is still racy, but that's kinda a bigger vfio and kvm Hm I thought kvm is non-racy due to the mmu notifier catch races? No, but the plan is indeed to have some struct for each page that uses follow_pfn and update it from the MMU notifiers. Paolo picture. But since it does leak the pte beyond where it drops the pt lock, without anything else like an mmu notifier guaranteeing coherence, the problem is at least clearly visible in the vfio code. So good enough with me. I've decided to keep the explanation that after dropping the pt lock you must have an mmu notifier if you keep using the pte somehow by adjusting it and moving it into the kerneldoc for the new follow_pte() function. Cc: 3...@google.com Cc: Jann Horn Cc: Paolo Bonzini Cc: Jason Gunthorpe Cc: Cornelia Huck Cc: Peter Xu Cc: Alex Williamson Cc: linux...@kvack.org Cc: linux-arm-ker...@lists.infradead.org Cc: linux-samsung-...@vger.kernel.org Cc: linux-me...@vger.kernel.org Cc: k...@vger.kernel.org Signed-off-by: Daniel Vetter --- include/linux/mm.h | 2 -- mm/memory.c| 26 +- mm/nommu.c | 13 + 3 files changed, 6 insertions(+), 35 deletions(-) Reviewed-by: Jason Gunthorpe Thanks for your r-b tags, I'll add them. -Daniel Jason
Re: [PATCH 3/3] mm: unexport follow_pfn
On Mon, Mar 29, 2021 at 10:31:01AM -0300, Jason Gunthorpe wrote: > On Tue, Mar 16, 2021 at 04:33:03PM +0100, Daniel Vetter wrote: > > Both kvm (in bd2fae8da794 ("KVM: do not assume PTE is writable after > > follow_pfn")) and vfio (in 07956b6269d3 ("vfio/type1: Use > > follow_pte()")) have lost their callsites of follow_pfn(). All the > > other ones have been switched over to unsafe_follow_pfn because they > > cannot be fixed without breaking userspace api. > > > > Argueably the vfio code is still racy, but that's kinda a bigger > > vfio and kvm Hm I thought kvm is non-racy due to the mmu notifier catch races? > > > picture. But since it does leak the pte beyond where it drops the pt > > lock, without anything else like an mmu notifier guaranteeing > > coherence, the problem is at least clearly visible in the vfio code. > > So good enough with me. > > > > I've decided to keep the explanation that after dropping the pt lock > > you must have an mmu notifier if you keep using the pte somehow by > > adjusting it and moving it into the kerneldoc for the new follow_pte() > > function. > > > > Cc: 3...@google.com > > Cc: Jann Horn > > Cc: Paolo Bonzini > > Cc: Jason Gunthorpe > > Cc: Cornelia Huck > > Cc: Peter Xu > > Cc: Alex Williamson > > Cc: linux...@kvack.org > > Cc: linux-arm-ker...@lists.infradead.org > > Cc: linux-samsung-...@vger.kernel.org > > Cc: linux-me...@vger.kernel.org > > Cc: k...@vger.kernel.org > > Signed-off-by: Daniel Vetter > > --- > > include/linux/mm.h | 2 -- > > mm/memory.c| 26 +- > > mm/nommu.c | 13 + > > 3 files changed, 6 insertions(+), 35 deletions(-) > > Reviewed-by: Jason Gunthorpe Thanks for your r-b tags, I'll add them. -Daniel > > Jason -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Re: [PATCH 3/3] mm: unexport follow_pfn
On Tue, Mar 16, 2021 at 04:33:03PM +0100, Daniel Vetter wrote: > Both kvm (in bd2fae8da794 ("KVM: do not assume PTE is writable after > follow_pfn")) and vfio (in 07956b6269d3 ("vfio/type1: Use > follow_pte()")) have lost their callsites of follow_pfn(). All the > other ones have been switched over to unsafe_follow_pfn because they > cannot be fixed without breaking userspace api. > > Argueably the vfio code is still racy, but that's kinda a bigger vfio and kvm > picture. But since it does leak the pte beyond where it drops the pt > lock, without anything else like an mmu notifier guaranteeing > coherence, the problem is at least clearly visible in the vfio code. > So good enough with me. > > I've decided to keep the explanation that after dropping the pt lock > you must have an mmu notifier if you keep using the pte somehow by > adjusting it and moving it into the kerneldoc for the new follow_pte() > function. > > Cc: 3...@google.com > Cc: Jann Horn > Cc: Paolo Bonzini > Cc: Jason Gunthorpe > Cc: Cornelia Huck > Cc: Peter Xu > Cc: Alex Williamson > Cc: linux...@kvack.org > Cc: linux-arm-ker...@lists.infradead.org > Cc: linux-samsung-...@vger.kernel.org > Cc: linux-me...@vger.kernel.org > Cc: k...@vger.kernel.org > Signed-off-by: Daniel Vetter > --- > include/linux/mm.h | 2 -- > mm/memory.c| 26 +- > mm/nommu.c | 13 + > 3 files changed, 6 insertions(+), 35 deletions(-) Reviewed-by: Jason Gunthorpe Jason
Re: [PATCH 3/3] mm: unexport follow_pfn
On Wed, Mar 24, 2021 at 08:17:10PM +0100, Daniel Vetter wrote: > On Wed, Mar 24, 2021 at 09:52:11AM -0300, Jason Gunthorpe wrote: > > On Tue, Mar 16, 2021 at 04:33:03PM +0100, Daniel Vetter wrote: > > > Both kvm (in bd2fae8da794 ("KVM: do not assume PTE is writable after > > > follow_pfn")) and vfio (in 07956b6269d3 ("vfio/type1: Use > > > follow_pte()")) have lost their callsites of follow_pfn(). All the > > > other ones have been switched over to unsafe_follow_pfn because they > > > cannot be fixed without breaking userspace api. > > > > > > Argueably the vfio code is still racy, but that's kinda a bigger > > > picture. But since it does leak the pte beyond where it drops the pt > > > lock, without anything else like an mmu notifier guaranteeing > > > coherence, the problem is at least clearly visible in the vfio code. > > > So good enough with me. > > > > > > I've decided to keep the explanation that after dropping the pt lock > > > you must have an mmu notifier if you keep using the pte somehow by > > > adjusting it and moving it into the kerneldoc for the new follow_pte() > > > function. > > > > > > Cc: 3...@google.com > > > Cc: Jann Horn > > > Cc: Paolo Bonzini > > > Cc: Jason Gunthorpe > > > Cc: Cornelia Huck > > > Cc: Peter Xu > > > Cc: Alex Williamson > > > Cc: linux...@kvack.org > > > Cc: linux-arm-ker...@lists.infradead.org > > > Cc: linux-samsung-...@vger.kernel.org > > > Cc: linux-me...@vger.kernel.org > > > Cc: k...@vger.kernel.org > > > Signed-off-by: Daniel Vetter > > > --- > > > include/linux/mm.h | 2 -- > > > mm/memory.c| 26 +- > > > mm/nommu.c | 13 + > > > 3 files changed, 6 insertions(+), 35 deletions(-) > > > > I think this is the right thing to do. > > Was just about to smash this into the topic branch for testing in > linux-next. Feel like an ack on the series, or at least the two mm > patches? Pushed them to my topic branch for a bit of testing in linux-next, hopefully goes all fine for a pull for 5.13. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Re: [PATCH 3/3] mm: unexport follow_pfn
On Wed, Mar 24, 2021 at 09:52:11AM -0300, Jason Gunthorpe wrote: > On Tue, Mar 16, 2021 at 04:33:03PM +0100, Daniel Vetter wrote: > > Both kvm (in bd2fae8da794 ("KVM: do not assume PTE is writable after > > follow_pfn")) and vfio (in 07956b6269d3 ("vfio/type1: Use > > follow_pte()")) have lost their callsites of follow_pfn(). All the > > other ones have been switched over to unsafe_follow_pfn because they > > cannot be fixed without breaking userspace api. > > > > Argueably the vfio code is still racy, but that's kinda a bigger > > picture. But since it does leak the pte beyond where it drops the pt > > lock, without anything else like an mmu notifier guaranteeing > > coherence, the problem is at least clearly visible in the vfio code. > > So good enough with me. > > > > I've decided to keep the explanation that after dropping the pt lock > > you must have an mmu notifier if you keep using the pte somehow by > > adjusting it and moving it into the kerneldoc for the new follow_pte() > > function. > > > > Cc: 3...@google.com > > Cc: Jann Horn > > Cc: Paolo Bonzini > > Cc: Jason Gunthorpe > > Cc: Cornelia Huck > > Cc: Peter Xu > > Cc: Alex Williamson > > Cc: linux...@kvack.org > > Cc: linux-arm-ker...@lists.infradead.org > > Cc: linux-samsung-...@vger.kernel.org > > Cc: linux-me...@vger.kernel.org > > Cc: k...@vger.kernel.org > > Signed-off-by: Daniel Vetter > > --- > > include/linux/mm.h | 2 -- > > mm/memory.c| 26 +- > > mm/nommu.c | 13 + > > 3 files changed, 6 insertions(+), 35 deletions(-) > > I think this is the right thing to do. Was just about to smash this into the topic branch for testing in linux-next. Feel like an ack on the series, or at least the two mm patches? -Daniel > > Alex is working on fixing VFIO and while kvm is still racy using > follow pte, I think they are working on it too? > > Jason -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Re: [PATCH 3/3] mm: unexport follow_pfn
On 24/03/21 13:52, Jason Gunthorpe wrote: I think this is the right thing to do. Alex is working on fixing VFIO and while kvm is still racy using follow pte, I think they are working on it too? Yeah, or at least we have a plan. Paolo
Re: [PATCH 3/3] mm: unexport follow_pfn
On Tue, Mar 16, 2021 at 04:33:03PM +0100, Daniel Vetter wrote: > Both kvm (in bd2fae8da794 ("KVM: do not assume PTE is writable after > follow_pfn")) and vfio (in 07956b6269d3 ("vfio/type1: Use > follow_pte()")) have lost their callsites of follow_pfn(). All the > other ones have been switched over to unsafe_follow_pfn because they > cannot be fixed without breaking userspace api. > > Argueably the vfio code is still racy, but that's kinda a bigger > picture. But since it does leak the pte beyond where it drops the pt > lock, without anything else like an mmu notifier guaranteeing > coherence, the problem is at least clearly visible in the vfio code. > So good enough with me. > > I've decided to keep the explanation that after dropping the pt lock > you must have an mmu notifier if you keep using the pte somehow by > adjusting it and moving it into the kerneldoc for the new follow_pte() > function. > > Cc: 3...@google.com > Cc: Jann Horn > Cc: Paolo Bonzini > Cc: Jason Gunthorpe > Cc: Cornelia Huck > Cc: Peter Xu > Cc: Alex Williamson > Cc: linux...@kvack.org > Cc: linux-arm-ker...@lists.infradead.org > Cc: linux-samsung-...@vger.kernel.org > Cc: linux-me...@vger.kernel.org > Cc: k...@vger.kernel.org > Signed-off-by: Daniel Vetter > --- > include/linux/mm.h | 2 -- > mm/memory.c| 26 +- > mm/nommu.c | 13 + > 3 files changed, 6 insertions(+), 35 deletions(-) I think this is the right thing to do. Alex is working on fixing VFIO and while kvm is still racy using follow pte, I think they are working on it too? Jason