[PATCH] pci_claim_release: verify for non-null pci_dev

[Veaceslav Falico]

We might not get a valid pci device from pci_get_slot(), and thus panic:

[ 95.167003] RIP: 0010:[<ffffffffa000178f>] [<ffffffffa000178f>] jailhouse_pci_do_all_devices+0xaf/0x1a0 [jailhouse]
...
[ 95.167003] [<ffffffffa0000363>] jailhouse_cell_delete_all+0x33/0xd0 [jailhouse]
[ 95.167003] [<ffffffffa0000be5>] jailhouse_cmd_disable+0xe5/0x120 [jailhouse]
[ 95.167003] [<ffffffffa00011d5>] jailhouse_ioctl+0x65/0x90 [jailhouse]
...

Fix by verifying for non-null device.

Signed-off-by: Veaceslav Falico <veacesla...@huawei.com>
---

Notes:
Reproduced easily:
tools/jailhouse enable configs/qemu-vm.cell
tools/jailhouse cell create configs/tiny-demo.cell
tools/jailhouse cell load tiny-demo inmates/demos/x86/32-bit-demo.bin -a 0xf0000
tools/jailhouse cell start tiny-demo
tools/jailhouse disable

with qemu 2.3.90:
sudo /usr/local/bin/qemu-system-x86_64 \
-m 1G -drive file=$WD/fed22.img,if=virtio -smp 4 \
-name jailhouse -machine q35 -enable-kvm \
-cpu kvm64,-kvm_pv_eoi,-kvm_steal_time,-kvm_asyncpf,-kvmclock,+vmx,+x2apic \
-virtfs local,path=/home/vfalico/git/,mount_tag=git,security_model=none -s \
-netdev type=tap,script=$WD/qemu-ifup.sh,downscript=no,id=net0 \
-device virtio-net-pci,netdev=net0 \
-serial vc -serial vc

driver/pci.c | 2 ++
1 file changed, 2 insertions(+)

diff --git a/driver/pci.c b/driver/pci.c
index 24158ac..6fcf7de 100644
--- a/driver/pci.c
+++ b/driver/pci.c
@@ -86,6 +86,8 @@ static void jailhouse_pci_claim_release(const struct jailhouse_pci_device *dev,
if (!bus)
return;
l_dev = pci_get_slot(bus, dev->bdf & 0xff);
+ if (!l_dev)
+ return;
drv = l_dev->dev.driver;

if (action == JAILHOUSE_PCI_ACTION_CLAIM) {
--
2.4.3

[Henning Schild]

Good spot. Should be applied to be safe on the Linux side.

But that looks like you are using a broken config, mentioning a
non-existing device?

Henning

[Jan Kiszka]

On 2015-07-15 17:17, Henning Schild wrote:
> Good spot. Should be applied to be safe on the Linux side.
>
> But that looks like you are using a broken config, mentioning a
> non-existing device?

Yeah, I would bet so as well. A quick attempt to reproduced didn't succeed.

Jan

--
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

[Veaceslav Falico]

On Wed, Jul 15, 2015 at 05:18:29PM +0200, Jan Kiszka wrote:
>On 2015-07-15 17:17, Henning Schild wrote:
>> Good spot. Should be applied to be safe on the Linux side.
>>
>> But that looks like you are using a broken config, mentioning a
>> non-existing device?
>
>Yeah, I would bet so as well. A quick attempt to reproduced didn't succeed.

Yeah, I do actually :). Sorry for the confusion - I'm pretty new to the
underlying magic. Anyway, I don't have bdf == 0xd8, or ICH HD audio.

What's interesting, though, is that I also don't have the e1000 one (which
comes before snd) - but I guess it's filtered out on the pci_find_bus()
stage.

[Veaceslav Falico]

On Wed, Jul 15, 2015 at 05:34:15PM +0200, Veaceslav Falico wrote:
>On Wed, Jul 15, 2015 at 05:18:29PM +0200, Jan Kiszka wrote:
>>On 2015-07-15 17:17, Henning Schild wrote:
>>>Good spot. Should be applied to be safe on the Linux side.
>>>
>>>But that looks like you are using a broken config, mentioning a
>>>non-existing device?
>>
>>Yeah, I would bet so as well. A quick attempt to reproduced didn't succeed.
>
>Yeah, I do actually :). Sorry for the confusion - I'm pretty new to the
>underlying magic. Anyway, I don't have bdf == 0xd8, or ICH HD audio.
>
>What's interesting, though, is that I also don't have the e1000 one (which
>comes before snd) - but I guess it's filtered out on the pci_find_bus()
>stage.

Actually not, it's just that on this exactly bdf resides virtfs
(1af4:1009) - so the pci_dev is found, even though it's not e1000...

[Jan Kiszka]

On 2015-07-15 17:41, Veaceslav Falico wrote:
> On Wed, Jul 15, 2015 at 05:34:15PM +0200, Veaceslav Falico wrote:
>> On Wed, Jul 15, 2015 at 05:18:29PM +0200, Jan Kiszka wrote:
>>> On 2015-07-15 17:17, Henning Schild wrote:
>>>> Good spot. Should be applied to be safe on the Linux side.
>>>>
>>>> But that looks like you are using a broken config, mentioning a
>>>> non-existing device?
>>>
>>> Yeah, I would bet so as well. A quick attempt to reproduced didn't
>>> succeed.
>>
>> Yeah, I do actually :). Sorry for the confusion - I'm pretty new to the
>> underlying magic. Anyway, I don't have bdf == 0xd8, or ICH HD audio.
>>
>> What's interesting, though, is that I also don't have the e1000 one
>> (which
>> comes before snd) - but I guess it's filtered out on the pci_find_bus()
>> stage.
>
> Actually not, it's just that on this exactly bdf resides virtfs
> (1af4:1009) - so the pci_dev is found, even though it's not e1000...

Well, I think you were somehow lucky that the variation of the "real" hw
and the config "only" triggered this crash and not a full lock-up on
hypervisor activation.

However - having a validity check is better than panic'ing. Applied your
patch to next, thanks!

Jan