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

[PATCH RFC v2 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.

v1 posting:

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

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

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

[Lorenzo Pieralisi]

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.

Therefore, the changes applied by commits:

8c05cd08a7504b855c26526
3b519e4ea618b6943a82931

imply 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: 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 92b6d9a..777d8bc 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: [PATCH RFC v2 1/2] drivers: pci: fix pci_mmap_fits() implementation for procfs mmap

[Bjorn Helgaas]

[+cc Michael, since he merged 2311b1f2bbd3, which added
pci_resource_to_user()]

On Fri, Oct 24, 2014 at 05:28:05PM +0100, Lorenzo Pieralisi wrote:
> 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.
> 
> Therefore, the changes applied by commits:
> 
> 8c05cd08a7504b855c26526
> 3b519e4ea618b6943a82931
> 
> imply 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.

I'm trying to figure out what's going on here.  I think this fix is
correct, but it seems like there might be some additional simplification we
could do.

This patch is apparently a bug fix for mmap via procfs.  And the bug
apparently affects platforms where pci_resource_to_user() applies a
non-zero offset, i.e., microblaze, mips, power, and sparc.  It would be
helpful to have a bug report or an example of something that doesn't work.

The second patch fixes a bug on ARM.  How does that patch depend on this
one?  Since ARM doesn't implement pci_resource_to_user(), I wouldn't think
this first patch would change anything on ARM.

Here's what I think I understand so far:

  Applications can mmap PCI memory space via either sysfs or procfs (the
  procfs method is deprecated but still supported):

    - In sysfs, there's a separate /sys/devices/pci*/.../resource* file
      for each device BAR, and the application opens the appropriate
      file and supplies the offset from the beginning of the BAR as the
      mmap(2) offset.

    - In procfs, the application opens the single /proc/bus/pci/... file
      for the device.  On most platforms, it supplies the CPU physical
      address as the mmap(2) offset.  On a few platforms, such as SPARC,
      it supplies the bus address, i.e., a BAR value, instead.

But I'm not sure I have this right.  If the procfs offset is either the
CPU physical address or the BAR value, then pci_resource_to_user()
should be (depending on the arch) either a no-op or use
pci_resource_to_bus().

But that's not how it's implemented.  Maybe it *could* be?  If
pci_resource_to_user() gives you something that's not a CPU physical
address and not a bus address, what *does* it give you, and why would we
need this third kind of thing?

FWIW, I think the discussion leading up to pci_resource_to_user() is here:
http://lkml.iu.edu/hypermail/linux/kernel/0504.3/0467.html

Bjorn

> Cc: Arnd Bergmann <arnd@arndb.de>
> 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 92b6d9a..777d8bc 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: [PATCH RFC v2 1/2] drivers: pci: fix pci_mmap_fits() implementation for procfs mmap

[Lorenzo Pieralisi]

On Mon, Nov 10, 2014 at 11:04:54PM +0000, Bjorn Helgaas wrote:
> [+cc Michael, since he merged 2311b1f2bbd3, which added
> pci_resource_to_user()]
> 
> On Fri, Oct 24, 2014 at 05:28:05PM +0100, Lorenzo Pieralisi wrote:
> > 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.
> > 
> > Therefore, the changes applied by commits:
> > 
> > 8c05cd08a7504b855c26526
> > 3b519e4ea618b6943a82931
> > 
> > imply 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.
> 
> I'm trying to figure out what's going on here.  I think this fix is
> correct, but it seems like there might be some additional simplification we
> could do.
> 
> This patch is apparently a bug fix for mmap via procfs.  And the bug
> apparently affects platforms where pci_resource_to_user() applies a
> non-zero offset, i.e., microblaze, mips, power, and sparc.  It would be
> helpful to have a bug report or an example of something that doesn't work.

For ARM it is currently impossible to mmap resources to userspace on
eg integrator platform (where CPU <-> PCI bus addresses are not a 1:1
mapping).

> The second patch fixes a bug on ARM.  How does that patch depend on this
> one?  Since ARM doesn't implement pci_resource_to_user(), I wouldn't think
> this first patch would change anything on ARM.

It does not depend on patch 1, but patch 2 is a fix as long as we keep the
*current* mmap implementation (and in particular the expectations about
the offset mmap parameter), which is still uncertain given that patch
1 is just an RFC and the procfs mmap fix can be implemented differently (ie
we might decide that all arch should pass the PCI BAR value in the procfs
mmap offset parameter, if that's the case patch 2 might need to change or
become useless/wrong).

The gist of what we are discussing (and the reason why I posted this set) is
that we have to agree on the procfs and sysfs resource mmap interface, in
particular what the mmap offset parameter is meant to be.

If pci_resource_to_user() is there to make a resource value visible to
the user and that's the value that should be used as offset in the
procfs mmap call, fine by me, as long as we agree on that.

Current code is extremely confusing, that's certain.

> Here's what I think I understand so far:
> 
>   Applications can mmap PCI memory space via either sysfs or procfs (the
>   procfs method is deprecated but still supported):
> 
>     - In sysfs, there's a separate /sys/devices/pci*/.../resource* file
>       for each device BAR, and the application opens the appropriate
>       file and supplies the offset from the beginning of the BAR as the
>       mmap(2) offset.
> 
>     - In procfs, the application opens the single /proc/bus/pci/... file
>       for the device.  On most platforms, it supplies the CPU physical
>       address as the mmap(2) offset.  On a few platforms, such as SPARC,
>       it supplies the bus address, i.e., a BAR value, instead.
> 
> But I'm not sure I have this right.  If the procfs offset is either the
> CPU physical address or the BAR value, then pci_resource_to_user()
> should be (depending on the arch) either a no-op or use
> pci_resource_to_bus().

Exactly (pcibios_resource_to_bus() ?).

> But that's not how it's implemented.  Maybe it *could* be?  If
> pci_resource_to_user() gives you something that's not a CPU physical
> address and not a bus address, what *does* it give you, and why would we
> need this third kind of thing?

Well, you need a per arch function implementation where to define if
the conversion from CPU physical address to PCI bus should take place
or not right ? As you mentioned above, if that should be a per-arch
decision, there has to be a per-arch function to filter the resource
in question, I guess that's my understanding behind pci_resource_to_user(),
but I am not sure either, and understanding that was the primary reason
for this patchset so comments are welcome.

Thanks,
Lorenzo

> FWIW, I think the discussion leading up to pci_resource_to_user() is here:
> http://lkml.iu.edu/hypermail/linux/kernel/0504.3/0467.html
> 
> Bjorn
> 
> > Cc: Arnd Bergmann <arnd@arndb.de>
> > 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 92b6d9a..777d8bc 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: [PATCH RFC v2 1/2] drivers: pci: fix pci_mmap_fits() implementation for procfs mmap

[Bjorn Helgaas]

On Tue, Nov 11, 2014 at 4:48 AM, Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
> On Mon, Nov 10, 2014 at 11:04:54PM +0000, Bjorn Helgaas wrote:
>> ...
>> Here's what I think I understand so far:
>>
>>   Applications can mmap PCI memory space via either sysfs or procfs (the
>>   procfs method is deprecated but still supported):
>>
>>     - In sysfs, there's a separate /sys/devices/pci*/.../resource* file
>>       for each device BAR, and the application opens the appropriate
>>       file and supplies the offset from the beginning of the BAR as the
>>       mmap(2) offset.
>>
>>     - In procfs, the application opens the single /proc/bus/pci/... file
>>       for the device.  On most platforms, it supplies the CPU physical
>>       address as the mmap(2) offset.  On a few platforms, such as SPARC,
>>       it supplies the bus address, i.e., a BAR value, instead.
>>
>> But I'm not sure I have this right.  If the procfs offset is either the
>> CPU physical address or the BAR value, then pci_resource_to_user()
>> should be (depending on the arch) either a no-op or use
>> pci_resource_to_bus().
>
> Exactly (pcibios_resource_to_bus() ?).
>
>> But that's not how it's implemented.  Maybe it *could* be?  If
>> pci_resource_to_user() gives you something that's not a CPU physical
>> address and not a bus address, what *does* it give you, and why would we
>> need this third kind of thing?
>
> Well, you need a per arch function implementation where to define if
> the conversion from CPU physical address to PCI bus should take place
> or not right ? As you mentioned above, if that should be a per-arch
> decision, there has to be a per-arch function to filter the resource
> in question, I guess that's my understanding behind pci_resource_to_user(),
> but I am not sure either, and understanding that was the primary reason
> for this patchset so comments are welcome.

I agree that we need pci_resource_to_user() because arches do
different things, so we can't just remove pci_resource_to_user() and
replace it with pci_resource_to_bus().  My point is that we have a
generic pci_resource_to_user() implementation that does nothing, and
if an arch *does* implement its own pci_resource_to_user(), it seems
like it should simply call pci_resource_to_user().

Bjorn

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

[Lorenzo Pieralisi]

On Tue, Nov 11, 2014 at 02:20:31PM +0000, Bjorn Helgaas wrote:
> On Tue, Nov 11, 2014 at 4:48 AM, Lorenzo Pieralisi
> <lorenzo.pieralisi@arm.com> wrote:
> > On Mon, Nov 10, 2014 at 11:04:54PM +0000, Bjorn Helgaas wrote:
> >> ...
> >> Here's what I think I understand so far:
> >>
> >>   Applications can mmap PCI memory space via either sysfs or procfs (the
> >>   procfs method is deprecated but still supported):
> >>
> >>     - In sysfs, there's a separate /sys/devices/pci*/.../resource* file
> >>       for each device BAR, and the application opens the appropriate
> >>       file and supplies the offset from the beginning of the BAR as the
> >>       mmap(2) offset.
> >>
> >>     - In procfs, the application opens the single /proc/bus/pci/... file
> >>       for the device.  On most platforms, it supplies the CPU physical
> >>       address as the mmap(2) offset.  On a few platforms, such as SPARC,
> >>       it supplies the bus address, i.e., a BAR value, instead.
> >>
> >> But I'm not sure I have this right.  If the procfs offset is either the
> >> CPU physical address or the BAR value, then pci_resource_to_user()
> >> should be (depending on the arch) either a no-op or use
> >> pci_resource_to_bus().
> >
> > Exactly (pcibios_resource_to_bus() ?).
> >
> >> But that's not how it's implemented.  Maybe it *could* be?  If
> >> pci_resource_to_user() gives you something that's not a CPU physical
> >> address and not a bus address, what *does* it give you, and why would we
> >> need this third kind of thing?
> >
> > Well, you need a per arch function implementation where to define if
> > the conversion from CPU physical address to PCI bus should take place
> > or not right ? As you mentioned above, if that should be a per-arch
> > decision, there has to be a per-arch function to filter the resource
> > in question, I guess that's my understanding behind pci_resource_to_user(),
> > but I am not sure either, and understanding that was the primary reason
> > for this patchset so comments are welcome.
> 
> I agree that we need pci_resource_to_user() because arches do
> different things, so we can't just remove pci_resource_to_user() and
> replace it with pci_resource_to_bus().  My point is that we have a
> generic pci_resource_to_user() implementation that does nothing, and
> if an arch *does* implement its own pci_resource_to_user(), it seems
> like it should simply call pci_resource_to_user().

to_bus() you mean. Well, I agree, but I am not sure it would work on all
arches that deviate from the generic implementation, I can't speak for other
architectures since I do not have an in-depth knowledge of their PCI
internal implementations, in particular in relation to CPU <-> PCI
address map conversions/mappings.

I read your comment as an agreement on the approach I took in my patch,
except for the current pci_resource_to_user() implementation(s), which I did
not touch.

Thanks,
Lorenzo

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

[Bjorn Helgaas]

On Tue, Nov 11, 2014 at 8:57 AM, Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
> On Tue, Nov 11, 2014 at 02:20:31PM +0000, Bjorn Helgaas wrote:
>> On Tue, Nov 11, 2014 at 4:48 AM, Lorenzo Pieralisi
>> <lorenzo.pieralisi@arm.com> wrote:
>> > On Mon, Nov 10, 2014 at 11:04:54PM +0000, Bjorn Helgaas wrote:
>> >> ...
>> >> Here's what I think I understand so far:
>> >>
>> >>   Applications can mmap PCI memory space via either sysfs or procfs (the
>> >>   procfs method is deprecated but still supported):
>> >>
>> >>     - In sysfs, there's a separate /sys/devices/pci*/.../resource* file
>> >>       for each device BAR, and the application opens the appropriate
>> >>       file and supplies the offset from the beginning of the BAR as the
>> >>       mmap(2) offset.
>> >>
>> >>     - In procfs, the application opens the single /proc/bus/pci/... file
>> >>       for the device.  On most platforms, it supplies the CPU physical
>> >>       address as the mmap(2) offset.  On a few platforms, such as SPARC,
>> >>       it supplies the bus address, i.e., a BAR value, instead.
>> >>
>> >> But I'm not sure I have this right.  If the procfs offset is either the
>> >> CPU physical address or the BAR value, then pci_resource_to_user()
>> >> should be (depending on the arch) either a no-op or use
>> >> pci_resource_to_bus().
>> >
>> > Exactly (pcibios_resource_to_bus() ?).
>> >
>> >> But that's not how it's implemented.  Maybe it *could* be?  If
>> >> pci_resource_to_user() gives you something that's not a CPU physical
>> >> address and not a bus address, what *does* it give you, and why would we
>> >> need this third kind of thing?
>> >
>> > Well, you need a per arch function implementation where to define if
>> > the conversion from CPU physical address to PCI bus should take place
>> > or not right ? As you mentioned above, if that should be a per-arch
>> > decision, there has to be a per-arch function to filter the resource
>> > in question, I guess that's my understanding behind pci_resource_to_user(),
>> > but I am not sure either, and understanding that was the primary reason
>> > for this patchset so comments are welcome.
>>
>> I agree that we need pci_resource_to_user() because arches do
>> different things, so we can't just remove pci_resource_to_user() and
>> replace it with pci_resource_to_bus().  My point is that we have a
>> generic pci_resource_to_user() implementation that does nothing, and
>> if an arch *does* implement its own pci_resource_to_user(), it seems
>> like it should simply call pci_resource_to_user().
>
> to_bus() you mean.

Yes, exactly, sorry for my typo.  (And earlier, when I said
pci_resource_to_bus() instead of pcibios_resource_to_bus().
pcibios_resource_to_bus() *used* to be an arch-specific function with
many implementations, which is why it's named pcibios_*.  Now that the
PCI core supports address translation, it's no longer arch-specific.
We haven't changed the name to reflect that, but I don't think of it
along with the rest of the pcibios_*() functions anymore.  Hmm, and
now we have this pci_resource_to_user() thing, which *is*
arch-specific, but doesn't have a pcibios_* name.  No wonder this is
confusing :))

