Hi Jesse & linux-pci,
may I kindly ask if anyone has had a look at this patch yet?
https://patchwork.kernel.org/patch/270271/
http://kerneltrap.org/mailarchive/linux-kernel/2010/10/21/4635039
Regards
Martin
--
Dr. Martin Wilck
PRIMERGY System Software Engineer
x86 Server Engineering
FUJITSU
Fujitsu Technology Solutions GmbH
Heinz-Nixdorf-Ring 1
33106 Paderborn, Germany
Phone: ++49 5251 525 2796
Fax: ++49 5251 525 2820
Email: [email protected]
Internet: http://ts.fujitsu.com
Company Details: http://ts.fujitsu.com/imprint
On Tue, 09 Nov 2010 10:43:07 +0100
Martin Wilck <[email protected]> wrote:
> Hi Jesse & linux-pci,
>
> may I kindly ask if anyone has had a look at this patch yet?
>
> https://patchwork.kernel.org/patch/270271/
> http://kerneltrap.org/mailarchive/linux-kernel/2010/10/21/4635039
Yes I looked at it, and then promptly lost it in my inbox.
Thanks for fixing this; /proc/bus/pci semantics are definitely
different than /sys so we definitely want this.
I don't really like the new int argument to pci_mmap_fits (it's not
obvious from reading callers what that arg would do), maybe you could
make it into an enum that better describes the API or semantic being
used for the check?
Thanks,
--
Jesse Barnes, Intel Open Source Technology Center
The checks for valid mmaps of PCI resources made through /proc/bus/pci files
that were introduced in 9eff02e2042f96fb2aedd02e032eca1c5333d767 have several
problems:
1. mmap() calls on /proc/bus/pci files are made with real file offsets > 0,
whereas under /sys/bus/pci/devices, the start of the resource corresponds
to offset 0. This may lead to false negatives in pci_mmap_fits(), which
implicitly assumes the /sys/bus/pci/devices layout.
2. The loop in proc_bus_pci_mmap doesn't skip empty resouces. This leads
to false positives, because pci_mmap_fits() doesn't treat empty resources
correctly (the calculated size is 1 << (8*sizeof(resource_size_t)-PAGE_SHIFT)
in this case!).
3. If a user maps resources with BAR > 0, pci_mmap_fits will emit bogus
WARNINGS for the first resources that don't fit until the correct one is found.
On many controllers the first 2-4 BARs are used, and the others are empty.
In this case, an mmap attempt will first fail on the non-empty BARs
(including the "right" BAR because of 1.) and emit bogus WARNINGS because
of 3., and finally succeed on the first empty BAR because of 2.
This is certainly not the intended behaviour.
This patch addresses all 3 issues.
Updated with an enum type for the additional parameter for pci_mmap_fits().
Signed-off-by: Martin Wilck <[email protected]>
---
drivers/pci/pci-sysfs.c | 22 ++++++++++++++++------
drivers/pci/pci.h | 7 ++++++-
drivers/pci/proc.c | 2 +-
3 files changed, 23 insertions(+), 8 deletions(-)
diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index b5a7d9b..25accc9 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -705,17 +705,21 @@ void pci_remove_legacy_files(struct pci_bus *b)
#ifdef HAVE_PCI_MMAP
-int pci_mmap_fits(struct pci_dev *pdev, int resno, struct vm_area_struct *vma)
+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;
+ unsigned long nr, start, size, pci_start;
+ if (pci_resource_len(pdev, resno) == 0)
+ return 0;
nr = (vma->vm_end - vma->vm_start) >> PAGE_SHIFT;
start = vma->vm_pgoff;
size = ((pci_resource_len(pdev, resno) - 1) >> PAGE_SHIFT) + 1;
- if (start < size && size - start >= nr)
+ pci_start = (mmap_api == PCI_MMAP_SYSFS) ?
+ pci_resource_start(pdev, resno) >> PAGE_SHIFT : 0;
+ if (start >= pci_start && start < pci_start + size &&
+ start + nr <= pci_start + size)
return 1;
- WARN(1, "process \"%s\" tried to map 0x%08lx-0x%08lx on %s BAR %d (size 0x%08lx)\n",
- current->comm, start, start+nr, pci_name(pdev), resno, size);
return 0;
}
@@ -745,8 +749,14 @@ pci_mmap_resource(struct kobject *kobj, struct bin_attribute *attr,
if (i >= PCI_ROM_RESOURCE)
return -ENODEV;
- if (!pci_mmap_fits(pdev, i, vma))
+ if (!pci_mmap_fits(pdev, i, vma, PCI_MMAP_SYSFS)) {
+ WARN(1, "process \"%s\" tried to map 0x%08lx bytes "
+ "at page 0x%08lx on %s BAR %d (start 0x%16Lx, size 0x%16Lx)\n",
+ current->comm, vma->vm_end-vma->vm_start, vma->vm_pgoff,
+ pci_name(pdev), i,
+ pci_resource_start(pdev, i), pci_resource_len(pdev, i));
return -EINVAL;
+ }
/* pci_mmap_page_range() expects the same kind of entry as coming
* from /proc/bus/pci/ which is a "user visible" value. If this is
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index f5c7c38..7d33f66 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -22,8 +22,13 @@ extern void pci_remove_firmware_label_files(struct pci_dev *pdev);
#endif
extern void pci_cleanup_rom(struct pci_dev *dev);
#ifdef HAVE_PCI_MMAP
+enum pci_mmap_api {
+ PCI_MMAP_SYSFS, /* mmap on /sys/bus/pci/devices/<BDF>/resource<N> */
+ PCI_MMAP_PROCFS /* mmap on /proc/bus/pci/<BDF> */
+};
extern int pci_mmap_fits(struct pci_dev *pdev, int resno,
- struct vm_area_struct *vma);
+ struct vm_area_struct *vmai,
+ enum pci_mmap_api mmap_api);
#endif
int pci_probe_reset_function(struct pci_dev *dev);
diff --git a/drivers/pci/proc.c b/drivers/pci/proc.c
index 297b72c..ea00647 100644
--- a/drivers/pci/proc.c
+++ b/drivers/pci/proc.c
@@ -257,7 +257,7 @@ static int proc_bus_pci_mmap(struct file *file, struct vm_area_struct *vma)
/* Make sure the caller is mapping a real resource for this device */
for (i = 0; i < PCI_ROM_RESOURCE; i++) {
- if (pci_mmap_fits(dev, i, vma))
+ if (pci_mmap_fits(dev, i, vma, PCI_MMAP_PROCFS))
break;
}
--
1.6.3.2
On Wed, 10 Nov 2010 11:03:21 +0100
Martin Wilck <[email protected]> wrote:
> The checks for valid mmaps of PCI resources made
> through /proc/bus/pci files that were introduced in
> 9eff02e2042f96fb2aedd02e032eca1c5333d767 have several problems:
>
> 1. mmap() calls on /proc/bus/pci files are made with real file
> offsets > 0, whereas under /sys/bus/pci/devices, the start of the
> resource corresponds to offset 0. This may lead to false negatives in
> pci_mmap_fits(), which implicitly assumes the /sys/bus/pci/devices
> layout.
>
> 2. The loop in proc_bus_pci_mmap doesn't skip empty resouces. This
> leads to false positives, because pci_mmap_fits() doesn't treat empty
> resources correctly (the calculated size is 1 <<
> (8*sizeof(resource_size_t)-PAGE_SHIFT) in this case!).
>
> 3. If a user maps resources with BAR > 0, pci_mmap_fits will emit
> bogus WARNINGS for the first resources that don't fit until the
> correct one is found.
>
> On many controllers the first 2-4 BARs are used, and the others are
> empty. In this case, an mmap attempt will first fail on the non-empty
> BARs (including the "right" BAR because of 1.) and emit bogus
> WARNINGS because of 3., and finally succeed on the first empty BAR
> because of 2. This is certainly not the intended behaviour.
>
> This patch addresses all 3 issues.
> Updated with an enum type for the additional parameter for
> pci_mmap_fits().
>
> Signed-off-by: Martin Wilck <[email protected]>
Thanks Martin, I'll push this into my for-linus branch for 2.6.37; may
as well cc: stable as well, since this is a long standing bug.
--
Jesse Barnes, Intel Open Source Technology Center
On Wed, Nov 10, 2010 at 10:44:34AM -0800, Jesse Barnes wrote:
> On Wed, 10 Nov 2010 11:03:21 +0100
> Martin Wilck <[email protected]> wrote:
>
> > The checks for valid mmaps of PCI resources made
> > through /proc/bus/pci files that were introduced in
> > 9eff02e2042f96fb2aedd02e032eca1c5333d767 have several problems:
> >
> > 1. mmap() calls on /proc/bus/pci files are made with real file
> > offsets > 0, whereas under /sys/bus/pci/devices, the start of the
> > resource corresponds to offset 0. This may lead to false negatives in
> > pci_mmap_fits(), which implicitly assumes the /sys/bus/pci/devices
> > layout.
> >
> > 2. The loop in proc_bus_pci_mmap doesn't skip empty resouces. This
> > leads to false positives, because pci_mmap_fits() doesn't treat empty
> > resources correctly (the calculated size is 1 <<
> > (8*sizeof(resource_size_t)-PAGE_SHIFT) in this case!).
> >
> > 3. If a user maps resources with BAR > 0, pci_mmap_fits will emit
> > bogus WARNINGS for the first resources that don't fit until the
> > correct one is found.
> >
> > On many controllers the first 2-4 BARs are used, and the others are
> > empty. In this case, an mmap attempt will first fail on the non-empty
> > BARs (including the "right" BAR because of 1.) and emit bogus
> > WARNINGS because of 3., and finally succeed on the first empty BAR
> > because of 2. This is certainly not the intended behaviour.
> >
> > This patch addresses all 3 issues.
> > Updated with an enum type for the additional parameter for
> > pci_mmap_fits().
> >
> > Signed-off-by: Martin Wilck <[email protected]>
>
> Thanks Martin, I'll push this into my for-linus branch for 2.6.37; may
> as well cc: stable as well, since this is a long standing bug.
Please cc: stable, as it is something that -stable releases cares about.
thanks,
greg k-h