| 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
| ||
|
Message-ID: <BYAPR21MB1688BE7ACC049989A3EE400ED7969@BYAPR21MB1688.namprd21.prod.outlook.com>
Date: Fri, 7 Apr 2023 16:10:31 +0000
From: "Michael Kelley (LINUX)" <mikelley@...rosoft.com>
To: Dexuan Cui <decui@...rosoft.com>,
"bhelgaas@...gle.com" <bhelgaas@...gle.com>,
"davem@...emloft.net" <davem@...emloft.net>,
"edumazet@...gle.com" <edumazet@...gle.com>,
Haiyang Zhang <haiyangz@...rosoft.com>,
Jake Oshins <jakeo@...rosoft.com>,
"kuba@...nel.org" <kuba@...nel.org>, "kw@...ux.com" <kw@...ux.com>,
KY Srinivasan <kys@...rosoft.com>,
"leon@...nel.org" <leon@...nel.org>,
"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
"lpieralisi@...nel.org" <lpieralisi@...nel.org>,
"pabeni@...hat.com" <pabeni@...hat.com>,
"robh@...nel.org" <robh@...nel.org>,
"saeedm@...dia.com" <saeedm@...dia.com>,
"wei.liu@...nel.org" <wei.liu@...nel.org>,
Long Li <longli@...rosoft.com>,
"boqun.feng@...il.com" <boqun.feng@...il.com>,
Saurabh Singh Sengar <ssengar@...rosoft.com>,
"helgaas@...nel.org" <helgaas@...nel.org>
CC: "linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-rdma@...r.kernel.org" <linux-rdma@...r.kernel.org>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"stable@...r.kernel.org" <stable@...r.kernel.org>
Subject: RE: [PATCH v2 5/6] PCI: hv: Add a per-bus mutex state_lock
From: Dexuan Cui <decui@...rosoft.com> Sent: Monday, April 3, 2023 7:06 PM
>
> In the case of fast device addition/removal, it's possible that
> hv_eject_device_work() can start to run before create_root_hv_pci_bus()
> starts to run; as a result, the pci_get_domain_bus_and_slot() in
> hv_eject_device_work() can return a 'pdev' of NULL, and
> hv_eject_device_work() can remove the 'hpdev', and immediately send a
> message PCI_EJECTION_COMPLETE to the host, and the host immediately
> unassigns the PCI device from the guest; meanwhile,
> create_root_hv_pci_bus() and the PCI device driver can be probing the
> dead PCI device and reporting timeout errors.
>
> Fix the issue by adding a per-bus mutex 'state_lock' and grabbing the
> mutex before powering on the PCI bus in hv_pci_enter_d0(): when
> hv_eject_device_work() starts to run, it's able to find the 'pdev' and call
> pci_stop_and_remove_bus_device(pdev): if the PCI device driver has
> loaded, the PCI device driver's probe() function is already called in
> create_root_hv_pci_bus() -> pci_bus_add_devices(), and now
> hv_eject_device_work() -> pci_stop_and_remove_bus_device() is able
> to call the PCI device driver's remove() function and remove the device
> reliably; if the PCI device driver hasn't loaded yet, the function call
> hv_eject_device_work() -> pci_stop_and_remove_bus_device() is able to
> remove the PCI device reliably and the PCI device driver's probe()
> function won't be called; if the PCI device driver's probe() is already
> running (e.g., systemd-udev is loading the PCI device driver), it must
> be holding the per-device lock, and after the probe() finishes and releases
> the lock, hv_eject_device_work() -> pci_stop_and_remove_bus_device() is
> able to proceed to remove the device reliably.
>
> Fixes: 4daace0d8ce8 ("PCI: hv: Add paravirtual PCI front-end for Microsoft Hyper-V VMs")
> Signed-off-by: Dexuan Cui <decui@...rosoft.com>
> Cc: stable@...r.kernel.org
> ---
>
> v2:
> Removed the "debug code".
> Fixed the "goto out" in hv_pci_resume() [Michael Kelley]
> Added Cc:stable
>
> drivers/pci/controller/pci-hyperv.c | 29 ++++++++++++++++++++++++++---
> 1 file changed, 26 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
> index 48feab095a144..3ae2f99dea8c2 100644
> --- a/drivers/pci/controller/pci-hyperv.c
> +++ b/drivers/pci/controller/pci-hyperv.c
> @@ -489,7 +489,10 @@ struct hv_pcibus_device {
> struct fwnode_handle *fwnode;
> /* Protocol version negotiated with the host */
> enum pci_protocol_version_t protocol_version;
> +
> + struct mutex state_lock;
> enum hv_pcibus_state state;
> +
> struct hv_device *hdev;
> resource_size_t low_mmio_space;
> resource_size_t high_mmio_space;
> @@ -2512,6 +2515,8 @@ static void pci_devices_present_work(struct work_struct *work)
> if (!dr)
> return;
>
> + mutex_lock(&hbus->state_lock);
> +
> /* First, mark all existing children as reported missing. */
> spin_lock_irqsave(&hbus->device_list_lock, flags);
> list_for_each_entry(hpdev, &hbus->children, list_entry) {
> @@ -2593,6 +2598,8 @@ static void pci_devices_present_work(struct work_struct *work)
> break;
> }
>
> + mutex_unlock(&hbus->state_lock);
> +
> kfree(dr);
> }
>
> @@ -2741,6 +2748,8 @@ static void hv_eject_device_work(struct work_struct *work)
> hpdev = container_of(work, struct hv_pci_dev, wrk);
> hbus = hpdev->hbus;
>
> + mutex_lock(&hbus->state_lock);
> +
> /*
> * Ejection can come before or after the PCI bus has been set up, so
> * attempt to find it and tear down the bus state, if it exists. This
> @@ -2777,6 +2786,8 @@ static void hv_eject_device_work(struct work_struct *work)
> put_pcichild(hpdev);
> put_pcichild(hpdev);
> /* hpdev has been freed. Do not use it any more. */
> +
> + mutex_unlock(&hbus->state_lock);
> }
>
> /**
> @@ -3562,6 +3573,7 @@ static int hv_pci_probe(struct hv_device *hdev,
> return -ENOMEM;
>
> hbus->bridge = bridge;
> + mutex_init(&hbus->state_lock);
> hbus->state = hv_pcibus_init;
> hbus->wslot_res_allocated = -1;
>
> @@ -3670,9 +3682,11 @@ static int hv_pci_probe(struct hv_device *hdev,
> if (ret)
> goto free_irq_domain;
>
> + mutex_lock(&hbus->state_lock);
> +
> ret = hv_pci_enter_d0(hdev);
> if (ret)
> - goto free_irq_domain;
> + goto release_state_lock;
>
> ret = hv_pci_allocate_bridge_windows(hbus);
> if (ret)
> @@ -3690,12 +3704,15 @@ static int hv_pci_probe(struct hv_device *hdev,
> if (ret)
> goto free_windows;
>
> + mutex_unlock(&hbus->state_lock);
> return 0;
>
> free_windows:
> hv_pci_free_bridge_windows(hbus);
> exit_d0:
> (void) hv_pci_bus_exit(hdev, true);
> +release_state_lock:
> + mutex_unlock(&hbus->state_lock);
> free_irq_domain:
> irq_domain_remove(hbus->irq_domain);
> free_fwnode:
> @@ -3945,20 +3962,26 @@ static int hv_pci_resume(struct hv_device *hdev)
> if (ret)
> goto out;
>
> + mutex_lock(&hbus->state_lock);
> +
> ret = hv_pci_enter_d0(hdev);
> if (ret)
> - goto out;
> + goto release_state_lock;
>
> ret = hv_send_resources_allocated(hdev);
> if (ret)
> - goto out;
> + goto release_state_lock;
>
> prepopulate_bars(hbus);
>
> hv_pci_restore_msi_state(hbus);
>
> hbus->state = hv_pcibus_installed;
> + mutex_unlock(&hbus->state_lock);
> return 0;
> +
> +release_state_lock:
> + mutex_unlock(&hbus->state_lock);
> out:
> vmbus_close(hdev->channel);
> return ret;
> --
> 2.25.1
Reviewed-by: Michael Kelley <mikelley@...rosoft.com>
Powered by blists - more mailing lists