[RFC v2 0/2] vmxnet3: Fix inconsistent DMA accesses

STALE699d

Revision v2 of 2 in this series.

3 messages, 1 author, 2024-11-19 · open the first message on its own page

[RFC v2 0/2] vmxnet3: Fix inconsistent DMA accesses

From: Brian Johannesmeyer <hidden>
Date: 2024-11-19 22:13:58 

We identified hundreds of inconsistent DMA accesses in the VMXNET3 driver,
stemming from the handling of the `adapter` object. This RFC patch series
proposes two alternative fixes for this issue. For an overview of rules
related to streaming DMA access (violations of which lead to inconsistent
accesses), see Figure 4a in [0].

*** Issue ***
The inconsistent accesses occur because the `adapter` object is mapped into
streaming DMA, which means it is "owned" by the device. Any subsequent CPU
access to `adapter` without prior synchronization (e.g.,
`dma_sync_single_for_cpu()`) may cause unexpected hardware behaviors.

*** Patch overview ***
This series includes two mutually exclusive patches:

1. **Patch 1**: Apply if `adapter` *should* be mapped to DMA.
    - Adds synchronization operations in `vmxnet3_probe_device()` to ensure
      consistent accesses when initializing `adapter`.
    - Note: This patch does not cover all instances of inconsistent
      accesses, so similar synchronization will need to be applied
throughout the driver where `adapter` is accessed.

2. **Patch 2**: Apply if `adapter` *should not* be mapped to DMA.
    - Removes `adapter` from streaming DMA entirely, preventing any
      potential inconsistent accesses.
    - It is unclear why `adapter` was mapped to DMA in [1], as it did not
      appear to be mapped before this commit. I welcome any insights into
why this change was originally made, as I am not deeply familiar with
VMXNET3 internals.

*** Request for Feedback ***
Only one of these patches should be applied, and I am seeking feedback to
determine which approach is correct. Thank you for your time and feedback.

*** Changes ***
- Changes vs. v1 [2]: Jakub points out that applying both patch 1 *and*
  patch 2 is odd. Thus, I changed the patch series to apply patch 1 *or*
patch 2. I also made other minor fixes based on his reminder to run
checkpatch.

[0] Link: https://www.usenix.org/system/files/sec21-bai.pdf
[1] commit b0eb57cb97e7837ebb746404c2c58c6f536f23fa ("VMXNET3: Add support
for virtual IOMMU")
[2] Link:
https://lore.kernel.org/lkml/20241113200001.3567479-1-bjohannesmeyer@gmail.com/

Brian Johannesmeyer (1):
  vmxnet3: Fix inconsistent DMA accesses in vmxnet3_probe_device()

 drivers/net/vmxnet3/vmxnet3_drv.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

-- 
2.34.1

[RFC v2 2/2] vmxnet3: Remove adapter from DMA region

From: Brian Johannesmeyer <hidden>
Date: 2024-11-19 22:14:00 

Revert parts of [0] that map `adapter` into a streaming DMA region. Also
revert any other DMA-related uses of `adapter`. Doing so mitigates all
inconsistent accesses to it.

[0] commit b0eb57cb97e7837ebb746404c2c58c6f536f23fa ("VMXNET3: Add support
for virtual IOMMU")

Co-developed-by: Raphael Isemann <redacted>
Signed-off-by: Raphael Isemann <redacted>
Signed-off-by: Brian Johannesmeyer <redacted>
---
 drivers/net/vmxnet3/vmxnet3_drv.c | 17 ++---------------
 drivers/net/vmxnet3/vmxnet3_int.h |  1 -
 2 files changed, 2 insertions(+), 16 deletions(-)

diff --git a/drivers/net/vmxnet3/vmxnet3_drv.c b/drivers/net/vmxnet3/vmxnet3_drv.c
index 7fa74b8b2100..5219992f6a63 100644
--- a/drivers/net/vmxnet3/vmxnet3_drv.c
+++ b/drivers/net/vmxnet3/vmxnet3_drv.c
@@ -2605,7 +2605,7 @@ vmxnet3_setup_driver_shared(struct vmxnet3_adapter *adapter)
 	devRead->misc.driverInfo.vmxnet3RevSpt = cpu_to_le32(1);
 	devRead->misc.driverInfo.uptVerSpt = cpu_to_le32(1);
 
-	devRead->misc.ddPA = cpu_to_le64(adapter->adapter_pa);
+	devRead->misc.ddPA = cpu_to_le64(virt_to_phys(adapter));
 	devRead->misc.ddLen = cpu_to_le32(sizeof(struct vmxnet3_adapter));
 
 	/* set up feature flags */
@@ -3662,14 +3662,6 @@ vmxnet3_probe_device(struct pci_dev *pdev,
 	}
 
 	spin_lock_init(&adapter->cmd_lock);
-	adapter->adapter_pa = dma_map_single(&adapter->pdev->dev, adapter,
-					     sizeof(struct vmxnet3_adapter),
-					     DMA_TO_DEVICE);
-	if (dma_mapping_error(&adapter->pdev->dev, adapter->adapter_pa)) {
-		dev_err(&pdev->dev, "Failed to map dma\n");
-		err = -EFAULT;
-		goto err_set_mask;
-	}
 	adapter->shared = dma_alloc_coherent(
 				&adapter->pdev->dev,
 				sizeof(struct Vmxnet3_DriverShared),
@@ -3677,7 +3669,7 @@ vmxnet3_probe_device(struct pci_dev *pdev,
 	if (!adapter->shared) {
 		dev_err(&pdev->dev, "Failed to allocate memory\n");
 		err = -ENOMEM;
-		goto err_alloc_shared;
+		goto err_set_mask;
 	}
 
 	err = vmxnet3_alloc_pci_resources(adapter);
@@ -3954,9 +3946,6 @@ vmxnet3_probe_device(struct pci_dev *pdev,
 	dma_free_coherent(&adapter->pdev->dev,
 			  sizeof(struct Vmxnet3_DriverShared),
 			  adapter->shared, adapter->shared_pa);
-err_alloc_shared:
-	dma_unmap_single(&adapter->pdev->dev, adapter->adapter_pa,
-			 sizeof(struct vmxnet3_adapter), DMA_TO_DEVICE);
 err_set_mask:
 	free_netdev(netdev);
 	return err;
@@ -4023,8 +4012,6 @@ vmxnet3_remove_device(struct pci_dev *pdev)
 	dma_free_coherent(&adapter->pdev->dev,
 			  sizeof(struct Vmxnet3_DriverShared),
 			  adapter->shared, adapter->shared_pa);
-	dma_unmap_single(&adapter->pdev->dev, adapter->adapter_pa,
-			 sizeof(struct vmxnet3_adapter), DMA_TO_DEVICE);
 	free_netdev(netdev);
 }
 
diff --git a/drivers/net/vmxnet3/vmxnet3_int.h b/drivers/net/vmxnet3/vmxnet3_int.h
index 3367db23aa13..b45ed1045ca3 100644
--- a/drivers/net/vmxnet3/vmxnet3_int.h
+++ b/drivers/net/vmxnet3/vmxnet3_int.h
@@ -404,7 +404,6 @@ struct vmxnet3_adapter {
 	struct Vmxnet3_CoalesceScheme *coal_conf;
 	bool   default_coal_mode;
 
-	dma_addr_t adapter_pa;
 	dma_addr_t pm_conf_pa;
 	dma_addr_t rss_conf_pa;
 	bool   queuesExtEnabled;
-- 
2.34.1

[RFC v2 1/2] vmxnet3: Fix inconsistent DMA accesses in vmxnet3_probe_device()

From: Brian Johannesmeyer <hidden>
Date: 2024-11-19 22:14:01 

After mapping `adapter` to streaming DMA, but before accessing it,
synchronize it to the CPU. Then, before returning, synchronize it back to
the device. This mitigates any inconsistent accesses to it from
vmxnet3_probe_device().

Co-developed-by: Raphael Isemann <redacted>
Signed-off-by: Raphael Isemann <redacted>
Signed-off-by: Brian Johannesmeyer <redacted>
---
 drivers/net/vmxnet3/vmxnet3_drv.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/net/vmxnet3/vmxnet3_drv.c b/drivers/net/vmxnet3/vmxnet3_drv.c
index 7fa74b8b2100..032d3cd34be1 100644
--- a/drivers/net/vmxnet3/vmxnet3_drv.c
+++ b/drivers/net/vmxnet3/vmxnet3_drv.c
@@ -3623,6 +3623,8 @@ vmxnet3_probe_device(struct pci_dev *pdev,
 	int num_rx_queues;
 	int queues;
 	unsigned long flags;
+	struct device *dev;
+	dma_addr_t adapter_pa;
 
 	if (!pci_msi_enabled())
 		enable_mq = 0;
@@ -3662,14 +3664,19 @@ vmxnet3_probe_device(struct pci_dev *pdev,
 	}
 
 	spin_lock_init(&adapter->cmd_lock);
-	adapter->adapter_pa = dma_map_single(&adapter->pdev->dev, adapter,
-					     sizeof(struct vmxnet3_adapter),
-					     DMA_TO_DEVICE);
-	if (dma_mapping_error(&adapter->pdev->dev, adapter->adapter_pa)) {
+	dev = &adapter->pdev->dev;
+	adapter_pa = dma_map_single(dev, adapter,
+				    sizeof(struct vmxnet3_adapter),
+				    DMA_TO_DEVICE);
+	if (dma_mapping_error(dev, adapter_pa)) {
 		dev_err(&pdev->dev, "Failed to map dma\n");
 		err = -EFAULT;
 		goto err_set_mask;
 	}
+	dma_sync_single_for_cpu(dev, adapter_pa,
+				sizeof(struct vmxnet3_adapter), DMA_TO_DEVICE);
+	adapter->adapter_pa = adapter_pa;
+
 	adapter->shared = dma_alloc_coherent(
 				&adapter->pdev->dev,
 				sizeof(struct Vmxnet3_DriverShared),
@@ -3928,6 +3935,9 @@ vmxnet3_probe_device(struct pci_dev *pdev,
 	}
 
 	vmxnet3_check_link(adapter, false);
+	dma_sync_single_for_device(dev, adapter_pa,
+				   sizeof(struct vmxnet3_adapter),
+				   DMA_TO_DEVICE);
 	return 0;
 
 err_register:
-- 
2.34.1
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help