netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v4 0/5] pci-hyperv: Fix race condition bugs for fast device hotplug
@ 2023-06-15  4:44 Dexuan Cui
  2023-06-15  4:44 ` [PATCH v4 1/5] PCI: hv: Fix a race condition bug in hv_pci_query_relations() Dexuan Cui
                   ` (5 more replies)
  0 siblings, 6 replies; 7+ messages in thread
From: Dexuan Cui @ 2023-06-15  4:44 UTC (permalink / raw)
  To: bhelgaas, davem, decui, edumazet, haiyangz, jakeo, kuba, kw, kys,
	leon, linux-pci, lpieralisi, mikelley, pabeni, robh, saeedm,
	wei.liu, longli, boqun.feng, ssengar, helgaas
  Cc: linux-hyperv, linux-kernel, linux-rdma, netdev, josete, simon.horman

Before the guest finishes probing a device, the host may be already starting
to remove the device. Currently there are multiple race condition bugs in the
pci-hyperv driver, which can cause the guest to panic.  The patchset fixes
the crashes.

The patchset also does some cleanup work: patch 3 removes the useless
hv_pcichild_state, and patch 4 reverts an old patch which is not really
useful (without patch 4, it would be hard to make patch 5 clean).

Patch 6 in v3 is dropped for now since it's a feature rather than a fix.
Patch 6 will be split into two patches as suggested by Lorenzo and will be
posted after the 5 patches are accepted first.

The v4 addressed Lorenzo's comments and added Lorenzo' Acks to patch
1, 3 and 5.

The v4 is based on v6.4-rc6, and can apply cleanly to the Hyper-V tree's
hyperv-fixes branch.

The patchset is also availsble in my github branch:
https://github.com/dcui/tdx/commits/decui/vpci/v6.4-rc6-vpci-v4

FYI, v3 can be found here:
https://lwn.net/ml/linux-kernel/20230420024037.5921-1-decui@microsoft.com/

Please review. Thanks!


Dexuan Cui (5):
  PCI: hv: Fix a race condition bug in hv_pci_query_relations()
  PCI: hv: Fix a race condition in hv_irq_unmask() that can cause panic
  PCI: hv: Remove the useless hv_pcichild_state from struct hv_pci_dev
  Revert "PCI: hv: Fix a timing issue which causes kdump to fail
    occasionally"
  PCI: hv: Add a per-bus mutex state_lock

 drivers/pci/controller/pci-hyperv.c | 139 ++++++++++++++++------------
 1 file changed, 82 insertions(+), 57 deletions(-)

-- 
2.25.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v4 1/5] PCI: hv: Fix a race condition bug in hv_pci_query_relations()
  2023-06-15  4:44 [PATCH v4 0/5] pci-hyperv: Fix race condition bugs for fast device hotplug Dexuan Cui
@ 2023-06-15  4:44 ` Dexuan Cui
  2023-06-15  4:44 ` [PATCH v4 2/5] PCI: hv: Fix a race condition in hv_irq_unmask() that can cause panic Dexuan Cui
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 7+ messages in thread
From: Dexuan Cui @ 2023-06-15  4:44 UTC (permalink / raw)
  To: bhelgaas, davem, decui, edumazet, haiyangz, jakeo, kuba, kw, kys,
	leon, linux-pci, lpieralisi, mikelley, pabeni, robh, saeedm,
	wei.liu, longli, boqun.feng, ssengar, helgaas
  Cc: linux-hyperv, linux-kernel, linux-rdma, netdev, josete,
	simon.horman, stable

Since day 1 of the driver, there has been a race between
hv_pci_query_relations() and survey_child_resources(): during fast
device hotplug, hv_pci_query_relations() may error out due to
device-remove and the stack variable 'comp' is no longer valid;
however, pci_devices_present_work() -> survey_child_resources() ->
complete() may be running on another CPU and accessing the no-longer-valid
'comp'. Fix the race by flushing the workqueue before we exit from
hv_pci_query_relations().

Fixes: 4daace0d8ce8 ("PCI: hv: Add paravirtual PCI front-end for Microsoft Hyper-V VMs")
Signed-off-by: Dexuan Cui <decui@microsoft.com>
Reviewed-by: Michael Kelley <mikelley@microsoft.com>
Acked-by: Lorenzo Pieralisi <lpieralisi@kernel.org>
Cc: stable@vger.kernel.org
---

v2:
  Removed the "debug code".
  No change to the patch body.
  Added Cc:stable

v3:
  Added Michael's Reviewed-by.

v4:
  Provided more details of the issue in the comment and commit log.
  Added Lorenzo's Acked-by.

 drivers/pci/controller/pci-hyperv.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
index bc32662c6bb7f..ea8862e656b68 100644
--- a/drivers/pci/controller/pci-hyperv.c
+++ b/drivers/pci/controller/pci-hyperv.c
@@ -3401,6 +3401,24 @@ static int hv_pci_query_relations(struct hv_device *hdev)
 	if (!ret)
 		ret = wait_for_response(hdev, &comp);
 
+	/*
+	 * In the case of fast device addition/removal, it's possible that
+	 * vmbus_sendpacket() or wait_for_response() returns -ENODEV but we
+	 * already got a PCI_BUS_RELATIONS* message from the host and the
+	 * channel callback already scheduled a work to hbus->wq, which can be
+	 * running pci_devices_present_work() -> survey_child_resources() ->
+	 * complete(&hbus->survey_event), even after hv_pci_query_relations()
+	 * exits and the stack variable 'comp' is no longer valid; as a result,
+	 * a hang or a page fault may happen when the complete() calls
+	 * raw_spin_lock_irqsave(). Flush hbus->wq before we exit from
+	 * hv_pci_query_relations() to avoid the issues. Note: if 'ret' is
+	 * -ENODEV, there can't be any more work item scheduled to hbus->wq
+	 * after the flush_workqueue(): see vmbus_onoffer_rescind() ->
+	 * vmbus_reset_channel_cb(), vmbus_rescind_cleanup() ->
+	 * channel->rescind = true.
+	 */
+	flush_workqueue(hbus->wq);
+
 	return ret;
 }
 
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH v4 2/5] PCI: hv: Fix a race condition in hv_irq_unmask() that can cause panic
  2023-06-15  4:44 [PATCH v4 0/5] pci-hyperv: Fix race condition bugs for fast device hotplug Dexuan Cui
  2023-06-15  4:44 ` [PATCH v4 1/5] PCI: hv: Fix a race condition bug in hv_pci_query_relations() Dexuan Cui
@ 2023-06-15  4:44 ` Dexuan Cui
  2023-06-15  4:44 ` [PATCH v4 3/5] PCI: hv: Remove the useless hv_pcichild_state from struct hv_pci_dev Dexuan Cui
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 7+ messages in thread
From: Dexuan Cui @ 2023-06-15  4:44 UTC (permalink / raw)
  To: bhelgaas, davem, decui, edumazet, haiyangz, jakeo, kuba, kw, kys,
	leon, linux-pci, lpieralisi, mikelley, pabeni, robh, saeedm,
	wei.liu, longli, boqun.feng, ssengar, helgaas
  Cc: linux-hyperv, linux-kernel, linux-rdma, netdev, josete,
	simon.horman, stable

When the host tries to remove a PCI device, the host first sends a
PCI_EJECT message to the guest, and the guest is supposed to gracefully
remove the PCI device and send a PCI_EJECTION_COMPLETE message to the host;
the host then sends a VMBus message CHANNELMSG_RESCIND_CHANNELOFFER to
the guest (when the guest receives this message, the device is already
unassigned from the guest) and the guest can do some final cleanup work;
if the guest fails to respond to the PCI_EJECT message within one minute,
the host sends the VMBus message CHANNELMSG_RESCIND_CHANNELOFFER and
removes the PCI device forcibly.

In the case of fast device addition/removal, it's possible that the PCI
device driver is still configuring MSI-X interrupts when the guest receives
the PCI_EJECT message; the channel callback calls hv_pci_eject_device(),
which sets hpdev->state to hv_pcichild_ejecting, and schedules a work
hv_eject_device_work(); if the PCI device driver is calling
pci_alloc_irq_vectors() -> ... -> hv_compose_msi_msg(), we can break the
while loop in hv_compose_msi_msg() due to the updated hpdev->state, and
leave data->chip_data with its default value of NULL; later, when the PCI
device driver calls request_irq() -> ... -> hv_irq_unmask(), the guest
crashes in hv_arch_irq_unmask() due to data->chip_data being NULL.

Fix the issue by not testing hpdev->state in the while loop: when the
guest receives PCI_EJECT, the device is still assigned to the guest, and
the guest has one minute to finish the device removal gracefully. We don't
really need to (and we should not) test hpdev->state in the loop.

Fixes: de0aa7b2f97d ("PCI: hv: Fix 2 hang issues in hv_compose_msi_msg()")
Signed-off-by: Dexuan Cui <decui@microsoft.com>
Reviewed-by: Michael Kelley <mikelley@microsoft.com>
Cc: stable@vger.kernel.org
---

v2:
  Removed the "debug code".
  No change to the patch body.
  Added Cc:stable

v3:
  Added Michael's Reviewed-by.

v4:
  No change since v3.

 drivers/pci/controller/pci-hyperv.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
index ea8862e656b68..733637d967654 100644
--- a/drivers/pci/controller/pci-hyperv.c
+++ b/drivers/pci/controller/pci-hyperv.c
@@ -635,6 +635,11 @@ static void hv_arch_irq_unmask(struct irq_data *data)
 	pbus = pdev->bus;
 	hbus = container_of(pbus->sysdata, struct hv_pcibus_device, sysdata);
 	int_desc = data->chip_data;
+	if (!int_desc) {
+		dev_warn(&hbus->hdev->device, "%s() can not unmask irq %u\n",
+			 __func__, data->irq);
+		return;
+	}
 
 	local_irq_save(flags);
 
@@ -2004,12 +2009,6 @@ static void hv_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
 		hv_pci_onchannelcallback(hbus);
 		spin_unlock_irqrestore(&channel->sched_lock, flags);
 
-		if (hpdev->state == hv_pcichild_ejecting) {
-			dev_err_once(&hbus->hdev->device,
-				     "the device is being ejected\n");
-			goto enable_tasklet;
-		}
-
 		udelay(100);
 	}
 
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH v4 3/5] PCI: hv: Remove the useless hv_pcichild_state from struct hv_pci_dev
  2023-06-15  4:44 [PATCH v4 0/5] pci-hyperv: Fix race condition bugs for fast device hotplug Dexuan Cui
  2023-06-15  4:44 ` [PATCH v4 1/5] PCI: hv: Fix a race condition bug in hv_pci_query_relations() Dexuan Cui
  2023-06-15  4:44 ` [PATCH v4 2/5] PCI: hv: Fix a race condition in hv_irq_unmask() that can cause panic Dexuan Cui
@ 2023-06-15  4:44 ` Dexuan Cui
  2023-06-15  4:44 ` [PATCH v4 4/5] Revert "PCI: hv: Fix a timing issue which causes kdump to fail occasionally" Dexuan Cui
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 7+ messages in thread
From: Dexuan Cui @ 2023-06-15  4:44 UTC (permalink / raw)
  To: bhelgaas, davem, decui, edumazet, haiyangz, jakeo, kuba, kw, kys,
	leon, linux-pci, lpieralisi, mikelley, pabeni, robh, saeedm,
	wei.liu, longli, boqun.feng, ssengar, helgaas
  Cc: linux-hyperv, linux-kernel, linux-rdma, netdev, josete,
	simon.horman, stable

The hpdev->state is never really useful. The only use in
hv_pci_eject_device() and hv_eject_device_work() is not really necessary.

Signed-off-by: Dexuan Cui <decui@microsoft.com>
Reviewed-by: Michael Kelley <mikelley@microsoft.com>
Acked-by: Lorenzo Pieralisi <lpieralisi@kernel.org>
Cc: stable@vger.kernel.org
---

v2:
  No change to the patch body.
  Added Cc:stable

v3:
  Added Michael's Reviewed-by.

v3:
  Added Lorenzo's Acked-by.

 drivers/pci/controller/pci-hyperv.c | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
index 733637d967654..a826b41c949a1 100644
--- a/drivers/pci/controller/pci-hyperv.c
+++ b/drivers/pci/controller/pci-hyperv.c
@@ -545,19 +545,10 @@ struct hv_dr_state {
 	struct hv_pcidev_description func[];
 };
 
-enum hv_pcichild_state {
-	hv_pcichild_init = 0,
-	hv_pcichild_requirements,
-	hv_pcichild_resourced,
-	hv_pcichild_ejecting,
-	hv_pcichild_maximum
-};
-
 struct hv_pci_dev {
 	/* List protected by pci_rescan_remove_lock */
 	struct list_head list_entry;
 	refcount_t refs;
-	enum hv_pcichild_state state;
 	struct pci_slot *pci_slot;
 	struct hv_pcidev_description desc;
 	bool reported_missing;
@@ -2843,8 +2834,6 @@ static void hv_eject_device_work(struct work_struct *work)
 	hpdev = container_of(work, struct hv_pci_dev, wrk);
 	hbus = hpdev->hbus;
 
-	WARN_ON(hpdev->state != hv_pcichild_ejecting);
-
 	/*
 	 * 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
@@ -2901,7 +2890,6 @@ static void hv_pci_eject_device(struct hv_pci_dev *hpdev)
 		return;
 	}
 
-	hpdev->state = hv_pcichild_ejecting;
 	get_pcichild(hpdev);
 	INIT_WORK(&hpdev->wrk, hv_eject_device_work);
 	queue_work(hbus->wq, &hpdev->wrk);
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH v4 4/5] Revert "PCI: hv: Fix a timing issue which causes kdump to fail occasionally"
  2023-06-15  4:44 [PATCH v4 0/5] pci-hyperv: Fix race condition bugs for fast device hotplug Dexuan Cui
                   ` (2 preceding siblings ...)
  2023-06-15  4:44 ` [PATCH v4 3/5] PCI: hv: Remove the useless hv_pcichild_state from struct hv_pci_dev Dexuan Cui
@ 2023-06-15  4:44 ` Dexuan Cui
  2023-06-15  4:44 ` [PATCH v4 5/5] PCI: hv: Add a per-bus mutex state_lock Dexuan Cui
  2023-06-18  3:06 ` [PATCH v4 0/5] pci-hyperv: Fix race condition bugs for fast device hotplug Wei Liu
  5 siblings, 0 replies; 7+ messages in thread