> Well, I agree, but I am not sure it would work on all
> arches that deviate from the generic implementation, I can't speak for other
> architectures since I do not have an in-depth knowledge of their PCI
> internal implementations, in particular in relation to CPU <-> PCI
> address map conversions/mappings.
>
> I read your comment as an agreement on the approach I took in my patch,
> except for the current pci_resource_to_user() implementation(s), which I did
> not touch.

Yes.  I have two things I'd like to clear up:

  1) Your patch changes behavior on platforms that implement their own
pci_resource_to_user().  So I'd like to mention the details of that in
the changelog, e.g., "procfs mmap on arches X, Y, Z has been broken
since commit C, and this change fixes them."  ARM doesn't implement
pci_resource_to_user(), so I don't think ARM is one of those arches.
But I'd really like to include specifics on what those arches are, and
what we think is currently broken, so their maintainers at least get a
heads-up and can look for that  breakage.

  2) I'd like to take this opportunity to analyze the current
pci_resource_to_user() implementations and see if we can reduce them
to calling pcibios_resource_to_bus().  This is obviously separable,
and I'll apply these patches anyway, but this does seem like a perfect
opportunity for someone (not necessarily you) to clean this stuff up,
since it's fresh in our minds.

Bjorn

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

[Lorenzo Pieralisi]

On Tue, Nov 11, 2014 at 05:19:31PM +0000, Bjorn Helgaas wrote:

[...]

> > I read your comment as an agreement on the approach I took in my patch,
> > except for the current pci_resource_to_user() implementation(s), which I did
> > not touch.
> 
> Yes.  I have two things I'd like to clear up:
> 
>   1) Your patch changes behavior on platforms that implement their own
> pci_resource_to_user().  So I'd like to mention the details of that in
> the changelog, e.g., "procfs mmap on arches X, Y, Z has been broken
> since commit C, and this change fixes them."  ARM doesn't implement
> pci_resource_to_user(), so I don't think ARM is one of those arches.
> But I'd really like to include specifics on what those arches are, and
> what we think is currently broken, so their maintainers at least get a
> heads-up and can look for that  breakage.

I posted a v3, where I *tried* to bisect the commits that actually broke
the procfs interface and I added a commit log to explain why, it is very
hard to bisect a specific commit (given the dependency on the arch code)
and I do not have HW to test the fix on apart from ARM machines so there
is not much I can do on that side.

Please let me know what you think, thanks for having a look.

Lorenzo

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

[Benjamin Herrenschmidt]

On Mon, 2014-11-10 at 16:04 -0700, Bjorn Helgaas wrote:
> But I'm not sure I have this right.  If the procfs offset is either
> the
> CPU physical address or the BAR value, then pci_resource_to_user()
> should be (depending on the arch) either a no-op or use
> pci_resource_to_bus().
> 
> But that's not how it's implemented.  Maybe it *could* be?  If
> pci_resource_to_user() gives you something that's not a CPU physical
> address and not a bus address, what *does* it give you, and why would
> we
> need this third kind of thing?
> 
> FWIW, I think the discussion leading up to pci_resource_to_user() is
> here:
> http://lkml.iu.edu/hypermail/linux/kernel/0504.3/0467.html

