From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753541AbbAUSpM (ORCPT ); Wed, 21 Jan 2015 13:45:12 -0500 Received: from foss-mx-na.foss.arm.com ([217.140.108.86]:40451 "EHLO foss-mx-na.foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751484AbbAUSpI (ORCPT ); Wed, 21 Jan 2015 13:45:08 -0500 Date: Wed, 21 Jan 2015 18:44:56 +0000 From: Lorenzo Pieralisi To: Bjorn Helgaas Cc: "linux-kernel@vger.kernel.org" , Arnd Bergmann , Jesse Barnes , Benjamin Herrenschmidt , Russell King , "David S. Miller" , Michal Simek , Martin Wilck , Linux PCI Subject: Re: [RFC PATCH v3 1/2] drivers: pci: fix pci_mmap_fits() implementation for procfs mmap Message-ID: <20150121184456.GA30346@red-moon> References: <1415877558-8906-1-git-send-email-lorenzo.pieralisi@arm.com> <1415877558-8906-2-git-send-email-lorenzo.pieralisi@arm.com> <20141121175114.GC6578@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20141121175114.GC6578@google.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Bjorn, On Fri, Nov 21, 2014 at 05:51:14PM +0000, Bjorn Helgaas wrote: > On Thu, Nov 13, 2014 at 11:19:15AM +0000, Lorenzo Pieralisi wrote: > > The introduction of pci_mmap_fits() in commit: > > > > b5ff7df3df9efab511244d5a299fce706c71af48 > > "Check mapped ranges on sysfs resource files" > > > > allowed to check for valid range mappings of PCI resources to user space > > when mapping PCI resources through the sysfs filesystem. > > > > The mapping of resources through the sysfs expects the offset passed > > by the user through the mmap syscall to be 0, and the pgoff is adjusted > > by the kernel to memory map the resource to the CPU physical address > > corresponding to the PCI resource in question. > > > > The usage of procfs mapping of PCI resources (/proc/bus/pci files) > > is more controversial in that userspace programs mapping PCI resources > > are expected to pass in the mmap offset field either a CPU physical address > > or a PCI bar value, depending on the architecture. > > > > By the time pci_mmap_fits() was used to check PCI resource ranges for > > procfs PCI resources mapping in commit: > > > > 9eff02e2042f96fb2aedd02e032eca1c5333d7 > > "PCI: check mmap range of /proc/bus/pci files too" > > > > the procfs interface for mmapping resources to user space broke, since > > pci_mmap_fits() expected the offset passed from user space in the mmap > > call to be 0, not the CPU physical address or PCI BAR value of the > > resource in question. > > > > Subsequent attempts at fixing the pci_mmap_fits() implementation failed > > to fix the issue (or fixed the issue in some architectures but not for > > all of them, ARM and SPARC procfs interface PCI resources mapping stayed > > broken throughout) in particular commits: > > > > 8c05cd08a7504b855c265263e84af61aabafa329 > > "PCI: fix offset check for sysfs mmapped files" > > > > and > > > > 3b519e4ea618b6943a82931630872907f9ac2c2b > > "PCI: fix size checks for mmap() on /proc/bus/pci files" > > > > fixed procfs PCI resources mapping checks in pci_mmap_fits for some > > architectures, but not for architectures like SPARC that expects > > the offset value passed from user space through the mmap syscall > > (when mapping through procfs) to represent the PCI BAR value of the > > resource to be mapped. > > > > The reason behind the breakage is the following. The addresses stored > > in PCI device resources for memory spaces correspond to CPU physical > > addresses, which do not necessarily map 1:1 to PCI bus addresses as > > programmed in PCI devices configuration spaces. > > > > This implies that the sanity checks carried out in pci_mmap_fits() to > > ensure that the user executes an mmap of a "real" pci resource are > > erroneous when executed through procfs. Some platforms (ie SPARC) > > expect the offset value to be passed in (procfs mapping) to be the > > PCI BAR configuration value as read from the PCI device configuration > > space, not the fixed-up CPU physical address that is present in PCI > > device resources. > > > > The required pgoff (offset in mmap syscall) value passed from userspace > > is supposed to represent the resource value exported through > > /proc/bus/pci/devices when the resource is mmapped though procfs (and 0 > > when the mapping is carried out through sysfs resource files), which > > corresponds to the PCI resource filtered through the pci_resource_to_user() > > API. > > > > This patch converts the PCI resource to the expected "user visible" > > value through pci_resource_to_user() before carrying out sanity checks > > in pci_mmap_fits() so that the check is carried out on the resource > > values as expected from the userspace mmap API. > > > > Cc: Arnd Bergmann > > Cc: Jesse Barnes > > Cc: Bjorn Helgaas > > Cc: Benjamin Herrenschmidt > > Cc: Russell King > > Cc: David S. Miller > > Cc: Michal Simek > > Cc: Martin Wilck > > Signed-off-by: Lorenzo Pieralisi > > Hi Lorenzo, > > I think this patch is the right thing to do. I'm going to try to write > patches for microblaze, mips, powerpc, and sparc that implement their > pci_resource_to_user() in terms of pcibios_resource_to_bus() (the patches > are easy; it's the arguments for correctness that take time). Then I'll > try to convince myself that those arches are currently broken and will be > fixed by your patch below. > > But I'll be on vacation all next week, so this will take me some time and > it may not make the next merge window. I do not know if you had time to implement the patches above, I would like to ask Russell to merge patch 2 of this series though, since (1) it is a fix in the first place given the current proc/sys interface, and (2) we need it to remove dependency on pcibios for arm64 drivers; I am just waiting to merge patch 2 upstream since in a way it depends on your decision on what the final proc/sys interface should look like and how we implement it. As things stand, patch 2 can be merged and fixes the current API on ARM. Thanks, Lorenzo > > Bjorn > > > --- > > drivers/pci/pci-sysfs.c | 13 ++++++++----- > > 1 file changed, 8 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c > > index 2c6643f..e4634e3 100644 > > --- a/drivers/pci/pci-sysfs.c > > +++ b/drivers/pci/pci-sysfs.c > > @@ -963,17 +963,20 @@ void pci_remove_legacy_files(struct pci_bus *b) > > int pci_mmap_fits(struct pci_dev *pdev, int resno, struct vm_area_struct *vma, > > enum pci_mmap_api mmap_api) > > { > > - unsigned long nr, start, size, pci_start; > > + unsigned long nr, start, size, pci_offset; > > + resource_size_t pci_start, pci_end; > > > > if (pci_resource_len(pdev, resno) == 0) > > return 0; > > nr = vma_pages(vma); > > start = vma->vm_pgoff; > > + pci_resource_to_user(pdev, resno, &pdev->resource[resno], > > + &pci_start, &pci_end); > > size = ((pci_resource_len(pdev, resno) - 1) >> PAGE_SHIFT) + 1; > > - pci_start = (mmap_api == PCI_MMAP_PROCFS) ? > > - pci_resource_start(pdev, resno) >> PAGE_SHIFT : 0; > > - if (start >= pci_start && start < pci_start + size && > > - start + nr <= pci_start + size) > > + pci_offset = (mmap_api == PCI_MMAP_PROCFS) ? > > + pci_start >> PAGE_SHIFT : 0; > > + if (start >= pci_offset && start < pci_offset + size && > > + start + nr <= pci_offset + size) > > return 1; > > return 0; > > } > > -- > > 2.1.2 > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ >