[RFC PATCH v3 0/2] Fix procfs PCI resources mmap

[RFC PATCH v3 0/2] Fix procfs PCI resources mmap

[Lorenzo Pieralisi]

The way PCI memory resources are exported to user space through procfs
is not uniform across architectures. In particular, some architectures
(ie SPARC) export the resource PCI bus address to user space, whereas
others (ARM, PowerPC, Microblaze) export the resource CPU physical address.
This convention should be followed when it comes to passing the pgoff
values to the mmap syscall to map the resource in question.

Consequently, the checks applied to the offset passed to the mmap syscall
(in pci_mmap_fits()) are to be interpreted differently on different
architectures, and in particular they should match the values exported to
user space through the pci_resource_to_user() conversion function.

This patch series addresses two issues. First patch applies the
pci_resource_to_user() filter to the PCI resource that is being
mapped in order to carry out a proper check against the pgoff passed
from user space. Second patch fixes the way the pgoff is handled in
the ARM pci_mmap_page_range() implementation.

v2 posting:

http://marc.info/?l=linux-kernel&m=141416813409829&w=2

v2 => v3
- Reworded commit log in order to try to bisect the breakage
  and provide insights into the commit history that led to the
  current interface

v1 => v2
- Reworded commit log as per RMK comments

Lorenzo Pieralisi (2):
  drivers: pci: fix pci_mmap_fits() implementation for procfs mmap
  arm: kernel: fix pci_mmap_page_range() offset calculation

 arch/arm/kernel/bios32.c | 10 ++--------
 drivers/pci/pci-sysfs.c  | 13 ++++++++-----
 2 files changed, 10 insertions(+), 13 deletions(-)

-- 
2.1.2

[RFC PATCH v3 1/2] drivers: pci: fix pci_mmap_fits() implementation for procfs mmap

[Lorenzo Pieralisi]

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 <arnd@arndb.de>
Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: David S. Miller <davem@davemloft.net>
Cc: Michal Simek <monstr@monstr.eu>
Cc: Martin Wilck <martin.wilck@ts.fujitsu.com>
Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
---
 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

[RFC PATCH v3 2/2] arm: kernel: fix pci_mmap_page_range() offset calculation

[Lorenzo Pieralisi]

The pci_mmap_page_range() API should be written to expect offset
values representing PCI memory resource addresses as seen by user
space, through the pci_resource_to_user() API.

ARM relies on the standard implementation of pci_resource_to_user()
which actually is an identity map and exports to user space
PCI memory resources as they are stored in PCI devices resources
structures, which represent CPU physical addresses (fixed-up using
BUS to CPU address conversions) not PCI bus addresses.

Therefore, on ARM platforms where the mapping between CPU and BUS
address is not a 1:1 the current pci_mmap_page_range() implementation is
erroneous, in that an additional shift is applied to an already fixed-up
offset passed from userspace.

Hence, this patch removes the mem_offset from the pgoff calculation
since the offset as passed from user space already represents the CPU
physical address corresponding to the resource to be mapped, ie no
additional offset should be applied.

Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Russell King <linux@arm.linux.org.uk>
Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
---
 arch/arm/kernel/bios32.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