From: Dexuan Cui @ 2023-06-15  4:44 UTC (permalink / raw)
  To: bhelgaas, davem, decui, edumazet, haiyangz, jakeo, kuba, kw, kys,
	leon, linux-pci, lpieralisi, mikelley, pabeni, robh, saeedm,
	wei.liu, longli, boqun.feng, ssengar, helgaas
  Cc: linux-hyperv, linux-kernel, linux-rdma, netdev, josete,
	simon.horman, stable

This reverts commit d6af2ed29c7c1c311b96dac989dcb991e90ee195.

The statement "the hv_pci_bus_exit() call releases structures of all its
child devices" in commit d6af2ed29c7c is not true: in the path
hv_pci_probe() -> hv_pci_enter_d0() -> hv_pci_bus_exit(hdev, true): the
parameter "keep_devs" is true, so hv_pci_bus_exit() does *not* release the
child "struct hv_pci_dev *hpdev" that is created earlier in
pci_devices_present_work() -> new_pcichild_device().

The commit d6af2ed29c7c was originally made in July 2020 for RHEL 7.7,
where the old version of hv_pci_bus_exit() was used; when the commit was
rebased and merged into the upstream, people didn't notice that it's
not really necessary. The commit itself doesn't cause any issue, but it
makes hv_pci_probe() more complicated. Revert it to facilitate some
upcoming changes to hv_pci_probe().

