From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-9.5 required=3.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id E745DC4363D for ; Mon, 12 Oct 2020 14:19:13 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 418E92076E for ; Mon, 12 Oct 2020 14:19:13 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="signature verification failed" (1024-bit key) header.d=ffwll.ch header.i=@ffwll.ch header.b="b2jM2N7c" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 418E92076E Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=ffwll.ch Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id A29D0900004; Mon, 12 Oct 2020 10:19:12 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 9D980900002; Mon, 12 Oct 2020 10:19:12 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 8A230900004; Mon, 12 Oct 2020 10:19:12 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0091.hostedemail.com [216.40.44.91]) by kanga.kvack.org (Postfix) with ESMTP id 5E982900002 for ; Mon, 12 Oct 2020 10:19:12 -0400 (EDT) Received: from smtpin04.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay04.hostedemail.com (Postfix) with ESMTP id E4BBB1EE6 for ; Mon, 12 Oct 2020 14:19:11 +0000 (UTC) X-FDA: 77363480502.04.grass27_270e9f1271fb Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin04.hostedemail.com (Postfix) with ESMTP id C27D8800575D for ; Mon, 12 Oct 2020 14:19:11 +0000 (UTC) X-HE-Tag: grass27_270e9f1271fb X-Filterd-Recvd-Size: 9943 Received: from mail-wm1-f66.google.com (mail-wm1-f66.google.com [209.85.128.66]) by imf46.hostedemail.com (Postfix) with ESMTP for ; Mon, 12 Oct 2020 14:19:11 +0000 (UTC) Received: by mail-wm1-f66.google.com with SMTP id a72so6723378wme.5 for ; Mon, 12 Oct 2020 07:19:10 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ffwll.ch; s=google; h=date:from:to:cc:subject:message-id:mail-followup-to:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to; bh=SuTUuPoSnGpk/kjzh2rwLKMK0NV61T2wKUHxGWe1zb8=; b=b2jM2N7c7mPZ8EQ74nK4as+DJOk3QIe1TTYSZ7oaXJ3365OVv4YcNg0qxxuCOPCxWm vCNnR/PtgLnbnCWWMkgiKiOIBral0E0p/MHJyNO76dWYwluCVeZIJCQEYXYML0RprK+3 MPKg0NsB7G5eAb0oAe3c/PxZvmGkrVc0l8J4g= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id :mail-followup-to:references:mime-version:content-disposition :content-transfer-encoding:in-reply-to; bh=SuTUuPoSnGpk/kjzh2rwLKMK0NV61T2wKUHxGWe1zb8=; b=qYZ1QoqVR+9rm5fJAirxNGN36UW84CcTgvwzAZ+/bwGDGyCCHzfLYQE2AppezuCfHI wHOL/KSfQSSqzdzwIeFfeACfwYmIA9DY3aJsrSGZoInicyHLw8g2oWkMvj26gpgxh2/a puKb2CPgkSUgu91jsAy3FSZSyAsoOrtsmYRSsHJCRhl1krsUbiAc7DwDEaQGa6rZR3Mg xXd+riwoywXtXpj3xNX8HJAdPt7fZdL2KR+Hk6v2JyOAafNLLhD7ThLyWB9wVlhhbGVD WFyo82yLYAoJ3B/y6n9SrrZv6Ulxso9OXpM5GSPOKtxYI44xldPUqp+n/XkTfSIesrsi Cx0g== X-Gm-Message-State: AOAM533ZoGMBRIi1i/inJuRrtm288qAo2g8z4TDAkUGwdj4g+koS/Muo AJwnJ+WG4T1pknlFRsHXpIq+jw== X-Google-Smtp-Source: ABdhPJzGnqdPcnQQAOL/8bt/9LG14tDuHDf/7FLmVEbsTsUaPr2zgpHozg5qRzqt8PtLD2kO4LhHrw== X-Received: by 2002:a1c:5685:: with SMTP id k127mr11456739wmb.135.1602512349502; Mon, 12 Oct 2020 07:19:09 -0700 (PDT) Received: from phenom.ffwll.local ([2a02:168:57f4:0:efd0:b9e5:5ae6:c2fa]) by smtp.gmail.com with ESMTPSA id z127sm23493365wmc.2.2020.10.12.07.19.07 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 12 Oct 2020 07:19:08 -0700 (PDT) Date: Mon, 12 Oct 2020 16:19:06 +0200 From: Daniel Vetter To: Niklas Schnelle Cc: Daniel Vetter , DRI Development , LKML , kvm@vger.kernel.org, linux-mm@kvack.org, linux-arm-kernel@lists.infradead.org, linux-samsung-soc@vger.kernel.org, linux-media@vger.kernel.org, linux-s390@vger.kernel.org, Gerald Schaefer , Daniel Vetter , Jason Gunthorpe , Dan Williams , Kees Cook , Andrew Morton , John Hubbard , =?iso-8859-1?B?Suly9G1l?= Glisse , Jan Kara Subject: Re: [PATCH v2 08/17] s390/pci: Remove races against pte updates Message-ID: <20201012141906.GX438822@phenom.ffwll.local> Mail-Followup-To: Niklas Schnelle , DRI Development , LKML , kvm@vger.kernel.org, linux-mm@kvack.org, linux-arm-kernel@lists.infradead.org, linux-samsung-soc@vger.kernel.org, linux-media@vger.kernel.org, linux-s390@vger.kernel.org, Gerald Schaefer , Daniel Vetter , Jason Gunthorpe , Dan Williams , Kees Cook , Andrew Morton , John Hubbard , =?iso-8859-1?B?Suly9G1l?= Glisse , Jan Kara References: <20201009075934.3509076-1-daniel.vetter@ffwll.ch> <20201009075934.3509076-9-daniel.vetter@ffwll.ch> <6deb08dd-46f3-bf26-5362-fdc696f6fd74@linux.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <6deb08dd-46f3-bf26-5362-fdc696f6fd74@linux.ibm.com> X-Operating-System: Linux phenom 5.7.0-1-amd64 Content-Transfer-Encoding: quoted-printable X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Mon, Oct 12, 2020 at 04:03:28PM +0200, Niklas Schnelle wrote: > Hi Daniel, >=20 > 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 >=20 > Acked-by: Niklas Schnelle >=20 > Content wise it all looks sane and clear and since Gerald did the testi= ng, > 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 resen= d. >=20 > 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: > >=20 > > - gpu drivers dynamically manage their memory nowadays, invalidating > > ptes with unmap_mapping_range when buffers get moved > >=20 > > - 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) > >=20 > > - even /dev/mem now invalidates mappings when the kernel requests tha= t > > iomem region when CONFIG_IO_STRICT_DEVMEM is set, see 3234ac664a87 >=20 > The above commit mention should use the format > 'commit 3234ac664a87 ("/dev/mem: Revoke mappings when a driver claims t= he region")' > otherwise this results in a checkpatch ERROR. >=20 > > ("/dev/mem: Revoke mappings when a driver claims the region") > >=20 > > Accessing pfns obtained from ptes without holding all the locks is > > therefore no longer a good idea. Fix this. > >=20 > > 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 need= s >=20 > just a typo but just saw it "we're" instead of "we've" >=20 > > the copy_from_user move, since we can't take userspace faults while > > holding the mmap sem. > >=20 > > Reviewed-by: Gerald Schaefer > >=20 > No empty line after the Revied-by tag. >=20 > > Signed-off-by: Daniel Vetter >=20 > Your Signed-off-by mail address does not match the one you're sending f= rom, > 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=20 > "Daniel Vetter " which is the one you're > sending from and also in the MAINTAINERS file? >=20 >=20 > > Cc: Jason Gunthorpe > > Cc: Dan Williams > > Cc: Kees Cook > > Cc: Andrew Morton > > Cc: John Hubbard > > Cc: J=E9r=F4me Glisse > > Cc: Jan Kara > > Cc: Dan Williams >=20 > The above Cc: line for Dan Williams is a duplicate >=20 > > 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 > > Cc: Gerald Schaefer > > Cc: linux-s390@vger.kernel.org > > -- > > v2: Move VM_IO | VM_PFNMAP checks around so they keep returning EINVA= L > > like before (Gerard) >=20 > 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(-) > >=20 > > 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 __i= omem *dst, > > return rc; > > } > > =20 > > -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 =3D -EINVAL; > > - vma =3D find_vma(current->mm, user_addr); > > - if (!vma) > > - goto out; > > - ret =3D -EACCES; > > - if (!(vma->vm_flags & access)) > > - goto out; > > - ret =3D 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; >=20 > 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 --=20 Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch