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