+ Sumit
Hi Sumit,
Do you have any thoughts on this patch?
It fixes a potential crash in on older kernel and I think limiting begin/end_cpu_access to only apply cache maintenance when the buffer is dma mapped makes sense from a logical perspective and performance perspective.
On Wed, 6 Feb 2019, Ørjan Eide wrote:
I've run some testing, and this patch does indeed fix the crash in dma_sync_sg_for_cpu when it tried to use the 0 dma_address from the sg list.
Tested-by: Ørjan Eide orjan.eide@arm.com
I tested this on an older kernel, v4.14, since the dma-mapping code moved, in v4.19, to ignore the dma_address and instead use sg_phys() to get a valid address from the page, which is always valid in the ion sg lists. While this wouldn't crash on newer kernels, it's still good to avoid the unnecessary work when no CMO is needed.
Isn't a fix like this also required from a stability perspective for future kernels? I understand from your analysis below that the crash has been fixed after 4.19 by using sg_phys to get the address but aren't we breaking the DMA API contract by calling dma_sync_* without first dma mapping the memory, if so then we have no guarantee that future implementations of functions like dma_direct_sync_sg_for_cpu will properly handle calls to dma_sync_* if the memory is not dma mapped.
Is this patch a candidate for the relevant stable kernels, those that have this bug exposed to user space via Ion and DMA_BUF_IOCTL_SYNC?
My belief is that is relevant for older kernels otherwise an unprivileged malicious userspace application may be able to crash the system if they can call DMA_BUF_IOCTL_SYNC at the right time.
BTW thanks Ørjan testing and anaalsyis you have carried out on this change.
Liam
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project