Re: [PATCH v2 08/17] s390/pci: Remove races against pte updates

[Niklas Schnelle]

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.

> ---
>  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 ...