Oh, man... I remember that was all a giant trainwreck and some stuff
just couldn't be made completely right due to broken assumptions by
the proc code and users of it... but I don't remember all the details.

I think /proc users don't necessarily pass a BAR value but something
they try to somewhat translates themselves via the "resources" file,
which ends up working ... or not, depending on various factors such
as 32 vs 64 bit etc...

I wonder who still uses this interface....

Cheers,
Ben.

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

[Lorenzo Pieralisi]

On Wed, Nov 12, 2014 at 07:23:49AM +0000, Benjamin Herrenschmidt wrote:
> On Mon, 2014-11-10 at 16:04 -0700, Bjorn Helgaas wrote:
> > But I'm not sure I have this right.  If the procfs offset is either
> > the
> > CPU physical address or the BAR value, then pci_resource_to_user()
> > should be (depending on the arch) either a no-op or use
> > pci_resource_to_bus().
> > 
> > But that's not how it's implemented.  Maybe it *could* be?  If
> > pci_resource_to_user() gives you something that's not a CPU physical
> > address and not a bus address, what *does* it give you, and why would
> > we
> > need this third kind of thing?
> > 
> > FWIW, I think the discussion leading up to pci_resource_to_user() is
> > here:
> > http://lkml.iu.edu/hypermail/linux/kernel/0504.3/0467.html
> 
> Oh, man... I remember that was all a giant trainwreck and some stuff
> just couldn't be made completely right due to broken assumptions by
> the proc code and users of it... but I don't remember all the details.
> 
> I think /proc users don't necessarily pass a BAR value but something
> they try to somewhat translates themselves via the "resources" file,
> which ends up working ... or not, depending on various factors such
> as 32 vs 64 bit etc...
> 
> I wonder who still uses this interface....

+1, even though I do not think that leaving it as it is is a good idea,
hence I posted this series.

I tried to fix it while fixing the way ARM pcibios code handles
pci_mmap_page_range() (for both procfs and sysfs mappings).

I will do what Bjorn suggested, more comments from arches maintainers
are welcome.

Lorenzo

[PATCH RFC v2 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: [PATCH RFC v2 0/2] Fix procfs PCI resources mmap

[Lorenzo Pieralisi]

On Fri, Oct 24, 2014 at 05:28:04PM +0100, 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.

I appreciate it is hardly number one priority and by no means urgent,
but I would like to get some comments if possible on patch 1 since patch
2, that is a fix for ARM, relies on the approach we go for in order to
fix the pci_mmap_fits() behaviour (patch 1).

Comments welcome.

Thanks,
Lorenzo

> 
> v1 posting:
> 
> http://marc.info/?l=linux-kernel&m=141337461318554&w=2
> 
> 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
> 
>