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.8 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable 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 95E6FC43457 for ; Mon, 12 Oct 2020 14:20:48 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 2F45E2076E for ; Mon, 12 Oct 2020 14:20:48 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="IDfKj8O6"; 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 2F45E2076E Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=ffwll.ch Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References:Message-ID: Subject:To:From:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=3Ouv0EPY/Z4HqszVC1qLV0dhh+RdkhtvLV2N00y4XrQ=; b=IDfKj8O6Eq3oyk1rxNJ0U0xXT EIoq+RdMVTZyPqTWK+P1STMXTFGsvK5jbvjqFn3CSlrDHiUdcaB3AKx8cFax0mJbWeKg4e0tGl03v eQOCEc4uCOo8mKjNvXcbA2NQUm5JmdtDMry07ICTP6QZkmaEjUjRchXLGwIh1sj2aDz0kboaXCSOs eW/Ie9vLonDbzDlsWWqFlOVDng6lQ3xsuzQZwHCDAxRoNX+Yekp33cNwHmyDfSqrgLM/XNAI57Rz7 6usx6KKBvTK6faZbm7qbwC/TP/bMl8NOb5pRqtic6jXfoGHJAgBnMoO3MmMk7EmDPNKZDad3HE/7e wMXO9foIA==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kRyfT-0000Uy-3O; Mon, 12 Oct 2020 14:19:15 +0000 Received: from mail-wm1-x344.google.com ([2a00:1450:4864:20::344]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1kRyfQ-0000TM-AW for linux-arm-kernel@lists.infradead.org; Mon, 12 Oct 2020 14:19:13 +0000 Received: by mail-wm1-x344.google.com with SMTP id z22so13876005wmi.0 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=QoOzB9rf9pUoITcPSp09MYrjKBQoxbvQVVrhPpIFFNvKliODVZWbe1hKulB0IUyXuG S5UvTwkMoMbXsL4Qfewr6CtbdPvgDinuG5SYRfkhiZX8y0qfyivb2Kl648k4XXTAyZE3 D3lZeu8pBHsPRzzhPcnMl9rc+gFVHlD+HtccVOSR4UiTszZkdrK+KC7nBpvakpcqrMwi eaSOnr7+SWi/OXFKyMF6gSXSbNK0y12Jwm5G2KsGPNg7EehVEzE/NqaFnYtn8sCb0/oe UFYC5lDUTk+Gbm9O515vQaIPkj415SxTj8W6ng92H2IMtcZ3ciMoHtSqFIyxllPPjCEE Wdiw== X-Gm-Message-State: AOAM531MnRBwRlYmXr1QQLoXNN4lFyRRixhQjz6AUzqLklcMVEYBR7ZL V2mtbRiCuGVJpqhhrgfD0vP2RQ== 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 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-Disposition: inline In-Reply-To: <6deb08dd-46f3-bf26-5362-fdc696f6fd74@linux.ibm.com> X-Operating-System: Linux phenom 5.7.0-1-amd64 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20201012_101912_374206_5A3BDB1F X-CRM114-Status: GOOD ( 42.36 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: linux-s390@vger.kernel.org, linux-samsung-soc@vger.kernel.org, Jan Kara , Kees Cook , kvm@vger.kernel.org, Jason Gunthorpe , Daniel Vetter , LKML , DRI Development , Andrew Morton , linux-mm@kvack.org, =?iso-8859-1?B?Suly9G1l?= Glisse , John Hubbard , Daniel Vetter , Gerald Schaefer , Dan Williams , linux-arm-kernel@lists.infradead.org, linux-media@vger.kernel.org Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org 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 > = > 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 > > = > No empty line after the Revied-by tag. > = > > Signed-off-by: Daniel Vetter > = > Your Signed-off-by mail address does not match the one you're sending fro= m, > 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 " which is the one you're > sending from and also in the MAINTAINERS file? > = > = > > 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 > = > 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 > > Cc: Gerald Schaefer > > 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 __iom= em *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 =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; > = > 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel