linux-kernel - Re: [PATCH v2] 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] [day] [month] [year] [list]

Message-ID: <CAErSpo5545u1pHoDnAs-F=xgdLYs+xu_XtSixDi1+UcGj2zPVg@mail.gmail.com>
Date:	Wed, 15 Jan 2014 13:10:39 -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 v2] vfio-pci: Use pci "try" reset interface

On Tue, Jan 14, 2014 at 8:45 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>
> ---
>
> v2: Update error message to make it clear that device reset failed
>     and change back to pr_warn to make sure it's visible.  I'll
>     address whether dev_warn would be more appropriate in future
>     updates.
>
> Bjorn, if this looks good and you don't mind, please take this in
> through your tree so that it syncs with the introduction of the
> pci_try_reset interfaces.  Thanks!

I applied this to pci/reset (along with the try_reset implementation)
and pushed it to my "next" branch, thanks!

>  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..2319d20 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_warn("%s: Failed to reset device %s (%d)\n",
> +                               __func__, dev_name(&pdev->dev), ret);
>         }
>
>         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-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/