DMA mapping API abuse in exynos-drm

[Inki Dae]

2013/5/5 Tomasz Figa <tomasz.figa at gmail.com>

> Hi,
>
> Recently I've been working a bit on a DRM driver for the GPU of Samsung
> S3C6410 SoCs, which required me to familiarize a bit with exynos-drm, as
> it already contains a KMS driver which is compatible with the SoC I'm
> working with, making it a good place to put my driver in.
>
> Reading through exynos_drm_buf.c I stumbled across following (a bit long)
> piece of code:
>
>         dma_set_attr(DMA_ATTR_NO_KERNEL_MAPPING, &buf->dma_attrs);
>
>         nr_pages = buf->size >> PAGE_SHIFT;
>
>         if (!is_drm_iommu_supported(dev)) {
>                 dma_addr_t start_addr;
>                 unsigned int i = 0;
>
>                 buf->pages = kzalloc(sizeof(struct page) * nr_pages,
>                                         GFP_KERNEL);
>                 if (!buf->pages) {
>                         DRM_ERROR("failed to allocate pages.\n");
>                         return -ENOMEM;
>                 }
>
>                 buf->kvaddr = dma_alloc_attrs(dev->dev, buf->size,
>                                         &buf->dma_addr, GFP_KERNEL,
>                                         &buf->dma_attrs);
>                 if (!buf->kvaddr) {
>                         DRM_ERROR("failed to allocate buffer.\n");
>                         kfree(buf->pages);
>                         return -ENOMEM;
>                 }
>
>                 start_addr = buf->dma_addr;
>                 while (i < nr_pages) {
>                         buf->pages[i] = phys_to_page(start_addr);
>                         start_addr += PAGE_SIZE;
>                         i++;
>                 }
>         } else {
>
>                 buf->pages = dma_alloc_attrs(dev->dev, buf->size,
>                                         &buf->dma_addr, GFP_KERNEL,
>                                         &buf->dma_attrs);
>                 if (!buf->pages) {
>                         DRM_ERROR("failed to allocate buffer.\n");
>                         return -ENOMEM;
>                 }
>         }
>
>         buf->sgt = drm_prime_pages_to_sg(buf->pages, nr_pages);
>         if (!buf->sgt) {
>                 DRM_ERROR("failed to get sg table.\n");
>                 ret = -ENOMEM;
>                 goto err_free_attrs;
>         }
>
> Notice that the value returned by dma_alloc_attrs() is assumed by this
> code to be either a kernel virtual address (in !is_drm_iommu_supported()
> case) or struct pages ** (in is_drm_iommu_supported() case), which seemed
> a bit worrying to me, making me do a more in depth research on how
> dma_alloc_attrs works.
>
> This, in turn, led me to following excerpt of Documentation/DMA-
> attributes.txt :
>
> DMA_ATTR_NO_KERNEL_MAPPING
> --------------------------
>
> DMA_ATTR_NO_KERNEL_MAPPING lets the platform to avoid creating a kernel
> virtual mapping for the allocated buffer. On some architectures creating
> such mapping is non-trivial task and consumes very limited resources
> (like kernel virtual address space or dma consistent address space).
> Buffers allocated with this attribute can be only passed to user space
> by calling dma_mmap_attrs(). By using this API, you are guaranteeing
> that you won't dereference the pointer returned by dma_alloc_attr(). You
> can threat it as a cookie that must be passed to dma_mmap_attrs() and
> dma_free_attrs(). Make sure that both of these also get this attribute
> set on each call.
>
> of which the following sentence is the most relevant:
>
> By using this API, you are guaranteeing that you won't dereference the
> pointer returned by dma_alloc_attr().
>
> This statement is obviously ignored by buffer allocation code of exynos-
> drm.
>
> A simple git blame revealed that this brokenness has been introduced by
> commit:
>
> 4744ad2 drm/exynos: use DMA_ATTR_NO_KERNEL_MAPPING attribute
>
>
> and later fixed a bit by following depending patches (because the original
> patch apparently broke any allocations without IOMMU):
>
> c704f1b drm/exynos: consider no iommu support to console framebuffer
> 694be45 drm/exynos: consider buffer allocation without iommu
>
>
> Now, the real question is whether my reasoning is incorrect (sorry for the
> noise then) or this is really a bug which needs to be fixed?
>
>
Hi Tomasz,

Your question is reasonable to me. And below is my opinion,

First, also the below attribute says like below,

DMA_ATTR_NO_KERNEL_MAPPING
--------------------------
...
Since it is optional for platforms to implement
DMA_ATTR_NO_KERNEL_MAPPING, those that do not will simply ignore the
attribute and exhibit default behavior.


In case of ARM SoC, it seems like that it just ignores the attribute
without iommu: in case of no iommu, dma_alloc_attr() always maps pages
allocated from highmem with kernel space. So I think we make sure that
exynos drm driver sets the attribute only with iommu to avoid such
confusing. For this, will post it soon.

Thanks,
Inki Dae

Best regards,
> Tomasz
>
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20130506/9f3fa9e5/attachment-0001.html>