[PATCH 0/2] vmxnet3: Fix inconsistent DMA accesses

[PATCH 0/2] vmxnet3: Fix inconsistent DMA accesses

[Brian Johannesmeyer]

Hello,

We found hundreds of inconsistent DMA accesses in the VMXNET3 driver. This
patch series aims to fix them. (For a nice summary of the rules around
accessing streaming DMA --- which, if violated, result in inconsistent
accesses --- see Figure 4a of this paper [0]).

The inconsistent accesses occur because the `adapter` object is mapped into
streaming DMA. However, when it is mapped into streaming DMA, it is then
"owned" by the device. Hence, any access to `adapter` thereafter, if not
preceded by a CPU-synchronization operation (e.g.,
`dma_sync_single_for_cpu()`), may cause unexpected hardware behaviors.

This patch series consists of two patches:
- Patch 1 adds synchronization operations into `vmxnet3_probe_device()`, to
  mitigate the inconsistent accesses when `adapter` is initialized.
However, this unfortunately does not mitigate all inconsistent accesses to
it, because `adapter` is accessed elsewhere in the driver without proper
synchronization.
- Patch 2 removes `adapter` from streaming DMA, which entirely mitigates
  the inconsistent accesses to it. It is not clear to me why `adapter` was
mapped into DMA in the first place (in [1]), because it seems that before
[1], it was not mapped into DMA. (However, I am not very familiar with the
VMXNET3 internals, so someone is welcome to correct me here). Alternatively
--- if `adapter` should indeed remain mapped in DMA --- then
synchronization operations should be added throughout the driver code (as
Patch 1 begins to do).

[0] Link: https://www.usenix.org/system/files/sec21-bai.pdf
[1] commit b0eb57cb97e7837ebb746404c2c58c6f536f23fa ("VMXNET3: Add support
for virtual IOMMU")

Brian Johannesmeyer (2):
  vmxnet3: Fix inconsistent DMA accesses in vmxnet3_probe_device()
  vmxnet3: Remove adapter from DMA region

 drivers/net/vmxnet3/vmxnet3_drv.c | 17 ++---------------
 drivers/net/vmxnet3/vmxnet3_int.h |  1 -
 2 files changed, 2 insertions(+), 16 deletions(-)

-- 
2.34.1

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

[Brian Johannesmeyer]

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 <teemperor@gmail.com>
Signed-off-by: Raphael Isemann <teemperor@gmail.com>
Signed-off-by: Brian Johannesmeyer <bjohannesmeyer@gmail.com>
---
 drivers/net/vmxnet3/vmxnet3_drv.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/net/vmxnet3/vmxnet3_drv.c b/drivers/net/vmxnet3/vmxnet3_drv.c
index 7fa74b8b2100..cc76134c7db4 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,
+	dev = &adapter->pdev->dev;
+	adapter_pa = dma_map_single(dev, adapter,
 					     sizeof(struct vmxnet3_adapter),
 					     DMA_TO_DEVICE);
-	if (dma_mapping_error(&adapter->pdev->dev, adapter->adapter_pa)) {
+	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,8 @@ 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

[PATCH 2/2] vmxnet3: Remove adapter from DMA region

[Brian Johannesmeyer]

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 <teemperor@gmail.com>
Signed-off-by: Raphael Isemann <teemperor@gmail.com>
Signed-off-by: Brian Johannesmeyer <bjohannesmeyer@gmail.com>
---
 drivers/net/vmxnet3/vmxnet3_drv.c | 26 ++------------------------
 drivers/net/vmxnet3/vmxnet3_int.h |  1 -
 2 files changed, 2 insertions(+), 25 deletions(-)

diff --git a/drivers/net/vmxnet3/vmxnet3_drv.c b/drivers/net/vmxnet3/vmxnet3_drv.c
index cc76134c7db4..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 */
@@ -3623,8 +3623,6 @@ 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;
@@ -3664,19 +3662,6 @@ vmxnet3_probe_device(struct pci_dev *pdev,
 	}
 
 	spin_lock_init(&adapter->cmd_lock);
-	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),
@@ -3684,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);
@@ -3935,8 +3920,6 @@ 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:
@@ -3963,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;
@@ -4032,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

Re: [PATCH 0/2] vmxnet3: Fix inconsistent DMA accesses

[Jakub Kicinski]

On Wed, 13 Nov 2024 20:59:59 +0100 Brian Johannesmeyer wrote:
> We found hundreds of inconsistent DMA accesses in the VMXNET3 driver. This
> patch series aims to fix them. (For a nice summary of the rules around
> accessing streaming DMA --- which, if violated, result in inconsistent
> accesses --- see Figure 4a of this paper [0]).
> 
> The inconsistent accesses occur because the `adapter` object is mapped into
> streaming DMA. However, when it is mapped into streaming DMA, it is then
> "owned" by the device. Hence, any access to `adapter` thereafter, if not
> preceded by a CPU-synchronization operation (e.g.,
> `dma_sync_single_for_cpu()`), may cause unexpected hardware behaviors.
> 
> This patch series consists of two patches:
> - Patch 1 adds synchronization operations into `vmxnet3_probe_device()`, to
>   mitigate the inconsistent accesses when `adapter` is initialized.
> However, this unfortunately does not mitigate all inconsistent accesses to
> it, because `adapter` is accessed elsewhere in the driver without proper
> synchronization.
> - Patch 2 removes `adapter` from streaming DMA, which entirely mitigates
>   the inconsistent accesses to it. It is not clear to me why `adapter` was
> mapped into DMA in the first place (in [1]), because it seems that before
> [1], it was not mapped into DMA. (However, I am not very familiar with the
> VMXNET3 internals, so someone is welcome to correct me here). Alternatively
> --- if `adapter` should indeed remain mapped in DMA --- then
> synchronization operations should be added throughout the driver code (as
> Patch 1 begins to do).

