On Mon, Oct 12, 2020 at 04:03:28PM +0200, Niklas Schnelle wrote:
> 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@xxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxx>
> >
> No empty line after the Revied-by tag.
>
> > Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx>
>
> 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@xxxxxxxx>" which is the one you're
> sending from and also in the MAINTAINERS file?
>
>
> > Cc: Jason Gunthorpe <jgg@xxxxxxxx>
> > Cc: Dan Williams <dan.j.williams@xxxxxxxxx>
> > Cc: Kees Cook <keescook@xxxxxxxxxxxx>
> > Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> > Cc: John Hubbard <jhubbard@xxxxxxxxxx>
> > Cc: Jérôme Glisse <jglisse@xxxxxxxxxx>
> > Cc: Jan Kara <jack@xxxxxxx>
> > Cc: Dan Williams <dan.j.williams@xxxxxxxxx>
>
> The above Cc: line for Dan Williams is a duplicate
>
> > Cc: linux-mm@xxxxxxxxx
> > Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> > Cc: linux-samsung-soc@xxxxxxxxxxxxxxx
> > Cc: linux-media@xxxxxxxxxxxxxxx
> > Cc: Niklas Schnelle <schnelle@xxxxxxxxxxxxx>
> > Cc: Gerald Schaefer <gerald.schaefer@xxxxxxxxxxxxx>
> > Cc: linux-s390@xxxxxxxxxxxxxxx
> > --
> > 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
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
