Incorrect size checks on mmap() for sysfs PCI resource files is breaking X?

Incorrect size checks on mmap() for sysfs PCI resource files is breaking X?

[Darrick J. Wong]

Hi,

I just loaded 2.6.37-rc2 on my machines, and I noticed that X no longer starts.
Running an strace of the X server shows that it's doing this:

open("/sys/bus/pci/devices/0000:07:00.0/resource0", O_RDWR) = 10
mmap(NULL, 16777216, PROT_READ|PROT_WRITE, MAP_SHARED, 10, 0) = -1 EINVAL (Invalid argument)

This code seems to be asking for a shared read/write mapping of 16MB worth of
BAR0 starting at file offset 0, and letting the kernel assign a starting
address.  Here's the lspci dump:

07:00.0 VGA compatible controller: Matrox Graphics, Inc. MGA G200EV
	Subsystem: IBM Device 0369
	Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
	Status: Cap+ 66MHz- UDF- FastB2B+ ParErr- DEVSEL=medium >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Latency: 64 (4000ns min, 8000ns max), Cache Line Size: 64 bytes
	Interrupt: pin A routed to IRQ 11
	Region 0: Memory at 96000000 (32-bit, prefetchable) [size=16M]
	Region 1: Memory at 97800000 (32-bit, non-prefetchable) [size=16K]
	Region 2: Memory at 97000000 (32-bit, non-prefetchable) [size=8M]
	Capabilities: [dc] Power Management version 1
		Flags: PMEClk- DSI+ D1- D2- AuxCurrent=0mA PME(D0-,D1-,D2-,D3hot-,D3cold-)
		Status: D0 PME-Enable- DSel=0 DScale=0 PME-
	Kernel modules: matroxfb_base

Looking into dmesg, there's a complaint like so:

[  541.445705] ------------[ cut here ]------------
[  541.450339] WARNING: at /home/djwong/linux-2.6.37-rc2-fs/drivers/pci/pci-sysfs.c:758 pci_mmap_resource+0xff/0x161()
[  541.460801] Hardware name: System x3550 M2 -[7946AC1]-
[  541.465980] process "Xorg" tried to map 0x01000000 bytes at page 0x00000000 on 0000:07:00.0 BAR 0 (start 0x        96000000, size 0x         1000000)
[  541.479414] Modules linked in: ext4 mbcache jbd2 crc16 dm_round_robin dm_multipath dm_mod nfsd exportfs eeprom kvm_intel kvm acpi_cpufreq mperf k10temp k8temp coretemp ipv6 rtc_cmos rtc_core cdc_ether i7core_edac shpchp ioatdma serio_raw mousedev usbnet evdev edac_core rtc_lib pci_hotplug dca processor button af_packet nfs lockd fscache nfs_acl auth_rpcgss sunrpc virtio_pci sd_mod crc_t10dif sg sr_mod cdrom usbhid hid mptsas mptscsih ata_piix mptbase libata scsi_transport_sas uhci_hcd lpfc ehci_hcd scsi_transport_fc usbcore scsi_tgt scsi_mod bnx2 nls_base virtio_net virtio virtio_ring bridge stp llc raid10 raid456 async_raid6_recov async_pq raid6_pq async_xor xor async_memcpy async_tx raid1 raid0 multipath linear md_mod nbd
[  541.546034] Pid: 3983, comm: Xorg Tainted: G        W   2.6.37-rc2-fs64 #1
[  541.552898] Call Trace:
[  541.555342]  [<ffffffff81050809>] warn_slowpath_common+0x85/0x9d
[  541.561335]  [<ffffffff810508c4>] warn_slowpath_fmt+0x46/0x48
[  541.567073]  [<ffffffff811b99c5>] pci_mmap_resource+0xff/0x161
[  541.572932]  [<ffffffff811b9a40>] pci_mmap_resource_uc+0x19/0x1b
[  541.578965]  [<ffffffff8116568b>] mmap+0x73/0xfa
[  541.583619]  [<ffffffff810e9b56>] mmap_region+0x2bb/0x4c4
[  541.589051]  [<ffffffff8100e39b>] ? arch_get_unmapped_area_topdown+0x1c3/0x290
[  541.596295]  [<ffffffff810e9fef>] do_mmap_pgoff+0x290/0x2f3
[  541.601903]  [<ffffffff810ea148>] sys_mmap_pgoff+0xf6/0x12e
[  541.607511]  [<ffffffff8100df4e>] sys_mmap+0x22/0x24
[  541.612482]  [<ffffffff8100a00a>] tracesys+0xd0/0xd5
[  541.617504] ---[ end trace e20ec6912607e574 ]---

...with the following code in pci_mmap_fits:

	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)

It looks like the logic here is set up such that when the mmap call comes via
sysfs, the check in pci_mmap_fits wants vma->vm_pgoff to be between the
resource's start and end address, and the end of the vma to be no farther than
the end.  However, the sysfs PCI resource files always start at offset zero,
which means that this test always fails for programs that mmap the sysfs files.
Given the comment in the original commit
3b519e4ea618b6943a82931630872907f9ac2c2b, I _think_ the old procfs files
require that the file offset be equal to the resource's base address when
mmapping.

