linux-kernel - Re: [Xen-devel] [RFC PATCH 5/5] xen/pciback: PCI reset slot or bus

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <52AEE548.8010800@citrix.com>
Date:	Mon, 16 Dec 2013 11:34:32 +0000
From:	David Vrabel <david.vrabel@...rix.com>
To:	Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>
CC:	<xen-devel@...ts.xenproject.org>, <linux-kernel@...r.kernel.org>,
	<gordan@...ich.net>
Subject: Re: [Xen-devel] [RFC PATCH 5/5] xen/pciback: PCI reset slot or bus

On 13/12/13 16:09, Konrad Rzeszutek Wilk wrote:
> The life-cycle of a PCI device in Xen pciback is a bit complex.
> 
> It starts with the device being binded to us - for which
> we do a device function reset.
> 
> If the device is unbinded from us - we also do a function
> reset.

Spelling: bound and unbound.

> The reset is done when all of the functions of a device
> are binded to Xen pciback. Or when a device is un-assigned
> from a guest. We do this by having a 'completion' workqueue
> on which the users of the PCI device wait. This workqueue
> is started once the device has been 'binded' or de-assigned
> from a guest.

The use of a work item and a completion baffles me.  What problem does
this solve?

> --- a/drivers/xen/xen-pciback/pci_stub.c
> +++ b/drivers/xen/xen-pciback/pci_stub.c
[...]
> +	/* We expect X amount of slots (this would also find out
> +	 * if we do not have all of the slots assigned to us).
> +	 */
> +	list_for_each_entry(pci_dev, &dev->bus->devices, bus_list)
> +		slots++;
> +
> +	spin_lock_irqsave(&pcistub_devices_lock, flags);
> +	/* Iterate over the existing devices to ascertain whether
> +	 * all of them are under the bridge and not in use. */
> +	list_for_each_entry(psdev, &pcistub_devices, dev_list) {
> +		if (!psdev->dev)
> +			continue;
> +
> +		if (pci_domain_nr(psdev->dev->bus) == pci_domain_nr(dev->bus) &&
> +		    psdev->dev->bus->number == dev->bus->number &&
> +		    PCI_SLOT(psdev->dev->devfn) == PCI_SLOT(dev->devfn)) {
> +			slots--;
> +			/* Ignore ourselves in case hadn't cleaned up yet */
> +			if (psdev->pdev && psdev->dev != dev)
> +				inuse++;
> +		}
> +	}

This check looks broken.  A device added to pciback but still bound to
another driver will be considered as safe to reset.

I think you want something like:

list_for_each_entry(pdev, &dev->bus->devices, bus_list)
{
    if (pdev != dev && pdev->driver
        && pdev->driver != xen_pcibk_pci_driver))
        return -ENOTTY;
}

It is safe to reset unbound devices (even if they're not (or intended)
to be available to pciback).

It is also possible in the above loop if slot reset is supported to
ignore sibling devices that are on different slots.

> +	spin_unlock_irqrestore(&pcistub_devices_lock, flags);
> +	/* Slots should be zero (all slots under the bridge are
> +	 * accounted for), and inuse should be zero (not assigned
> +	 * to anybody). */
> +	if (!slots && !inuse) {
> +		int rc = 0, bus = 0;
> +		list_for_each_entry(pci_dev, &dev->bus->devices, bus_list) {
> +			dev_dbg(&pci_dev->dev, "resetting the slot device\n");
> +			if (!pci_probe_reset_slot(pci_dev->slot))
> +				rc = pci_reset_slot(pci_dev->slot);
> +			else
> +				bus = 1;
> +			if (rc)
> +				dev_info(&pci_dev->dev, "resetting slot failed with %d\n", rc);
> +		}

Why are you resetting every slot on the bus?  You only need to reset the
slot that the device is on.

Take a look at the vfio-pci driver.  It does this bus/slot reset choice
in a much more straightforward way.

David
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/