index 17a26c1..b56fa2d 100644
--- a/arch/arm/kernel/bios32.c
+++ b/arch/arm/kernel/bios32.c
@@ -626,21 +626,15 @@ int pcibios_enable_device(struct pci_dev *dev, int mask)
 int pci_mmap_page_range(struct pci_dev *dev, struct vm_area_struct *vma,
 			enum pci_mmap_state mmap_state, int write_combine)
 {
-	struct pci_sys_data *root = dev->sysdata;
-	unsigned long phys;
-
-	if (mmap_state == pci_mmap_io) {
+	if (mmap_state == pci_mmap_io)
 		return -EINVAL;
-	} else {
-		phys = vma->vm_pgoff + (root->mem_offset >> PAGE_SHIFT);
-	}
 
 	/*
 	 * Mark this as IO
 	 */
 	vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
 
-	if (remap_pfn_range(vma, vma->vm_start, phys,
+	if (remap_pfn_range(vma, vma->vm_start, vma->vm_pgoff,
 			     vma->vm_end - vma->vm_start,
 			     vma->vm_page_prot))
 		return -EAGAIN;
-- 
2.1.2

Re: [RFC PATCH v3 0/2] Fix procfs PCI resources mmap

[Lorenzo Pieralisi]

On Thu, Nov 13, 2014 at 11:19:14AM +0000, Lorenzo Pieralisi wrote:
> The way PCI memory resources are exported to user space through procfs
> is not uniform across architectures. In particular, some architectures
> (ie SPARC) export the resource PCI bus address to user space, whereas
> others (ARM, PowerPC, Microblaze) export the resource CPU physical address.
> This convention should be followed when it comes to passing the pgoff
> values to the mmap syscall to map the resource in question.
> 
> Consequently, the checks applied to the offset passed to the mmap syscall
> (in pci_mmap_fits()) are to be interpreted differently on different
> architectures, and in particular they should match the values exported to
> user space through the pci_resource_to_user() conversion function.
> 
> This patch series addresses two issues. First patch applies the
> pci_resource_to_user() filter to the PCI resource that is being
> mapped in order to carry out a proper check against the pgoff passed
> from user space. Second patch fixes the way the pgoff is handled in
> the ARM pci_mmap_page_range() implementation.
> 
> v2 posting:
> 
> http://marc.info/?l=linux-kernel&m=141416813409829&w=2
> 
> v2 => v3
> - Reworded commit log in order to try to bisect the breakage
>   and provide insights into the commit history that led to the
>   current interface
> 
> v1 => v2
> - Reworded commit log as per RMK comments

Any further comments on this patch ? I would like to send patch 2 to
RMK patch system asap and if possible get an agreement on patch 1.

Thank you,
Lorenzo

> Lorenzo Pieralisi (2):
>   drivers: pci: fix pci_mmap_fits() implementation for procfs mmap
>   arm: kernel: fix pci_mmap_page_range() offset calculation
> 
>  arch/arm/kernel/bios32.c | 10 ++--------
>  drivers/pci/pci-sysfs.c  | 13 ++++++++-----
>  2 files changed, 10 insertions(+), 13 deletions(-)
> 
> -- 
> 2.1.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [RFC PATCH v3 1/2] drivers: pci: fix pci_mmap_fits() implementation for procfs mmap

[Bjorn Helgaas]

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 <arnd@arndb.de>
> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Russell King <linux@arm.linux.org.uk>
> Cc: David S. Miller <davem@davemloft.net>
> Cc: Michal Simek <monstr@monstr.eu>
> Cc: Martin Wilck <martin.wilck@ts.fujitsu.com>
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>

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.

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
> 

Re: [RFC PATCH v3 1/2] drivers: pci: fix pci_mmap_fits() implementation for procfs mmap

[Lorenzo Pieralisi]

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 <arnd@arndb.de>
> > Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > Cc: Russell King <linux@arm.linux.org.uk>
> > Cc: David S. Miller <davem@davemloft.net>
> > Cc: Michal Simek <monstr@monstr.eu>
> > Cc: Martin Wilck <martin.wilck@ts.fujitsu.com>
> > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> 
> 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.

I think that in doing that you might end up breaking at least powerpc
and microblaze proc and sys interfaces, but I agree with the process.

> 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 understand that, it holds back patch 2, but it is fine since it can
get in as a fix even if it misses the merge window, as long as you
agree with the overall approach.

Thank you,
Lorenzo

Re: [RFC PATCH v3 1/2] drivers: pci: fix pci_mmap_fits() implementation for procfs mmap

[Benjamin Herrenschmidt]

On Fri, 2014-11-21 at 10:51 -0700, Bjorn Helgaas wrote:
> 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 remember all sort of funky breakage in that area a long time ago
but I can't quite remember the details and how I ended up with the
current code :-(

The one thing to keep in mind is that there was always confusion in
the userspace users of the /proc interface as to what offset they
had to use... the BAR value or something translated via /proc/.../resources

(Normally the latter but a lot of stuff got it wrong ...)

Cheers,
Ben.

Re: [RFC PATCH v3 1/2] drivers: pci: fix pci_mmap_fits() implementation for procfs mmap

[Lorenzo Pieralisi]

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 <arnd@arndb.de>
> > Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > Cc: Russell King <linux@arm.linux.org.uk>
> > Cc: David S. Miller <davem@davemloft.net>
> > Cc: Michal Simek <monstr@monstr.eu>
> > Cc: Martin Wilck <martin.wilck@ts.fujitsu.com>
> > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> 
> 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/
> 

Re: [RFC PATCH v3 1/2] drivers: pci: fix pci_mmap_fits() implementation for procfs mmap

[Bjorn Helgaas]

On Wed, Jan 21, 2015 at 06:44:56PM +0000, Lorenzo Pieralisi wrote:
> 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 <arnd@arndb.de>
> > > Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> > > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > > Cc: Russell King <linux@arm.linux.org.uk>
> > > Cc: David S. Miller <davem@davemloft.net>
> > > Cc: Michal Simek <monstr@monstr.eu>
> > > Cc: Martin Wilck <martin.wilck@ts.fujitsu.com>
> > > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > 
> > 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.

I haven't had a chance to do anything with these, sorry.  The second patch
only touches ARM, and I think it's fine to merge that.  I'll ack it in case
that's useful to you.

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

Re: [RFC PATCH v3 2/2] arm: kernel: fix pci_mmap_page_range() offset calculation

[Bjorn Helgaas]

On Thu, Nov 13, 2014 at 11:19:16AM +0000, Lorenzo Pieralisi wrote:
> The pci_mmap_page_range() API should be written to expect offset
> values representing PCI memory resource addresses as seen by user
> space, through the pci_resource_to_user() API.
> 
> ARM relies on the standard implementation of pci_resource_to_user()
> which actually is an identity map and exports to user space
> PCI memory resources as they are stored in PCI devices resources
> structures, which represent CPU physical addresses (fixed-up using
> BUS to CPU address conversions) not PCI bus addresses.
> 
> Therefore, on ARM platforms where the mapping between CPU and BUS
> address is not a 1:1 the current pci_mmap_page_range() implementation is
> erroneous, in that an additional shift is applied to an already fixed-up
> offset passed from userspace.
> 
> Hence, this patch removes the mem_offset from the pgoff calculation
> since the offset as passed from user space already represents the CPU
> physical address corresponding to the resource to be mapped, ie no
> additional offset should be applied.
> 
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Russell King <linux@arm.linux.org.uk>
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

> ---
>  arch/arm/kernel/bios32.c | 10 ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
> index 17a26c1..b56fa2d 100644
> --- a/arch/arm/kernel/bios32.c
> +++ b/arch/arm/kernel/bios32.c
> @@ -626,21 +626,15 @@ int pcibios_enable_device(struct pci_dev *dev, int mask)
>  int pci_mmap_page_range(struct pci_dev *dev, struct vm_area_struct *vma,
>  			enum pci_mmap_state mmap_state, int write_combine)
>  {
> -	struct pci_sys_data *root = dev->sysdata;
> -	unsigned long phys;
> -
> -	if (mmap_state == pci_mmap_io) {
> +	if (mmap_state == pci_mmap_io)
>  		return -EINVAL;
> -	} else {
> -		phys = vma->vm_pgoff + (root->mem_offset >> PAGE_SHIFT);
> -	}
>  
>  	/*
>  	 * Mark this as IO
>  	 */
>  	vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
>  
> -	if (remap_pfn_range(vma, vma->vm_start, phys,
> +	if (remap_pfn_range(vma, vma->vm_start, vma->vm_pgoff,
>  			     vma->vm_end - vma->vm_start,
>  			     vma->vm_page_prot))
>  		return -EAGAIN;
> -- 
> 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/

Re: [RFC PATCH v3 2/2] arm: kernel: fix pci_mmap_page_range() offset calculation

[Lorenzo Pieralisi]

On Fri, Jan 23, 2015 at 11:59:11PM +0000, Bjorn Helgaas wrote:
> On Thu, Nov 13, 2014 at 11:19:16AM +0000, Lorenzo Pieralisi wrote:
> > The pci_mmap_page_range() API should be written to expect offset
> > values representing PCI memory resource addresses as seen by user
> > space, through the pci_resource_to_user() API.
> > 
> > ARM relies on the standard implementation of pci_resource_to_user()
> > which actually is an identity map and exports to user space
> > PCI memory resources as they are stored in PCI devices resources
> > structures, which represent CPU physical addresses (fixed-up using
> > BUS to CPU address conversions) not PCI bus addresses.
> > 
> > Therefore, on ARM platforms where the mapping between CPU and BUS
> > address is not a 1:1 the current pci_mmap_page_range() implementation is
> > erroneous, in that an additional shift is applied to an already fixed-up
> > offset passed from userspace.
> > 
> > Hence, this patch removes the mem_offset from the pgoff calculation
> > since the offset as passed from user space already represents the CPU
> > physical address corresponding to the resource to be mapped, ie no
> > additional offset should be applied.
> > 
> > Cc: Arnd Bergmann <arnd@arndb.de>
> > Cc: Russell King <linux@arm.linux.org.uk>
> > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> 
> Acked-by: Bjorn Helgaas <bhelgaas@google.com>

Thank you, I will ask Russell to merge it.

Lorenzo