I guess we need to hear from vmxnet3 maintainers to know whether DMA
mapping is necessary for this virt device. But committing patch 1 just
to completely revert it in patch 2 seems a little odd.

Also trivial note, please checkpatch with --strict --max-line-length=80
-- 
pw-bot: cr

Re: [PATCH 0/2] vmxnet3: Fix inconsistent DMA accesses

[Brian Johannesmeyer]

> But committing patch 1 just
> to completely revert it in patch 2 seems a little odd.

Indeed, this was a poor choice on my part. I suppose the correct way
to do this would be to submit them separately (as opposed to as a
series)? I.e.: (i) one patch to start adding the synchronization
operations (in case `adapter` should indeed be in a DMA region), and
(ii) a second patch to remove `adapter` from a DMA region? Based on
the feedback, I can submit a V2 patch for either (i) or (ii).

> Also trivial note, please checkpatch with --strict --max-line-length=80

Thanks for the feedback.

-Brian

On Thu, Nov 14, 2024 at 8:38 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Wed, 13 Nov 2024 20:59:59 +0100 Brian Johannesmeyer wrote:
> > We found hundreds of inconsistent DMA accesses in the VMXNET3 driver. This
> > patch series aims to fix them. (For a nice summary of the rules around
> > accessing streaming DMA --- which, if violated, result in inconsistent
> > accesses --- see Figure 4a of this paper [0]).
> >
> > The inconsistent accesses occur because the `adapter` object is mapped into
> > streaming DMA. However, when it is mapped into streaming DMA, it is then
> > "owned" by the device. Hence, any access to `adapter` thereafter, if not
> > preceded by a CPU-synchronization operation (e.g.,
> > `dma_sync_single_for_cpu()`), may cause unexpected hardware behaviors.
> >
> > This patch series consists of two patches:
> > - Patch 1 adds synchronization operations into `vmxnet3_probe_device()`, to
> >   mitigate the inconsistent accesses when `adapter` is initialized.
> > However, this unfortunately does not mitigate all inconsistent accesses to
> > it, because `adapter` is accessed elsewhere in the driver without proper
> > synchronization.
> > - Patch 2 removes `adapter` from streaming DMA, which entirely mitigates
> >   the inconsistent accesses to it. It is not clear to me why `adapter` was
> > mapped into DMA in the first place (in [1]), because it seems that before
> > [1], it was not mapped into DMA. (However, I am not very familiar with the
> > VMXNET3 internals, so someone is welcome to correct me here). Alternatively
> > --- if `adapter` should indeed remain mapped in DMA --- then
> > synchronization operations should be added throughout the driver code (as
> > Patch 1 begins to do).
>
> I guess we need to hear from vmxnet3 maintainers to know whether DMA
> mapping is necessary for this virt device. But committing patch 1 just
> to completely revert it in patch 2 seems a little odd.
>
> Also trivial note, please checkpatch with --strict --max-line-length=80
> --
> pw-bot: cr

Re: [PATCH 0/2] vmxnet3: Fix inconsistent DMA accesses

[Jakub Kicinski]

On Mon, 18 Nov 2024 08:31:35 -0700 Brian Johannesmeyer wrote:
> > But committing patch 1 just
> > to completely revert it in patch 2 seems a little odd.  
> 
> Indeed, this was a poor choice on my part. I suppose the correct way
> to do this would be to submit them separately (as opposed to as a
> series)? I.e.: (i) one patch to start adding the synchronization
> operations (in case `adapter` should indeed be in a DMA region), and
> (ii) a second patch to remove `adapter` from a DMA region? Based on
> the feedback, I can submit a V2 patch for either (i) or (ii).

What is the purpose of the first patch? Is it sufficient to make 
the device work correctly?

If yes, why do we need patch 2.
If no, why do we have patch 1, instead of a revert / patch 2...

Re: [PATCH 0/2] vmxnet3: Fix inconsistent DMA accesses

[Brian Johannesmeyer]

> What is the purpose of the first patch? Is it sufficient to make
> the device work correctly?
The purpose of the first patch is to fix the inconsistent accesses in
`vmxnet3_probe_device()`. This only partially fixes the issue,
however, because there are inconsistent accesses elsewhere in the
driver. So, no, it does not make the device work *entirely* correctly.

> If yes, why do we need patch 2.
> If no, why do we have patch 1, instead of a revert / patch 2...
The answer is that the way I submitted this patch series was a
mistake. Specifically, I submitted it as: (i) patch 1 is applied on
master, *and* (ii) patch 2 is applied on patch 1.

Instead, the way I should have submitted it was: (i) patch 1 is
applied on master, *or* (ii) a corrected version of patch 2 is applied
on master. (By "a corrected version of patch 2", I mean not
pointlessly reverting patch 1.)

The difference being:
- If `adapter` *should* be mapped to DMA, then patch 1 is the way to go. Or,
- If `adapter` *should not* be mapped to DMA, then a corrected version
of patch 2 is the way to go.

I hope this clears things up. I'm sorry for the confusion I caused. I
will submit a V2 patch series that makes this clear.

Thanks,

Brian