linux-kernel - Re: [PATCH] vfio-pci: Use pci "try" reset interface

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]

Date:	Tue, 14 Jan 2014 16:48:07 -0700
From:	Bjorn Helgaas <bhelgaas@...gle.com>
To:	Alex Williamson <alex.williamson@...hat.com>
Cc:	"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"kvm@...r.kernel.org" <kvm@...r.kernel.org>
Subject: Re: [PATCH] vfio-pci: Use pci "try" reset interface

On Mon, Dec 16, 2013 at 3:16 PM, Alex Williamson
<alex.williamson@...hat.com> wrote:
> PCI resets will attempt to take the device_lock for any device to be
> reset.  This is a problem if that lock is already held, for instance
> in the device remove path.  It's not sufficient to simply kill the
> user process or skip the reset if called after .remove as a race could
> result in the same deadlock.  Instead, we handle all resets as "best
> effort" using the PCI "try" reset interfaces.  This prevents the user
> from being able to induce a deadlock by triggering a reset.
>
> Signed-off-by: Alex Williamson <alex.williamson@...hat.com>
> ---
>
> This depends on the proposed pci "try" function/slot/bus reset patch.
>
>  drivers/vfio/pci/vfio_pci.c |   29 +++++++++--------------------
>  1 file changed, 9 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> index 6ab71b9..576e34e 100644
> --- a/drivers/vfio/pci/vfio_pci.c
> +++ b/drivers/vfio/pci/vfio_pci.c
> @@ -139,25 +139,14 @@ static void vfio_pci_disable(struct vfio_pci_device *vdev)
>         pci_write_config_word(pdev, PCI_COMMAND, PCI_COMMAND_INTX_DISABLE);
>
>         /*
> -        * Careful, device_lock may already be held.  This is the case if
> -        * a driver unbind is blocked.  Try to get the locks ourselves to
> -        * prevent a deadlock.
> +        * Try to reset the device.  The success of this is dependent on
> +        * being able to lock the device, which is not always possible.
>          */
>         if (vdev->reset_works) {
> -               bool reset_done = false;
> -
> -               if (pci_cfg_access_trylock(pdev)) {
> -                       if (device_trylock(&pdev->dev)) {
> -                               __pci_reset_function_locked(pdev);
> -                               reset_done = true;
> -                               device_unlock(&pdev->dev);
> -                       }
> -                       pci_cfg_access_unlock(pdev);
> -               }
> -
> -               if (!reset_done)
> -                       pr_warn("%s: Unable to acquire locks for reset of %s\n",
> -                               __func__, dev_name(&pdev->dev));
> +               int ret = pci_try_reset_function(pdev);
> +               if (ret)
> +                       pr_debug("%s: reset of device %s = %d\n",
> +                               __func__, dev_name(&pdev->dev), ret);

Why is this a pr_debug() instead of dev_dbg()?  I see vfio_pci.c seems
to always use pr_*() instead of dev_*(), so you probably have a
reason.

The text of the message ("vfio_pci_disable: reset of device
0000:00:00.1 = -35") doesn't seem terribly clear; as a user, I'd have
to go look up the code to know whether I should be concerned about it.
 Maybe it should be explicit that the device was not reset?

>         }
>
>         pci_restore_state(pdev);
> @@ -514,7 +503,7 @@ static long vfio_pci_ioctl(void *device_data,
>
>         } else if (cmd == VFIO_DEVICE_RESET) {
>                 return vdev->reset_works ?
> -                       pci_reset_function(vdev->pdev) : -EINVAL;
> +                       pci_try_reset_function(vdev->pdev) : -EINVAL;
>
>         } else if (cmd == VFIO_DEVICE_GET_PCI_HOT_RESET_INFO) {
>                 struct vfio_pci_hot_reset_info hdr;
> @@ -684,8 +673,8 @@ reset_info_exit:
>                                                     &info, slot);
>                 if (!ret)
>                         /* User has access, do the reset */
> -                       ret = slot ? pci_reset_slot(vdev->pdev->slot) :
> -                                    pci_reset_bus(vdev->pdev->bus);
> +                       ret = slot ? pci_try_reset_slot(vdev->pdev->slot) :
> +                                    pci_try_reset_bus(vdev->pdev->bus);
>
>  hot_reset_release:
>                 for (i--; i >= 0; i--)
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
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/