Intel-iommu: Possible memory corruption due to deferred iommu flush

Intel-iommu: Possible memory corruption due to deferred iommu flush

[nick klingsman]

* Note: Please 'CC'/Reply my email as I'm not subscribed to the list yet.

Hi,

During __iommu_flush_iotlb() we set the WD (WriteDrain) bit if the
iommu supports cap_write_drain(). This part of the logic exists from
day one of the code and later even added to QI logic.

Back than, when we dma_unmap_xxx() we 'synchronically' did the
iommu_flush() and within it the the Write-Drain logic.

However, today, we do a batch-flush  which means that all/most calls
to dma_unmap() don't perform the Write-Drain.

Questions:

1. If WD is a must, how come the batch-flush doesn't cause any memory
corruption ? For example:
   a. dma_map_page(100, from_device)
   b. Device writes data to device
   c. dma_unmap_page() -> batched and not sync-flushed
   d. Possible BUG ? Data read before the write is drained can be stale ?

2. If WD is not needed, why do we do it ? Why do we effect IOMMU work
with unneeded actions (in the most critical fast path of map/unmap) ?


Thanks,
Nick

Re: Intel-iommu: Possible memory corruption due to deferred iommu flush

[Raj, Ashok]

On Thu, Nov 16, 2017 at 12:20:47PM +0200, nick klingsman wrote:
> * Note: Please 'CC'/Reply my email as I'm not subscribed to the list yet.
> 
> Hi,
> 
> During __iommu_flush_iotlb() we set the WD (WriteDrain) bit if the
> iommu supports cap_write_drain(). This part of the logic exists from
> day one of the code and later even added to QI logic.
> 
> Back than, when we dma_unmap_xxx() we 'synchronically' did the
> iommu_flush() and within it the the Write-Drain logic.
> 
> However, today, we do a batch-flush  which means that all/most calls
> to dma_unmap() don't perform the Write-Drain.
> 
> Questions:
> 
> 1. If WD is a must, how come the batch-flush doesn't cause any memory
> corruption ? For example:
>    a. dma_map_page(100, from_device)
>    b. Device writes data to device
>    c. dma_unmap_page() -> batched and not sync-flushed
>    d. Possible BUG ? Data read before the write is drained can be stale ?

__iommu_flush_iotlb() will do DMA_TLB_WRITE_DRAIN.. so that's actually 
done even in the batched more.

> 
> 2. If WD is not needed, why do we do it ? Why do we effect IOMMU work
> with unneeded actions (in the most critical fast path of map/unmap) ?
> 
> 
> Thanks,
> Nick
> _______________________________________________
> iommu mailing list
> iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: Intel-iommu: Possible memory corruption due to deferred iommu flush

[nick klingsman]

Hi Ashok,

On 17 November 2017 at 00:52, Raj, Ashok <ashok.raj-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:
> On Thu, Nov 16, 2017 at 12:20:47PM +0200, nick klingsman wrote:
>> * Note: Please 'CC'/Reply my email as I'm not subscribed to the list yet.
>>
>> Hi,
>>
>> During __iommu_flush_iotlb() we set the WD (WriteDrain) bit if the
>> iommu supports cap_write_drain(). This part of the logic exists from
>> day one of the code and later even added to QI logic.
>>
>> Back than, when we dma_unmap_xxx() we 'synchronically' did the
>> iommu_flush() and within it the the Write-Drain logic.
>>
>> However, today, we do a batch-flush  which means that all/most calls
>> to dma_unmap() don't perform the Write-Drain.
>>
>> Questions:
>>
>> 1. If WD is a must, how come the batch-flush doesn't cause any memory
>> corruption ? For example:
>>    a. dma_map_page(100, from_device)
>>    b. Device writes data to device
>>    c. dma_unmap_page() -> batched and not sync-flushed
>>    d. Possible BUG ? Data read before the write is drained can be stale ?
>
> __iommu_flush_iotlb() will do DMA_TLB_WRITE_DRAIN.. so that's actually
> done even in the batched more.


That's the point.. this is not 'batched' mode. Its "deferred" mode.
the flush occurs *async* to the dma_unmap() call by the driver.

In my example, after the dma_unmap() @ 'c', the flush didn't yet
occured and so is the Write Drain request.


