[dpdk-dev] [PATCH] net/virtio: fix secondary process crash with PCI devices

[dpdk-dev] [PATCH] net/virtio: fix secondary process crash with PCI devices

[Maxime Coquelin]

The Virtio rework series mistakenly moved the rte_pci_device
pointer to struct virtio_hw, which is shared between the two
processes. But this structure is per-process, so this change
made secondary process to try accessing primary process-only
memory, leading to a crash

This patch reverts to proper behavior, by storing the
rte_pci_device pointer into the pre-process
virtio_pci_internal struct. It provides also helper to get
the pointer from the virtio_hw struct pointer.

Bugzilla ID: 633
Fixes: c8d4b02f72ae ("net/virtio: move legacy IO to virtio PCI")

Reported-by: Anatoly Burakov <anatoly.burakov@intel.com>
Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 drivers/net/virtio/virtio_pci.c        | 25 +++++--------------------
 drivers/net/virtio/virtio_pci.h        | 12 +++++++++++-
 drivers/net/virtio/virtio_pci_ethdev.c |  2 ++
 3 files changed, 18 insertions(+), 21 deletions(-)

diff --git a/drivers/net/virtio/virtio_pci.c b/drivers/net/virtio/virtio_pci.c
index d27e9eb515..c14d1339c9 100644
--- a/drivers/net/virtio/virtio_pci.c
+++ b/drivers/net/virtio/virtio_pci.c
@@ -31,13 +31,6 @@
 #define VIRTIO_PCI_CONFIG(dev) \
 		(((dev)->msix_status == VIRTIO_MSIX_ENABLED) ? 24 : 20)
 
-
-struct virtio_pci_internal {
-	struct rte_pci_ioport io;
-};
-
-#define VTPCI_IO(hw) (&virtio_pci_internal[(hw)->port_id].io)
-
 struct virtio_pci_internal virtio_pci_internal[RTE_MAX_ETHPORTS];
 
 static inline int
