Bug 754565 - Fix device assignment Coverity issues
Summary: Fix device assignment Coverity issues
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Red Hat Enterprise Linux 6
Classification: Red Hat
Component: qemu-kvm
Version: 6.3
Hardware: x86_64
OS: Unspecified
unspecified
unspecified
Target Milestone: rc
: ---
Assignee: Alex Williamson
QA Contact: Virtualization Bugs
URL:
Whiteboard: developer-ack-6.3
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2011-11-16 20:51 UTC by Alex Williamson
Modified: 2012-06-20 11:36 UTC (History)
9 users (show)

Fixed In Version: qemu-kvm-0.12.1.2-2.226.el6
Doc Type: Bug Fix
Doc Text:
No documentation needed
Clone Of:
Environment:
Last Closed: 2012-06-20 11:36:16 UTC
Target Upstream Version:


Attachments (Terms of Use)
Coverity scan on qemu-kvm-293 (167.93 KB, application/x-xz)
2012-05-23 04:58 UTC, Chao Yang
no flags Details


Links
System ID Private Priority Status Summary Last Updated
Red Hat Product Errata RHBA-2012:0746 0 normal SHIPPED_LIVE qemu-kvm bug fix and enhancement update 2012-06-19 19:31:48 UTC

Description Alex Williamson 2011-11-16 20:51:36 UTC
Description of problem:
Coverity found several subtle issues in device assignment with PCIe capability fields and the ioport probing error path.  Patches posted upstream.  Include these in RHEL

Version-Release number of selected component (if applicable):


How reproducible:


Steps to Reproduce:
1.
2.
3.
  
Actual results:


Expected results:


Additional info:
Patch pointers to follow once archives refresh

Comment 1 Alex Williamson 2011-11-16 20:57:16 UTC
http://www.spinics.net/lists/kvm/msg63852.html
http://www.spinics.net/lists/kvm/msg63853.html
http://www.spinics.net/lists/kvm/msg63854.html
http://www.spinics.net/lists/kvm/msg63855.html