I think what we want here is for pci_start to be 0 when mmap_api ==
PCI_MMAP_PROCFS, though I'll leave it up to the author to ack me or prove me
wrong.

In any case, X is totally broken -- I can't get it started on Matrox, Radeon,
or Mach64 hardware, and I'd bet everything else is broken too.

--D

Re: Incorrect size checks on mmap() for sysfs PCI resource files is breaking X?

[Dave Airlie]

On Tue, Nov 16, 2010 at 5:22 PM, Darrick J. Wong <djwong@us.ibm.com> wrote:
> Hi,
>
> I just loaded 2.6.37-rc2 on my machines, and I noticed that X no longer starts.
> Running an strace of the X server shows that it's doing this:

Can you poinpoint when it went wrong? does 2.6.36 work okay?

Just trying to figure out where the regression arrived.

Dave.

[REGRESSSION 2.6.37-rc2][PATCH] pci: Fix mmap address check in pci_mmap_fits

[Darrick J. Wong]

I just loaded 2.6.37-rc2 on my machines, and I noticed that X no longer starts.
Running an strace of the X server shows that it's doing this:

open("/sys/bus/pci/devices/0000:07:00.0/resource0", O_RDWR) = 10
mmap(NULL, 16777216, PROT_READ|PROT_WRITE, MAP_SHARED, 10, 0) = -1 EINVAL (Invalid argument)

This code seems to be asking for a shared read/write mapping of 16MB worth of
BAR0 starting at file offset 0, and letting the kernel assign a starting
address.  Unfortunately, this -EINVAL causes X not to start.  Looking into
dmesg, there's a complaint like so:

process "Xorg" tried to map 0x01000000 bytes at page 0x00000000 on 0000:07:00.0 BAR 0 (start 0x        96000000, size 0x         1000000)

...with the following code in pci_mmap_fits:

	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)

It looks like the logic here is set up such that when the mmap call comes via
sysfs, the check in pci_mmap_fits wants vma->vm_pgoff to be between the
resource's start and end address, and the end of the vma to be no farther than
the end.  However, the sysfs PCI resource files always start at offset zero,
which means that this test always fails for programs that mmap the sysfs files.
Given the comment in the original commit
3b519e4ea618b6943a82931630872907f9ac2c2b, I _think_ the old procfs files
require that the file offset be equal to the resource's base address when
mmapping.

I think what we want here is for pci_start to be 0 when mmap_api ==
PCI_MMAP_PROCFS.  The following patch makes that change, after which the Matrox
and Mach64 X drivers work again.

Signed-off-by: Darrick J. Wong <djwong@us.ibm.com>
---

 drivers/pci/pci-sysfs.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 95712a3..63d5042 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -715,7 +715,7 @@ int pci_mmap_fits(struct pci_dev *pdev, int resno, struct vm_area_struct *vma,
 	nr = (vma->vm_end - vma->vm_start) >> PAGE_SHIFT;
 	start = vma->vm_pgoff;
 	size = ((pci_resource_len(pdev, resno) - 1) >> PAGE_SHIFT) + 1;
-	pci_start = (mmap_api == PCI_MMAP_SYSFS) ?
+	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)

Re: Incorrect size checks on mmap() for sysfs PCI resource files is breaking X?

[Darrick J. Wong]

On Tue, Nov 16, 2010 at 05:53:35PM +1000, Dave Airlie wrote:
> On Tue, Nov 16, 2010 at 5:22 PM, Darrick J. Wong <djwong@us.ibm.com> wrote:
> > Hi,
> >
> > I just loaded 2.6.37-rc2 on my machines, and I noticed that X no longer starts.
> > Running an strace of the X server shows that it's doing this:
> 
> Can you poinpoint when it went wrong? does 2.6.36 work okay?

Yes, 2.6.36 is ok.  2.6.37-rc1 was ok too.

> Just trying to figure out where the regression arrived.

Looks like it's commit 3b519e4ea618b6943a82931630872907f9ac2c2b which went in
11 Nov 2010.  I just submitted a patch that fixes it, at least for me.

--D

Re: [REGRESSSION 2.6.37-rc2][PATCH] pci: Fix mmap address check in pci_mmap_fits

[Martin Wilck]

Hi Darrick,

> I think what we want here is for pci_start to be 0 when mmap_api ==
> PCI_MMAP_PROCFS.  The following patch makes that change, after which the Matrox
> and Mach64 X drivers work again.

Of course. I made a stupid mistake when I introduced the enum :-( Thanks 
for spotting and fixing it so quickly.

Jesse, please apply Darrick's fix.

Martin

> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index 95712a3..63d5042 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -715,7 +715,7 @@ int pci_mmap_fits(struct pci_dev *pdev, int resno, struct vm_area_struct *vma,
>   	nr = (vma->vm_end - vma->vm_start)>>  PAGE_SHIFT;
>   	start = vma->vm_pgoff;
>   	size = ((pci_resource_len(pdev, resno) - 1)>>  PAGE_SHIFT) + 1;
> -	pci_start = (mmap_api == PCI_MMAP_SYSFS) ?
> +	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)

-- 
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:			martin.wilck@ts.fujitsu.com
Internet:		http://ts.fujitsu.com
Company Details:	http://ts.fujitsu.com/imprint