@@ -313,16 +306,14 @@ legacy_intr_detect(struct virtio_hw *hw)
 {
 	struct virtio_pci_dev *dev = virtio_pci_get_dev(hw);
 
-	dev->msix_status = vtpci_msix_detect(dev->pci_dev);
+	dev->msix_status = vtpci_msix_detect(VTPCI_DEV(hw));
 	hw->intr_lsc = !!dev->msix_status;
 }
 
 static int
 legacy_dev_close(struct virtio_hw *hw)
 {
-	struct virtio_pci_dev *dev = virtio_pci_get_dev(hw);
-
-	rte_pci_unmap_device(dev->pci_dev);
+	rte_pci_unmap_device(VTPCI_DEV(hw));
 	rte_pci_ioport_unmap(VTPCI_IO(hw));
 
 	return 0;
@@ -574,16 +565,14 @@ modern_intr_detect(struct virtio_hw *hw)
 {
 	struct virtio_pci_dev *dev = virtio_pci_get_dev(hw);
 
-	dev->msix_status = vtpci_msix_detect(dev->pci_dev);
+	dev->msix_status = vtpci_msix_detect(VTPCI_DEV(hw));
 	hw->intr_lsc = !!dev->msix_status;
 }
 
 static int
 modern_dev_close(struct virtio_hw *hw)
 {
-	struct virtio_pci_dev *dev = virtio_pci_get_dev(hw);
-
-	rte_pci_unmap_device(dev->pci_dev);
+	rte_pci_unmap_device(VTPCI_DEV(hw));
 
 	return 0;
 }
@@ -772,8 +761,6 @@ vtpci_init(struct rte_pci_device *pci_dev, struct virtio_pci_dev *dev)
 
 	RTE_BUILD_BUG_ON(offsetof(struct virtio_pci_dev, hw) != 0);
 
-	dev->pci_dev = pci_dev;
-
 	/*
 	 * Try if we can succeed reading virtio pci caps, which exists
 	 * only on modern pci device. If failed, we fallback to legacy
@@ -816,7 +803,5 @@ void vtpci_legacy_ioport_unmap(struct virtio_hw *hw)
 
 int vtpci_legacy_ioport_map(struct virtio_hw *hw)
 {
-	struct virtio_pci_dev *dev = virtio_pci_get_dev(hw);
-
-	return rte_pci_ioport_map(dev->pci_dev, 0, VTPCI_IO(hw));
+	return rte_pci_ioport_map(VTPCI_DEV(hw), 0, VTPCI_IO(hw));
 }
diff --git a/drivers/net/virtio/virtio_pci.h b/drivers/net/virtio/virtio_pci.h
index 57596e471f..11e25a0142 100644
--- a/drivers/net/virtio/virtio_pci.h
+++ b/drivers/net/virtio/virtio_pci.h
@@ -104,7 +104,6 @@ enum virtio_msix_status {
 
 struct virtio_pci_dev {
 	struct virtio_hw hw;
-	struct rte_pci_device *pci_dev;
 	struct virtio_pci_common_cfg *common_cfg;
 	struct virtio_net_config *dev_cfg;
 	enum virtio_msix_status msix_status;
@@ -116,6 +115,17 @@ struct virtio_pci_dev {
 
 #define virtio_pci_get_dev(hwp) container_of(hwp, struct virtio_pci_dev, hw)
 
+struct virtio_pci_internal {
+	struct rte_pci_ioport io;
+	struct rte_pci_device *dev;
+};
+
+extern struct virtio_pci_internal virtio_pci_internal[RTE_MAX_ETHPORTS];
+
+#define VTPCI_IO(hw) (&virtio_pci_internal[(hw)->port_id].io)
+#define VTPCI_DEV(hw) (virtio_pci_internal[(hw)->port_id].dev)
+
+
 /*
  * How many bits to shift physical queue address written to QUEUE_PFN.
  * 12 is historical, and due to x86 page size.
diff --git a/drivers/net/virtio/virtio_pci_ethdev.c b/drivers/net/virtio/virtio_pci_ethdev.c
index 3bb5c6268b..4083853c48 100644
--- a/drivers/net/virtio/virtio_pci_ethdev.c
+++ b/drivers/net/virtio/virtio_pci_ethdev.c
@@ -78,12 +78,14 @@ eth_virtio_pci_init(struct rte_eth_dev *eth_dev)
 
 	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
 		hw->port_id = eth_dev->data->port_id;
+		VTPCI_DEV(hw) = pci_dev;
 		ret = vtpci_init(RTE_ETH_DEV_TO_PCI(eth_dev), dev);
 		if (ret) {
 			PMD_INIT_LOG(ERR, "Failed to init PCI device\n");
 			return -1;
 		}
 	} else {
+		VTPCI_DEV(hw) = pci_dev;
 		if (dev->modern)
 			VIRTIO_OPS(hw) = &modern_ops;
 		else
-- 
2.29.2

Re: [dpdk-dev] [PATCH] net/virtio: fix secondary process crash with PCI devices

[David Marchand]

On Wed, Feb 3, 2021 at 4:58 PM Maxime Coquelin
<maxime.coquelin@redhat.com> wrote:
>
> The Virtio rework series mistakenly moved the rte_pci_device
> pointer to struct virtio_hw, which is shared between the two
> processes. But this structure is per-process, so this change
> made secondary process to try accessing primary process-only
> memory, leading to a crash

crash.*

Ah the joy of multiprocess support :-).

>
> This patch reverts to proper behavior, by storing the
> rte_pci_device pointer into the pre-process

per*

> virtio_pci_internal struct. It provides also helper to get

also provides*

> the pointer from the virtio_hw struct pointer.
>
> Bugzilla ID: 633
> Fixes: c8d4b02f72ae ("net/virtio: move legacy IO to virtio PCI")
>
> Reported-by: Anatoly Burakov <anatoly.burakov@intel.com>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>

The patch lgtm.
Reviewed-by: David Marchand <david.marchand@redhat.com>

Thanks.

-- 
David Marchand

Re: [dpdk-dev] [PATCH] net/virtio: fix secondary process crash with PCI devices

[Maxime Coquelin]

On 2/3/21 4:58 PM, Maxime Coquelin wrote:
> The Virtio rework series mistakenly moved the rte_pci_device
> pointer to struct virtio_hw, which is shared between the two
> processes. But this structure is per-process, so this change
> made secondary process to try accessing primary process-only
> memory, leading to a crash
> 
> This patch reverts to proper behavior, by storing the
> rte_pci_device pointer into the pre-process
> virtio_pci_internal struct. It provides also helper to get
> the pointer from the virtio_hw struct pointer.
> 
> Bugzilla ID: 633
> Fixes: c8d4b02f72ae ("net/virtio: move legacy IO to virtio PCI")
> 
> Reported-by: Anatoly Burakov <anatoly.burakov@intel.com>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  drivers/net/virtio/virtio_pci.c        | 25 +++++--------------------
>  drivers/net/virtio/virtio_pci.h        | 12 +++++++++++-
>  drivers/net/virtio/virtio_pci_ethdev.c |  2 ++
>  3 files changed, 18 insertions(+), 21 deletions(-)
> 

Applied to dpdk-next-virtio/main with typo fixed.

Thanks,
Maxime