| Message ID | 20201009075934.3509076-9-daniel.vetter@ffwll.ch |
|---|---|
| State | Superseded |
| Headers | show |
| Series | follow_pfn and other iomap races | expand |
Hi Daniel, freshly back from my vacation I've just taken a look at your patch. First thanks for this fix and the detailed commit description. Definitely makes sense to fix this and you can add my Acked-by: Niklas Schnelle <schnelle@linux.ibm.com> Content wise it all looks sane and clear and since Gerald did the testing, I would have applied it to our tree already, but I got some trivial checkpatch violations that probably apply to the whole series. I've commented them inline below. If you confirm there I can do the fixups when applying or you can resend. On 10/9/20 9:59 AM, Daniel Vetter wrote: > Way back it was a reasonable assumptions that iomem mappings never > change the pfn range they point at. But this has changed: > > - gpu drivers dynamically manage their memory nowadays, invalidating > ptes with unmap_mapping_range when buffers get moved > > - contiguous dma allocations have moved from dedicated carvetouts to > cma regions. This means if we miss the unmap the pfn might contain > pagecache or anon memory (well anything allocated with GFP_MOVEABLE) > > - even /dev/mem now invalidates mappings when the kernel requests that > iomem region when CONFIG_IO_STRICT_DEVMEM is set, see 3234ac664a87 The above commit mention should use the format 'commit 3234ac664a87 ("/dev/mem: Revoke mappings when a driver claims the region")' otherwise this results in a checkpatch ERROR. > ("/dev/mem: Revoke mappings when a driver claims the region") > > Accessing pfns obtained from ptes without holding all the locks is > therefore no longer a good idea. Fix this. > > Since zpci_memcpy_from|toio seems to not do anything nefarious with > locks we just need to open code get_pfn and follow_pfn and make sure > we drop the locks only after we've done. The write function also needs just a typo but just saw it "we're" instead of "we've" > the copy_from_user move, since we can't take userspace faults while > holding the mmap sem. > > Reviewed-by: Gerald Schaefer <gerald.schaefer@linux.ibm.com> > No empty line after the Revied-by tag. > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> Your Signed-off-by mail address does not match the one you're sending from, this yields a checkpatch warning when using git am with your mail. This is probably just a silly misconfiguration but since Signed-offs are signatures should I change this to "Daniel Vetter <daniel.vetter@ffwll.ch>" which is the one you're sending from and also in the MAINTAINERS file? > Cc: Jason Gunthorpe <jgg@ziepe.ca> > Cc: Dan Williams <dan.j.williams@intel.com> > Cc: Kees Cook <keescook@chromium.org> > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: John Hubbard <jhubbard@nvidia.com> > Cc: Jérôme Glisse <jglisse@redhat.com> > Cc: Jan Kara <jack@suse.cz> > Cc: Dan Williams <dan.j.williams@intel.com> The above Cc: line for Dan Williams is a duplicate > Cc: linux-mm@kvack.org > Cc: linux-arm-kernel@lists.infradead.org > Cc: linux-samsung-soc@vger.kernel.org > Cc: linux-media@vger.kernel.org > Cc: Niklas Schnelle <schnelle@linux.ibm.com> > Cc: Gerald Schaefer <gerald.schaefer@linux.ibm.com> > Cc: linux-s390@vger.kernel.org > -- > v2: Move VM_IO | VM_PFNMAP checks around so they keep returning EINVAL > like before (Gerard) I think the above should go before the CC/Signed-off/Reviewev block. > --- > arch/s390/pci/pci_mmio.c | 98 +++++++++++++++++++++++----------------- > 1 file changed, 57 insertions(+), 41 deletions(-) > > diff --git a/arch/s390/pci/pci_mmio.c b/arch/s390/pci/pci_mmio.c > index 401cf670a243..1a6adbc68ee8 100644 > --- a/arch/s390/pci/pci_mmio.c > +++ b/arch/s390/pci/pci_mmio.c > @@ -119,33 +119,15 @@ static inline int __memcpy_toio_inuser(void __iomem *dst, > return rc; > } > > -static long get_pfn(unsigned long user_addr, unsigned long access, > - unsigned long *pfn) > -{ > - struct vm_area_struct *vma; > - long ret; > - > - mmap_read_lock(current->mm); > - ret = -EINVAL; > - vma = find_vma(current->mm, user_addr); > - if (!vma) > - goto out; > - ret = -EACCES; > - if (!(vma->vm_flags & access)) > - goto out; > - ret = follow_pfn(vma, user_addr, pfn); > -out: > - mmap_read_unlock(current->mm); > - return ret; > -} > - > SYSCALL_DEFINE3(s390_pci_mmio_write, unsigned long, mmio_addr, > const void __user *, user_buffer, size_t, length) > { > u8 local_buf[64]; > void __iomem *io_addr; > void *buf; > - unsigned long pfn; > + struct vm_area_struct *vma; > + pte_t *ptep; > + spinlock_t *ptl; With checkpatch.pl --strict the above yields a complained "CHECK: spinlock_t definition without comment" but I think that's really okay since your commit description is very clear. Same oin line 277. ... snip ...
... snip ... >>> Cc: linux-media@vger.kernel.org >>> Cc: Niklas Schnelle <schnelle@linux.ibm.com> >>> Cc: Gerald Schaefer <gerald.schaefer@linux.ibm.com> >>> Cc: linux-s390@vger.kernel.org >>> -- >>> v2: Move VM_IO | VM_PFNMAP checks around so they keep returning EINVAL >>> like before (Gerard) >> >> I think the above should go before the CC/Signed-off/Reviewev block. > > This is a per-subsystem bikeshed :-) drivers/gpu definitely wants it > above, but most core subsystems want it below. I'll move it. Today I learned, thanks! That said I think most of the time I've actually not seen version change information in the commit message itself only in the cover letters. I really don't care just looked odd to me. > >>> --- >>> arch/s390/pci/pci_mmio.c | 98 +++++++++++++++++++++++----------------- >>> 1 file changed, 57 insertions(+), 41 deletions(-) >>> >>> diff --git a/arch/s390/pci/pci_mmio.c b/arch/s390/pci/pci_mmio.c >>> index 401cf670a243..1a6adbc68ee8 100644 >>> --- a/arch/s390/pci/pci_mmio.c >>> +++ b/arch/s390/pci/pci_mmio.c >>> @@ -119,33 +119,15 @@ static inline int __memcpy_toio_inuser(void __iomem *dst, >>> return rc; >>> } >>> >>> -static long get_pfn(unsigned long user_addr, unsigned long access, >>> - unsigned long *pfn) >>> -{ >>> - struct vm_area_struct *vma; >>> - long ret; >>> - >>> - mmap_read_lock(current->mm); >>> - ret = -EINVAL; >>> - vma = find_vma(current->mm, user_addr); >>> - if (!vma) >>> - goto out; >>> - ret = -EACCES; >>> - if (!(vma->vm_flags & access)) >>> - goto out; >>> - ret = follow_pfn(vma, user_addr, pfn); >>> -out: >>> - mmap_read_unlock(current->mm); >>> - return ret; >>> -} >>> - >>> SYSCALL_DEFINE3(s390_pci_mmio_write, unsigned long, mmio_addr, >>> const void __user *, user_buffer, size_t, length) >>> { >>> u8 local_buf[64]; >>> void __iomem *io_addr; >>> void *buf; >>> - unsigned long pfn; >>> + struct vm_area_struct *vma; >>> + pte_t *ptep; >>> + spinlock_t *ptl; >> >> With checkpatch.pl --strict the above yields a complained >> "CHECK: spinlock_t definition without comment" but I think >> that's really okay since your commit description is very clear. >> Same oin line 277. > > I think this is a falls positive, checkpatch doesn't realize that > SYSCALL_DEFINE3 is a function, not a structure. And in a structure I'd > have added the kerneldoc or comment. Interesting, your theory sounds convincing, I too thought this was a bit too pedantic. > > I'll fix up all the nits you've found for the next round. Thanks for > taking a look. You're welcome hope I didn't sound pedantic. I think you've a lot more experience actually and this can indeed turn into bikeshedding but since I was answering anyway and most of this was checkpatch… > -Daniel >
Hi Daniel, friendly ping. I haven't seen a new version of this patch series, as I said I think your change for s390/pci is generally useful so I'm curious, are you planning on sending a new version soon? If you want you can also just sent this patch with the last few nitpicks (primarily the mail address) fixed and I'll happily apply. Best regards, Niklas Schnelle On 10/12/20 4:19 PM, Daniel Vetter wrote: > On Mon, Oct 12, 2020 at 04:03:28PM +0200, Niklas Schnelle wrote: ... snip .... >>> Cc: Jason Gunthorpe <jgg@ziepe.ca> >>> Cc: Dan Williams <dan.j.williams@intel.com> >>> Cc: Kees Cook <keescook@chromium.org> >>> Cc: Andrew Morton <akpm@linux-foundation.org> >>> Cc: John Hubbard <jhubbard@nvidia.com> >>> Cc: Jérôme Glisse <jglisse@redhat.com> >>> Cc: Jan Kara <jack@suse.cz> >>> Cc: Dan Williams <dan.j.williams@intel.com> >> >> The above Cc: line for Dan Williams is a duplicate >> >>> Cc: linux-mm@kvack.org >>> Cc: linux-arm-kernel@lists.infradead.org >>> Cc: linux-samsung-soc@vger.kernel.org >>> Cc: linux-media@vger.kernel.org >>> Cc: Niklas Schnelle <schnelle@linux.ibm.com> >>> Cc: Gerald Schaefer <gerald.schaefer@linux.ibm.com> >>> Cc: linux-s390@vger.kernel.org >>> -- >>> v2: Move VM_IO | VM_PFNMAP checks around so they keep returning EINVAL >>> like before (Gerard) >> >> I think the above should go before the CC/Signed-off/Reviewev block. > > This is a per-subsystem bikeshed :-) drivers/gpu definitely wants it > above, but most core subsystems want it below. I'll move it. > >>> --- >>> arch/s390/pci/pci_mmio.c | 98 +++++++++++++++++++++++----------------- >>> 1 file changed, 57 insertions(+), 41 deletions(-) >>> >>> diff --git a/arch/s390/pci/pci_mmio.c b/arch/s390/pci/pci_mmio.c >>> index 401cf670a243..1a6adbc68ee8 100644 >>> --- a/arch/s390/pci/pci_mmio.c >>> +++ b/arch/s390/pci/pci_mmio.c >>> @@ -119,33 +119,15 @@ static inline int __memcpy_toio_inuser(void __iomem *dst, >>> return rc; >>> } >>> >>> -static long get_pfn(unsigned long user_addr, unsigned long access, >>> - unsigned long *pfn) >>> -{ >>> - struct vm_area_struct *vma; >>> - long ret; >>> - >>> - mmap_read_lock(current->mm); >>> - ret = -EINVAL; >>> - vma = find_vma(current->mm, user_addr); >>> - if (!vma) >>> - goto out; >>> - ret = -EACCES; >>> - if (!(vma->vm_flags & access)) >>> - goto out; >>> - ret = follow_pfn(vma, user_addr, pfn); >>> -out: >>> - mmap_read_unlock(current->mm); >>> - return ret; >>> -} >>> - >>> SYSCALL_DEFINE3(s390_pci_mmio_write, unsigned long, mmio_addr, >>> const void __user *, user_buffer, size_t, length) >>> { >>> u8 local_buf[64]; >>> void __iomem *io_addr; >>> void *buf; >>> - unsigned long pfn; >>> + struct vm_area_struct *vma; >>> + pte_t *ptep; >>> + spinlock_t *ptl; >> >> With checkpatch.pl --strict the above yields a complained >> "CHECK: spinlock_t definition without comment" but I think >> that's really okay since your commit description is very clear. >> Same oin line 277. > > I think this is a falls positive, checkpatch doesn't realize that > SYSCALL_DEFINE3 is a function, not a structure. And in a structure I'd > have added the kerneldoc or comment. > > I'll fix up all the nits you've found for the next round. Thanks for > taking a look. > -Daniel >
On Wed, Oct 21, 2020 at 09:55:57AM +0200, Niklas Schnelle wrote: > Hi Daniel, > > friendly ping. I haven't seen a new version of this patch series, > as I said I think your change for s390/pci is generally useful so > I'm curious, are you planning on sending a new version soon? > If you want you can also just sent this patch with the last few > nitpicks (primarily the mail address) fixed and I'll happily apply. (I think this was stuck somewhere in moderation, only showed up just now) I was waiting for the testing result for the habana driver from Oded, but I guess Oded was waiting for v3. Hence the delay. Cheers, Daniel > > Best regards, > Niklas Schnelle > > On 10/12/20 4:19 PM, Daniel Vetter wrote: > > On Mon, Oct 12, 2020 at 04:03:28PM +0200, Niklas Schnelle wrote: > ... snip .... > >>> Cc: Jason Gunthorpe <jgg@ziepe.ca> > >>> Cc: Dan Williams <dan.j.williams@intel.com> > >>> Cc: Kees Cook <keescook@chromium.org> > >>> Cc: Andrew Morton <akpm@linux-foundation.org> > >>> Cc: John Hubbard <jhubbard@nvidia.com> > >>> Cc: Jérôme Glisse <jglisse@redhat.com> > >>> Cc: Jan Kara <jack@suse.cz> > >>> Cc: Dan Williams <dan.j.williams@intel.com> > >> > >> The above Cc: line for Dan Williams is a duplicate > >> > >>> Cc: linux-mm@kvack.org > >>> Cc: linux-arm-kernel@lists.infradead.org > >>> Cc: linux-samsung-soc@vger.kernel.org > >>> Cc: linux-media@vger.kernel.org > >>> Cc: Niklas Schnelle <schnelle@linux.ibm.com> > >>> Cc: Gerald Schaefer <gerald.schaefer@linux.ibm.com> > >>> Cc: linux-s390@vger.kernel.org > >>> -- > >>> v2: Move VM_IO | VM_PFNMAP checks around so they keep returning EINVAL > >>> like before (Gerard) > >> > >> I think the above should go before the CC/Signed-off/Reviewev block. > > > > This is a per-subsystem bikeshed :-) drivers/gpu definitely wants it > > above, but most core subsystems want it below. I'll move it. > > > >>> --- > >>> arch/s390/pci/pci_mmio.c | 98 +++++++++++++++++++++++----------------- > >>> 1 file changed, 57 insertions(+), 41 deletions(-) > >>> > >>> diff --git a/arch/s390/pci/pci_mmio.c b/arch/s390/pci/pci_mmio.c > >>> index 401cf670a243..1a6adbc68ee8 100644 > >>> --- a/arch/s390/pci/pci_mmio.c > >>> +++ b/arch/s390/pci/pci_mmio.c > >>> @@ -119,33 +119,15 @@ static inline int __memcpy_toio_inuser(void __iomem *dst, > >>> return rc; > >>> } > >>> > >>> -static long get_pfn(unsigned long user_addr, unsigned long access, > >>> - unsigned long *pfn) > >>> -{ > >>> - struct vm_area_struct *vma; > >>> - long ret; > >>> - > >>> - mmap_read_lock(current->mm); > >>> - ret = -EINVAL; > >>> - vma = find_vma(current->mm, user_addr); > >>> - if (!vma) > >>> - goto out; > >>> - ret = -EACCES; > >>> - if (!(vma->vm_flags & access)) > >>> - goto out; > >>> - ret = follow_pfn(vma, user_addr, pfn); > >>> -out: > >>> - mmap_read_unlock(current->mm); > >>> - return ret; > >>> -} > >>> - > >>> SYSCALL_DEFINE3(s390_pci_mmio_write, unsigned long, mmio_addr, > >>> const void __user *, user_buffer, size_t, length) > >>> { > >>> u8 local_buf[64]; > >>> void __iomem *io_addr; > >>> void *buf; > >>> - unsigned long pfn; > >>> + struct vm_area_struct *vma; > >>> + pte_t *ptep; > >>> + spinlock_t *ptl; > >> > >> With checkpatch.pl --strict the above yields a complained > >> "CHECK: spinlock_t definition without comment" but I think > >> that's really okay since your commit description is very clear. > >> Same oin line 277. > > > > I think this is a falls positive, checkpatch doesn't realize that > > SYSCALL_DEFINE3 is a function, not a structure. And in a structure I'd > > have added the kerneldoc or comment. > > > > I'll fix up all the nits you've found for the next round. Thanks for > > taking a look. > > -Daniel > > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
diff --git a/arch/s390/pci/pci_mmio.c b/arch/s390/pci/pci_mmio.c index 401cf670a243..1a6adbc68ee8 100644 --- a/arch/s390/pci/pci_mmio.c +++ b/arch/s390/pci/pci_mmio.c @@ -119,33 +119,15 @@ static inline int __memcpy_toio_inuser(void __iomem *dst, return rc; } -static long get_pfn(unsigned long user_addr, unsigned long access, - unsigned long *pfn) -{ - struct vm_area_struct *vma; - long ret; - - mmap_read_lock(current->mm); - ret = -EINVAL; - vma = find_vma(current->mm, user_addr); - if (!vma) - goto out; - ret = -EACCES; - if (!(vma->vm_flags & access)) - goto out; - ret = follow_pfn(vma, user_addr, pfn); -out: - mmap_read_unlock(current->mm); - return ret; -} - SYSCALL_DEFINE3(s390_pci_mmio_write, unsigned long, mmio_addr, const void __user *, user_buffer, size_t, length) { u8 local_buf[64]; void __iomem *io_addr; void *buf; - unsigned long pfn; + struct vm_area_struct *vma; + pte_t *ptep; + spinlock_t *ptl; long ret; if (!zpci_is_enabled()) @@ -158,7 +140,7 @@ SYSCALL_DEFINE3(s390_pci_mmio_write, unsigned long, mmio_addr, * We only support write access to MIO capable devices if we are on * a MIO enabled system. Otherwise we would have to check for every * address if it is a special ZPCI_ADDR and would have to do - * a get_pfn() which we don't need for MIO capable devices. Currently + * a pfn lookup which we don't need for MIO capable devices. Currently * ISM devices are the only devices without MIO support and there is no * known need for accessing these from userspace. */ @@ -176,21 +158,37 @@ SYSCALL_DEFINE3(s390_pci_mmio_write, unsigned long, mmio_addr, } else buf = local_buf; - ret = get_pfn(mmio_addr, VM_WRITE, &pfn); + ret = -EFAULT; + if (copy_from_user(buf, user_buffer, length)) + goto out_free; + + mmap_read_lock(current->mm); + ret = -EINVAL; + vma = find_vma(current->mm, mmio_addr); + if (!vma) + goto out_unlock_mmap; + if (!(vma->vm_flags & (VM_IO | VM_PFNMAP))) + goto out_unlock_mmap; + ret = -EACCES; + if (!(vma->vm_flags & VM_WRITE)) + goto out_unlock_mmap; + + ret = follow_pte_pmd(vma->vm_mm, mmio_addr, NULL, &ptep, NULL, &ptl); if (ret) - goto out; - io_addr = (void __iomem *)((pfn << PAGE_SHIFT) | + goto out_unlock_mmap; + + io_addr = (void __iomem *)((pte_pfn(*ptep) << PAGE_SHIFT) | (mmio_addr & ~PAGE_MASK)); - ret = -EFAULT; if ((unsigned long) io_addr < ZPCI_IOMAP_ADDR_BASE) - goto out; - - if (copy_from_user(buf, user_buffer, length)) - goto out; + goto out_unlock_pt; ret = zpci_memcpy_toio(io_addr, buf, length); -out: +out_unlock_pt: + pte_unmap_unlock(ptep, ptl); +out_unlock_mmap: + mmap_read_unlock(current->mm); +out_free: if (buf != local_buf) kfree(buf); return ret; @@ -274,7 +272,9 @@ SYSCALL_DEFINE3(s390_pci_mmio_read, unsigned long, mmio_addr, u8 local_buf[64]; void __iomem *io_addr; void *buf; - unsigned long pfn; + struct vm_area_struct *vma; + pte_t *ptep; + spinlock_t *ptl; long ret; if (!zpci_is_enabled()) @@ -287,7 +287,7 @@ SYSCALL_DEFINE3(s390_pci_mmio_read, unsigned long, mmio_addr, * We only support read access to MIO capable devices if we are on * a MIO enabled system. Otherwise we would have to check for every * address if it is a special ZPCI_ADDR and would have to do - * a get_pfn() which we don't need for MIO capable devices. Currently + * a pfn lookup which we don't need for MIO capable devices. Currently * ISM devices are the only devices without MIO support and there is no * known need for accessing these from userspace. */ @@ -306,22 +306,38 @@ SYSCALL_DEFINE3(s390_pci_mmio_read, unsigned long, mmio_addr, buf = local_buf; } - ret = get_pfn(mmio_addr, VM_READ, &pfn); + mmap_read_lock(current->mm); + ret = -EINVAL; + vma = find_vma(current->mm, mmio_addr); + if (!vma) + goto out_unlock_mmap; + if (!(vma->vm_flags & (VM_IO | VM_PFNMAP))) + goto out_unlock_mmap; + ret = -EACCES; + if (!(vma->vm_flags & VM_WRITE)) + goto out_unlock_mmap; + + ret = follow_pte_pmd(vma->vm_mm, mmio_addr, NULL, &ptep, NULL, &ptl); if (ret) - goto out; - io_addr = (void __iomem *)((pfn << PAGE_SHIFT) | (mmio_addr & ~PAGE_MASK)); + goto out_unlock_mmap; + + io_addr = (void __iomem *)((pte_pfn(*ptep) << PAGE_SHIFT) | + (mmio_addr & ~PAGE_MASK)); if ((unsigned long) io_addr < ZPCI_IOMAP_ADDR_BASE) { ret = -EFAULT; - goto out; + goto out_unlock_pt; } ret = zpci_memcpy_fromio(buf, io_addr, length); - if (ret) - goto out; - if (copy_to_user(user_buffer, buf, length)) + +out_unlock_pt: + pte_unmap_unlock(ptep, ptl); +out_unlock_mmap: + mmap_read_unlock(current->mm); + + if (!ret && copy_to_user(user_buffer, buf, length)) ret = -EFAULT; -out: if (buf != local_buf) kfree(buf); return ret;
Way back it was a reasonable assumptions that iomem mappings never change the pfn range they point at. But this has changed: - gpu drivers dynamically manage their memory nowadays, invalidating ptes with unmap_mapping_range when buffers get moved - contiguous dma allocations have moved from dedicated carvetouts to cma regions. This means if we miss the unmap the pfn might contain pagecache or anon memory (well anything allocated with GFP_MOVEABLE) - even /dev/mem now invalidates mappings when the kernel requests that iomem region when CONFIG_IO_STRICT_DEVMEM is set, see 3234ac664a87 ("/dev/mem: Revoke mappings when a driver claims the region") Accessing pfns obtained from ptes without holding all the locks is therefore no longer a good idea. Fix this. Since zpci_memcpy_from|toio seems to not do anything nefarious with locks we just need to open code get_pfn and follow_pfn and make sure we drop the locks only after we've done. The write function also needs the copy_from_user move, since we can't take userspace faults while holding the mmap sem. Reviewed-by: Gerald Schaefer <gerald.schaefer@linux.ibm.com> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> Cc: Jason Gunthorpe <jgg@ziepe.ca> Cc: Dan Williams <dan.j.williams@intel.com> Cc: Kees Cook <keescook@chromium.org> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: John Hubbard <jhubbard@nvidia.com> Cc: Jérôme Glisse <jglisse@redhat.com> Cc: Jan Kara <jack@suse.cz> Cc: Dan Williams <dan.j.williams@intel.com> Cc: linux-mm@kvack.org Cc: linux-arm-kernel@lists.infradead.org Cc: linux-samsung-soc@vger.kernel.org Cc: linux-media@vger.kernel.org Cc: Niklas Schnelle <schnelle@linux.ibm.com> Cc: Gerald Schaefer <gerald.schaefer@linux.ibm.com> Cc: linux-s390@vger.kernel.org -- v2: Move VM_IO | VM_PFNMAP checks around so they keep returning EINVAL like before (Gerard) --- arch/s390/pci/pci_mmio.c | 98 +++++++++++++++++++++++----------------- 1 file changed, 57 insertions(+), 41 deletions(-)