(Patches 1 & 2 aren't applicable since they were introduced with the new memory API)

Comment 2 Markus Armbruster 2011-11-24 17:21:57 UTC
Are you sure the first two aren't applicable?

Comment 3 Alex Williamson 2011-11-25 18:05:09 UTC
(In reply to comment #2)
> Are you sure the first two aren't applicable?

Patch #1 fixes de-registering memory sub-regions before destroying the memory container.  This was a bug introduced by the memory API port of device assignment.  If we don't backport the memory API, we don't need this fix.

Patch #2 fixes the ioport access functions.  The memory API port attempted to make use of the existing functions, but it doesn't work.  This patch refreshed the access function to the new style API.  If we don't backport the memory API, we don't need this fix.

Comment 4 Markus Armbruster 2011-11-28 08:19:53 UTC
Off-by two bug: I read "patch #2" as "second of the four links in comment#1" instead of "upstream PATCH 2/6".  Sorry for the noise.

Comment 5 juzhang 2011-11-28 08:57:22 UTC
Hi,Alex

   Would you please provide an efficient way for qe verify this kind(found by Coverity) of issue? thanks

Comment 6 Alex Williamson 2011-11-28 15:34:11 UTC
(In reply to comment #5)
> Hi,Alex
> 
>    Would you please provide an efficient way for qe verify this kind(found by
> Coverity) of issue? thanks

There is no simple way to test most of this.  The first patch in comment 1 might allow a PCIe non-endpoint device to be assigned, so we could maybe try assigning a PCIe root port (by qemu command line, not libvirt).  That shouldn't work after the patch.  The 2nd patch is a masking of PCI link capability bits, which you might notice in lspci for the device, but you'd have to be using a device that exposes an L1 exit latency.  Patch 3 is preventing bits in the PCIe link capabilities from being writable.  I guess we could try to write 0 or ~0 to the link capability register and see that there was a change and with the patch the write has no effect.  There's no way to test the last patch w/o modifying a kernel.

Comment 13 Dor Laor 2012-04-22 11:31:33 UTC
    Technical note added. If any revisions are required, please edit the "Technical Notes" field
    accordingly. All revisions will be proofread by the Engineering Content Services team.
    
    New Contents:
No documentation needed

Comment 14 Chao Yang 2012-05-10 13:15:14 UTC
(In reply to comment #6)
> (In reply to comment #5)
> > Hi,Alex
> > 
> >    Would you please provide an efficient way for qe verify this kind(found by
> > Coverity) of issue? thanks
> 
> There is no simple way to test most of this.  The first patch in comment 1
> might allow a PCIe non-endpoint device to be assigned, so we could maybe try
> assigning a PCIe root port (by qemu command line, not libvirt).  That 
Hi Alex, 
 I was trying to verify the first patch according to your method - try assigning a PCIe root port, seems it doesn't work on qemu-209, can you please take a look and tell my any step missing or offer me another way to verify? Thanks.

# /usr/libexec/qemu-kvm -M rhel6.2.0 -enable-kvm -m 2048 -smp 2,sockets=1,cores=2,threads=1 -name test -uuid `uuidgen` -rtc base=utc,clock=host,driftfix=slew -boot menu=on -drive file=/home/rhel6.3-64.qcow2,if=none,id=drive-virtio-0-0,media=disk,format=qcow2,cache=none,werror=stop,rerror=stop -device virtio-blk-pci,drive=drive-virtio-0-0,id=virt0-0-0,scsi=off -netdev tap,id=net -device virtio-net-pci,netdev=net,id=net0,mac=64:31:59:63:79:23 -usb -spice port=8000,disable-ticketing -vga qxl -global qxl-vga.vram_size=67108864 -monitor stdio -balloon none -device pci-assign,host=00:1c.0,id=root-port
do_spice_init: starting 0.10.1
spice_server_add_interface: SPICE_INTERFACE_MIGRATION
spice_server_add_interface: SPICE_INTERFACE_KEYBOARD
spice_server_add_interface: SPICE_INTERFACE_MOUSE
spice_server_add_interface: SPICE_INTERFACE_QXL
red_worker_main: begin
display_channel_create: create display channel
cursor_channel_create: create cursor channel
Failed to assign device "root-port" : Operation not permitted
qemu-kvm: -device pci-assign,host=00:1c.0,id=root-port: Device 'pci-assign' could not be initialized

# lspci -vvv -s 00:1c.0
00:1c.0 PCI bridge: Intel Corporation 82801JI (ICH10 Family) PCI Express Root Port 1 (prog-if 00 [Normal decode])
	Control: I/O+ Mem+ BusMaster- SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR+ FastB2B- DisINTx-
	Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Bus: primary=00, secondary=1c, subordinate=1c, sec-latency=0
	I/O behind bridge: 00003000-00003fff
	Memory behind bridge: fff00000-000fffff
	Prefetchable memory behind bridge: 00000000fff00000-00000000000fffff
	Secondary status: 66MHz- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- <SERR- <PERR-
	BridgeCtl: Parity- SERR+ NoISA- VGA- MAbort- >Reset- FastB2B-
		PriDiscTmr- SecDiscTmr- DiscTmrStat- DiscTmrSERREn-
	Capabilities: [40] Express (v1) Root Port (Slot+), MSI 00
		DevCap:	MaxPayload 128 bytes, PhantFunc 0, Latency L0s <64ns, L1 <1us
			ExtTag- RBE+ FLReset-
		DevCtl:	Report errors: Correctable- Non-Fatal- Fatal- Unsupported-
			RlxdOrd- ExtTag- PhantFunc- AuxPwr- NoSnoop-
			MaxPayload 128 bytes, MaxReadReq 128 bytes
		DevSta:	CorrErr- UncorrErr- FatalErr- UnsuppReq- AuxPwr+ TransPend-
		LnkCap:	Port #1, Speed 2.5GT/s, Width x4, ASPM L0s L1, Latency L0 <1us, L1 <4us
			ClockPM- Surprise- LLActRep+ BwNot-
		LnkCtl:	ASPM Disabled; RCB 64 bytes Disabled- Retrain- CommClk-
			ExtSynch- ClockPM- AutWidDis- BWInt- AutBWInt-
		LnkSta:	Speed 2.5GT/s, Width x0, TrErr- Train- SlotClk+ DLActive- BWMgmt- ABWMgmt-
		SltCap:	AttnBtn- PwrCtrl- MRL- AttnInd- PwrInd- HotPlug+ Surpise+
			Slot #  3, PowerLimit 10.000000; Interlock- NoCompl-
		SltCtl:	Enable: AttnBtn- PwrFlt- MRL- PresDet- CmdCplt- HPIrq- LinkChg-
			Control: AttnInd Unknown, PwrInd Unknown, Power- Interlock-
		SltSta:	Status: AttnBtn- PowerFlt- MRL- CmdCplt- PresDet- Interlock-
			Changed: MRL- PresDet- LinkState-
		RootCtl: ErrCorrectable- ErrNon-Fatal- ErrFatal- PMEIntEna- CRSVisible-
		RootCap: CRSVisible-
		RootSta: PME ReqID 0000, PMEStatus- PMEPending-
	Capabilities: [80] MSI: Enable- Count=1/1 Maskable- 64bit-
		Address: fee00000  Data: 4069
	Capabilities: [90] Subsystem: Hewlett-Packard Company Device 130b
	Capabilities: [a0] Power Management version 2
		Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0+,D1-,D2-,D3hot+,D3cold+)
		Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME-
	Capabilities: [100] Virtual Channel <?>
	Capabilities: [180] Root Complex Link <?>
	Kernel driver in use: pci-stub
	Kernel modules: shpchp

Comment 15 Alex Williamson 2012-05-10 16:33:04 UTC
(In reply to comment #14)
>  I was trying to verify the first patch according to your method - try
> assigning a PCIe root port, seems it doesn't work on qemu-209, can you please
> take a look and tell my any step missing or offer me another way to verify?
> Thanks.
> 
> # /usr/libexec/qemu-kvm -M rhel6.2.0 -enable-kvm -m 2048 -smp
> 2,sockets=1,cores=2,threads=1 -name test -uuid `uuidgen` -rtc
> base=utc,clock=host,driftfix=slew -boot menu=on -drive
> file=/home/rhel6.3-64.qcow2,if=none,id=drive-virtio-0-0,media=disk,format=qcow2,cache=none,werror=stop,rerror=stop
> -device virtio-blk-pci,drive=drive-virtio-0-0,id=virt0-0-0,scsi=off -netdev
> tap,id=net -device virtio-net-pci,netdev=net,id=net0,mac=64:31:59:63:79:23 -usb
> -spice port=8000,disable-ticketing -vga qxl -global qxl-vga.vram_size=67108864
> -monitor stdio -balloon none -device pci-assign,host=00:1c.0,id=root-port
> do_spice_init: starting 0.10.1
> spice_server_add_interface: SPICE_INTERFACE_MIGRATION
> spice_server_add_interface: SPICE_INTERFACE_KEYBOARD
> spice_server_add_interface: SPICE_INTERFACE_MOUSE
> spice_server_add_interface: SPICE_INTERFACE_QXL
> red_worker_main: begin
> display_channel_create: create display channel
> cursor_channel_create: create cursor channel
> Failed to assign device "root-port" : Operation not permitted

It looks like you got a -EPERM from kvm, which is from Bug 756093 where the kernel will no longer allow such devices to be assigned.  IIRC, with this change qemu-kvm won't even attempt to assign the device and you should see this error:

    fprintf(stderr,
            "Device assignment only supports endpoint assignment, "
            "device type %d\n", type);

So as long as the test above is qemu-kvm without these fixes, this could still be a valid test case.

Comment 16 Chao Yang 2012-05-18 12:25:26 UTC
Thanks Alex. But still have no idea how to reproduce the issue patch 2&3 fixes.

Reproduction:
------------

------- Patch 1: a PCIe root port could be assigned to guest on:
2.6.32-220.el6.x86_64, qemu-kvm-0.12.1.2-2.209.el6.x86_64
logs from dmesg:
pci-stub 0000:00:1c.0: restoring config space at offset 0x1 (was 0x100400, writing 0x100103)
assign device: host bdf = 0:1c:0
pci-stub 0000:00:1c.0: restoring config space at offset 0x1 (was 0x100400, writing 0x100103)
tap0: no IPv6 routers present
kvm: 9375: cpu0 unimplemented perfctr wrmsr: 0xc1 data 0xabcd
switch: port 2(tap0) entering disabled state
device tap0 left promiscuous mode
switch: port 2(tap0) entering disabled state
pci-stub 0000:00:1c.0: restoring config space at offset 0x1 (was 0x100400, writing 0x100007)

------- Patch 2 & 3: I don't see any difference by lspci in guest between unfixed version and fixed version. So I used Coverity to scan qemu-kvm-209. If there is any efficient way to reproduce, I'd like to retest.

A slice of qemu-kvm-0.12.1.2-2.209.el6/run1/qemu-kvm-0.12.1.2-2.209.el6.err:
...
Error: CONSTANT_EXPRESSION_RESULT:
/builddir/build/BUILD/qemu-kvm-0.12.1.2/hw/device-assignment.c:1510: extra_high_bits: In lnkcap &= 262143 /* (((0xf | 0x3f0) | 0xc00) | 0x7000) | 0x38000 */, wider 262143 /* (((0xf | 0x3f0) | 0xc00) | 0x7000) | 0x38000 */ has high-order bits (0x30000) that don't affect the narrower left-hand side.

Error: CONSTANT_EXPRESSION_RESULT:
/builddir/build/BUILD/qemu-kvm-0.12.1.2/hw/device-assignment.c:1477: result_independent_of_operands: (type & 0xf0) >> 8 is 0 regardless of the values of its operands. This occurs as the non-specific operand of assignment.


Verification:
------------

------- Patch 1, a PCIe root port could not be assigned to guest on qemu-kvm-0.12.1.2-2.293.el6.x86_64 on error:
Device assignment only supports endpoint assignment, device type 4
qemu-kvm: -device pci-assign,host=00:1c.0,id=root-port: Device 'pci-assign' could not be initialized

------- Patch 2 & 3, rescanned on qemu-kvm-0.12.1.2-2.293, no such defects found.

Comment 17 Alex Williamson 2012-05-23 03:21:27 UTC
(In reply to comment #16)
> 
> A slice of qemu-kvm-0.12.1.2-2.209.el6/run1/qemu-kvm-0.12.1.2-2.209.el6.err:
> ...
> Error: CONSTANT_EXPRESSION_RESULT:
> /builddir/build/BUILD/qemu-kvm-0.12.1.2/hw/device-assignment.c:1510:
> extra_high_bits: In lnkcap &= 262143 /* (((0xf | 0x3f0) | 0xc00) | 0x7000) |
> 0x38000 */, wider 262143 /* (((0xf | 0x3f0) | 0xc00) | 0x7000) | 0x38000 */
> has high-order bits (0x30000) that don't affect the narrower left-hand side.
> 
> Error: CONSTANT_EXPRESSION_RESULT:
> /builddir/build/BUILD/qemu-kvm-0.12.1.2/hw/device-assignment.c:1477:
> result_independent_of_operands: (type & 0xf0) >> 8 is 0 regardless of the
> values of its operands. This occurs as the non-specific operand of
> assignment.
> 
> 
> Verification:
> ------------

> ------- Patch 2 & 3, rescanned on qemu-kvm-0.12.1.2-2.293, no such defects
> found.

I think we should count the coverity fixes as verification.  These are very minut fixes that would require just the right device to verify otherwise.  Thanks.

Comment 18 Chao Yang 2012-05-23 04:58:45 UTC
Created attachment 586246 [details]
Coverity scan on qemu-kvm-293

I think this issue has been fixed according to Comment #16

Comment 19 Chao Yang 2012-05-23 05:07:14 UTC
(In reply to comment #18)
> Created attachment 586246 [details]
> Coverity scan on qemu-kvm-293
> 
> I think this issue has been fixed according to Comment #16

This issue has been fixed according to Comment #16, Comment #17

Comment 21 errata-xmlrpc 2012-06-20 11:36:16 UTC
Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

http://rhn.redhat.com/errata/RHBA-2012-0746.html


Note You need to log in before you can comment on or make changes to this bug.