Signed-off-by: Dexuan Cui <decui@microsoft.com>
Reviewed-by: Michael Kelley <mikelley@microsoft.com>
Acked-by: Wei Hu <weh@microsoft.com>
Cc: stable@vger.kernel.org
---

v2:
  No change to the patch body.
  Added Wei Hu's Acked-by.
  Added Cc:stable

v3:
  Added Michael's Reviewed-by.

v4:
  NO change since v3.

 drivers/pci/controller/pci-hyperv.c | 71 ++++++++++++++---------------
 1 file changed, 34 insertions(+), 37 deletions(-)

diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
index a826b41c949a1..1a5296fad1c48 100644
--- a/drivers/pci/controller/pci-hyperv.c
+++ b/drivers/pci/controller/pci-hyperv.c
@@ -3318,8 +3318,10 @@ static int hv_pci_enter_d0(struct hv_device *hdev)
 	struct pci_bus_d0_entry *d0_entry;
 	struct hv_pci_compl comp_pkt;
 	struct pci_packet *pkt;
+	bool retry = true;
 	int ret;
 
+enter_d0_retry:
 	/*
 	 * Tell the host that the bus is ready to use, and moved into the
 	 * powered-on state.  This includes telling the host which region
@@ -3346,6 +3348,38 @@ static int hv_pci_enter_d0(struct hv_device *hdev)
 	if (ret)
 		goto exit;
 
+	/*
+	 * In certain case (Kdump) the pci device of interest was
+	 * not cleanly shut down and resource is still held on host
+	 * side, the host could return invalid device status.
+	 * We need to explicitly request host to release the resource
+	 * and try to enter D0 again.
+	 */
+	if (comp_pkt.completion_status < 0 && retry) {
+		retry = false;
+
+		dev_err(&hdev->device, "Retrying D0 Entry\n");
+
+		/*
+		 * Hv_pci_bus_exit() calls hv_send_resource_released()
+		 * to free up resources of its child devices.
+		 * In the kdump kernel we need to set the
+		 * wslot_res_allocated to 255 so it scans all child
+		 * devices to release resources allocated in the
+		 * normal kernel before panic happened.
+		 */
+		hbus->wslot_res_allocated = 255;
+
+		ret = hv_pci_bus_exit(hdev, true);
+
+		if (ret == 0) {
+			kfree(pkt);
+			goto enter_d0_retry;
+		}
+		dev_err(&hdev->device,
+			"Retrying D0 failed with ret %d\n", ret);
+	}
+
 	if (comp_pkt.completion_status < 0) {
 		dev_err(&hdev->device,
 			"PCI Pass-through VSP failed D0 Entry with status %x\n",
@@ -3591,7 +3625,6 @@ static int hv_pci_probe(struct hv_device *hdev,
 	struct hv_pcibus_device *hbus;
 	u16 dom_req, dom;
 	char *name;
-	bool enter_d0_retry = true;
 	int ret;
 
 	bridge = devm_pci_alloc_host_bridge(&hdev->device, 0);
@@ -3708,47 +3741,11 @@ static int hv_pci_probe(struct hv_device *hdev,
 	if (ret)
 		goto free_fwnode;
 
-retry:
 	ret = hv_pci_query_relations(hdev);
 	if (ret)
 		goto free_irq_domain;
 
 	ret = hv_pci_enter_d0(hdev);
-	/*
-	 * In certain case (Kdump) the pci device of interest was
-	 * not cleanly shut down and resource is still held on host
-	 * side, the host could return invalid device status.
-	 * We need to explicitly request host to release the resource
-	 * and try to enter D0 again.
-	 * Since the hv_pci_bus_exit() call releases structures
-	 * of all its child devices, we need to start the retry from
-	 * hv_pci_query_relations() call, requesting host to send
-	 * the synchronous child device relations message before this
-	 * information is needed in hv_send_resources_allocated()
-	 * call later.
-	 */
-	if (ret == -EPROTO && enter_d0_retry) {
-		enter_d0_retry = false;
-
-		dev_err(&hdev->device, "Retrying D0 Entry\n");
-
-		/*
-		 * Hv_pci_bus_exit() calls hv_send_resources_released()
-		 * to free up resources of its child devices.
-		 * In the kdump kernel we need to set the
-		 * wslot_res_allocated to 255 so it scans all child
-		 * devices to release resources allocated in the
-		 * normal kernel before panic happened.
-		 */
-		hbus->wslot_res_allocated = 255;
-		ret = hv_pci_bus_exit(hdev, true);
-
-		if (ret == 0)
-			goto retry;
-
-		dev_err(&hdev->device,
-			"Retrying D0 failed with ret %d\n", ret);
-	}
 	if (ret)
 		goto free_irq_domain;
 
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH v4 5/5] PCI: hv: Add a per-bus mutex state_lock
  2023-06-15  4:44 [PATCH v4 0/5] pci-hyperv: Fix race condition bugs for fast device hotplug Dexuan Cui
                   ` (3 preceding siblings ...)
  2023-06-15  4:44 ` [PATCH v4 4/5] Revert "PCI: hv: Fix a timing issue which causes kdump to fail occasionally" Dexuan Cui
@ 2023-06-15  4:44 ` Dexuan Cui
  2023-06-18  3:06 ` [PATCH v4 0/5] pci-hyperv: Fix race condition bugs for fast device hotplug Wei Liu
  5 siblings, 0 replies; 7+ messages in thread
From: Dexuan Cui @ 2023-06-15  4:44 UTC (permalink / raw)
  To: bhelgaas, davem, decui, edumazet, haiyangz, jakeo, kuba, kw, kys,
	leon, linux-pci, lpieralisi, mikelley, pabeni, robh, saeedm,
	wei.liu, longli, boqun.feng, ssengar, helgaas
  Cc: linux-hyperv, linux-kernel, linux-rdma, netdev, josete,
	simon.horman, stable

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@microsoft.com>
Reviewed-by: Michael Kelley <mikelley@microsoft.com>
Acked-by: Lorenzo Pieralisi <lpieralisi@kernel.org>
Cc: stable@vger.kernel.org
---

v2:
  Removed the "debug code".
  Fixed the "goto out" in hv_pci_resume() [Michael Kelley]
  Added Cc:stable

v3:
  Added Michael's Reviewed-by.

v4:
  Added Lorenzo's Acked-by.

 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 1a5296fad1c48..2d93d0c4f10db 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;
@@ -2605,6 +2608,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) {
@@ -2686,6 +2691,8 @@ static void pci_devices_present_work(struct work_struct *work)
 		break;
 	}
 
+	mutex_unlock(&hbus->state_lock);
+
 	kfree(dr);
 }
 
@@ -2834,6 +2841,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
@@ -2870,6 +2879,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);
 }
 
 /**
@@ -3636,6 +3647,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;
 
@@ -3745,9 +3757,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)
@@ -3765,12 +3779,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:
@@ -4020,20 +4037,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


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH v4 0/5] pci-hyperv: Fix race condition bugs for fast device hotplug
  2023-06-15  4:44 [PATCH v4 0/5] pci-hyperv: Fix race condition bugs for fast device hotplug Dexuan Cui
                   ` (4 preceding siblings ...)
  2023-06-15  4:44 ` [PATCH v4 5/5] PCI: hv: Add a per-bus mutex state_lock Dexuan Cui
@ 2023-06-18  3:06 ` Wei Liu
  5 siblings, 0 replies; 7+ messages in thread
From: Wei Liu @ 2023-06-18  3:06 UTC (permalink / raw)
  To: Dexuan Cui
  Cc: bhelgaas, davem, edumazet, haiyangz, jakeo, kuba, kw, kys, leon,
	linux-pci, lpieralisi, mikelley, pabeni, robh, saeedm, wei.liu,
	longli, boqun.feng, ssengar, helgaas, linux-hyperv, linux-kernel,
	linux-rdma, netdev, josete, simon.horman

On Wed, Jun 14, 2023 at 09:44:46PM -0700, Dexuan Cui wrote:
> Before the guest finishes probing a device, the host may be already starting
> to remove the device. Currently there are multiple race condition bugs in the
> pci-hyperv driver, which can cause the guest to panic.  The patchset fixes
> the crashes.
> 
> The patchset also does some cleanup work: patch 3 removes the useless
> hv_pcichild_state, and patch 4 reverts an old patch which is not really
> useful (without patch 4, it would be hard to make patch 5 clean).
> 
> Patch 6 in v3 is dropped for now since it's a feature rather than a fix.
> Patch 6 will be split into two patches as suggested by Lorenzo and will be
> posted after the 5 patches are accepted first.
> 
> The v4 addressed Lorenzo's comments and added Lorenzo' Acks to patch
> 1, 3 and 5.
> 
> The v4 is based on v6.4-rc6, and can apply cleanly to the Hyper-V tree's
> hyperv-fixes branch.
> 
> The patchset is also availsble in my github branch:
> https://github.com/dcui/tdx/commits/decui/vpci/v6.4-rc6-vpci-v4
> 
> FYI, v3 can be found here:
> https://lwn.net/ml/linux-kernel/20230420024037.5921-1-decui@microsoft.com/
> 
> Please review. Thanks!
> 
> 
> Dexuan Cui (5):
>   PCI: hv: Fix a race condition bug in hv_pci_query_relations()
>   PCI: hv: Fix a race condition in hv_irq_unmask() that can cause panic
>   PCI: hv: Remove the useless hv_pcichild_state from struct hv_pci_dev
>   Revert "PCI: hv: Fix a timing issue which causes kdump to fail
>     occasionally"
>   PCI: hv: Add a per-bus mutex state_lock

Applied to hyperv-fixes. Thanks.

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2023-06-18  3:06 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-06-15  4:44 [PATCH v4 0/5] pci-hyperv: Fix race condition bugs for fast device hotplug Dexuan Cui
2023-06-15  4:44 ` [PATCH v4 1/5] PCI: hv: Fix a race condition bug in hv_pci_query_relations() Dexuan Cui
2023-06-15  4:44 ` [PATCH v4 2/5] PCI: hv: Fix a race condition in hv_irq_unmask() that can cause panic Dexuan Cui
2023-06-15  4:44 ` [PATCH v4 3/5] PCI: hv: Remove the useless hv_pcichild_state from struct hv_pci_dev Dexuan Cui
2023-06-15  4:44 ` [PATCH v4 4/5] Revert "PCI: hv: Fix a timing issue which causes kdump to fail occasionally" Dexuan Cui
2023-06-15  4:44 ` [PATCH v4 5/5] PCI: hv: Add a per-bus mutex state_lock Dexuan Cui
2023-06-18  3:06 ` [PATCH v4 0/5] pci-hyperv: Fix race condition bugs for fast device hotplug Wei Liu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).