>
>>
>> 2. If WD is not needed, why do we do it ? Why do we effect IOMMU work
>> with unneeded actions (in the most critical fast path of map/unmap) ?
>>
>>
>> Thanks,
>> Nick
>> _______________________________________________
>> iommu mailing list
>> iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
>> https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: Intel-iommu: Possible memory corruption due to deferred iommu flush

[nick klingsman]

On 17 November 2017 at 01:54, nick klingsman <nick.klingsman-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> Hi Ashok,
>
> On 17 November 2017 at 00:52, Raj, Ashok <ashok.raj-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:
>> On Thu, Nov 16, 2017 at 12:20:47PM +0200, nick klingsman wrote:
>>> * Note: Please 'CC'/Reply my email as I'm not subscribed to the list yet.
>>>
>>> Hi,
>>>
>>> During __iommu_flush_iotlb() we set the WD (WriteDrain) bit if the
>>> iommu supports cap_write_drain(). This part of the logic exists from
>>> day one of the code and later even added to QI logic.
>>>
>>> Back than, when we dma_unmap_xxx() we 'synchronically' did the
>>> iommu_flush() and within it the the Write-Drain logic.
>>>
>>> However, today, we do a batch-flush  which means that all/most calls
>>> to dma_unmap() don't perform the Write-Drain.
>>>
>>> Questions:
>>>
>>> 1. If WD is a must, how come the batch-flush doesn't cause any memory
>>> corruption ? For example:
>>>    a. dma_map_page(100, from_device)
>>>    b. Device writes data to device
>>>    c. dma_unmap_page() -> batched and not sync-flushed
>>>    d. Possible BUG ? Data read before the write is drained can be stale ?
>>
>> __iommu_flush_iotlb() will do DMA_TLB_WRITE_DRAIN.. so that's actually
>> done even in the batched more.
>
>
> That's the point.. this is not 'batched' mode. Its "deferred" mode.
> the flush occurs *async* to the dma_unmap() call by the driver.
>
> In my example, after the dma_unmap() @ 'c', the flush didn't yet
> occured and so is the Write Drain request.
>

Oh - now I understand the confusion. Although the subject said 'deferred'
I used the word 'Batched' in the email body. I meant 'deferred'. Not batching.
If it was only batching, than it would make sens as 'Ashok' pointed out.

>
>>
>>>
>>> 2. If WD is not needed, why do we do it ? Why do we effect IOMMU work
>>> with unneeded actions (in the most critical fast path of map/unmap) ?
>>>
>>>
>>> Thanks,
>>> Nick
>>> _______________________________________________
>>> iommu mailing list
>>> iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
>>> https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: Intel-iommu: Possible memory corruption due to deferred iommu flush

[Raj, Ashok]

Hi Nick

On Fri, Nov 17, 2017 at 01:57:19AM +0200, nick klingsman wrote:
> 
> Oh - now I understand the confusion. Although the subject said 'deferred'
> I used the word 'Batched' in the email body. I meant 'deferred'. Not batching.
> If it was only batching, than it would make sens as 'Ashok' pointed out.
> 
> >
> >>
> >>>
> >>> 2. If WD is not needed, why do we do it ? Why do we effect IOMMU work
> >>> with unneeded actions (in the most critical fast path of map/unmap) ?

The purpose of WRITE_DRAIN is something like this.. 

If you had a buffer that DMA writes are still happening but the entity using
the buffer has been terminated. You are just looking to ensure that before you
reallocate the buffer to another entity that the old writes aren't still
happening.

When we do the dma_unmap() that would get rid of the mappings and an iotlb 
flush would ensure any future access the DMA would fail. WRITE_DRAIN ensures
any address already translated and DMA is in progress completes. 

In the batched mode when we delay performing the iotlb invalidations + 
write_drain, the I/O is completed and driver was notified earlier. As
part of tracking the I/O completion the driver would have read some mmio
on the device that would have caused all the outstanding writes from the
device written to memory. So the buffer in this case is all globally 
observable by the consumer.. so there is no stale state for that DMA that 
has properly completed. In these cases the WD is probaly not required, but 
we have the same path for all invalidations so its simple to do it for 
consistency.



Hope this helps.

Cheers,
Ashok