mbox series

[v5,00/38] DRM: fix struct sg_table nents vs. orig_nents misuse

Message ID 20200513132114.6046-1-m.szyprowski@samsung.com
Headers show
Series DRM: fix struct sg_table nents vs. orig_nents misuse | expand

Message

Marek Szyprowski May 13, 2020, 1:21 p.m. UTC
Dear All,

During the Exynos DRM GEM rework and fixing the issues in the.
drm_prime_sg_to_page_addr_arrays() function [1] I've noticed that most
drivers in DRM framework incorrectly use nents and orig_nents entries of
the struct sg_table.

In case of the most DMA-mapping implementations exchanging those two
entries or using nents for all loops on the scatterlist is harmless,
because they both have the same value. There exists however a DMA-mapping
implementations, for which such incorrect usage breaks things. The nents
returned by dma_map_sg() might be lower than the nents passed as its
parameter and this is perfectly fine. DMA framework or IOMMU is allowed
to join consecutive chunks while mapping if such operation is supported
by the underlying HW (bus, bridge, IOMMU, etc). Example of the case
where dma_map_sg() might return 1 'DMA' chunk for the 4 'physical' pages
is described here [2]

The DMA-mapping framework documentation [3] states that dma_map_sg()
returns the numer of the created entries in the DMA address space.
However the subsequent calls to dma_sync_sg_for_{device,cpu} and
dma_unmap_sg must be called with the original number of entries passed to
dma_map_sg. The common pattern in DRM drivers were to assign the
dma_map_sg() return value to sg_table->nents and use that value for
the subsequent calls to dma_sync_sg_* or dma_unmap_sg functions. Also
the code iterated over nents times to access the pages stored in the
processed scatterlist, while it should use orig_nents as the numer of
the page entries.

I've tried to identify all such incorrect usage of sg_table->nents and
this is a result of my research. It looks that the incorrect pattern has
been copied over the many drivers mainly in the DRM subsystem. Too bad in
most cases it even worked correctly if the system used a simple, linear
DMA-mapping implementation, for which swapping nents and orig_nents
doesn't make any difference. To avoid similar issues in the future, I've
introduced a common wrappers for DMA-mapping calls, which operate directly
on the sg_table objects. I've also added wrappers for iterating over the
scatterlists stored in the sg_table objects and applied them where
possible. This, together with some common DRM prime helpers, allowed me
to almost get rid of all nents/orig_nents usage in the drivers. I hope
that such change makes the code robust, easier to follow and copy/paste
safe.

The biggest TODO is DRM/i915 driver and I don't feel brave enough to fix
it fully. The driver creatively uses sg_table->orig_nents to store the
size of the allocate scatterlist and ignores the number of the entries
returned by dma_map_sg function. In this patchset I only fixed the
sg_table objects exported by dmabuf related functions. I hope that I
didn't break anything there.

Patches are based on top of Linux next-20200512.

Christoph Hellwig already offered to take patches 1-3 into his immutable
branch [4]. If possible I would like ask for merging most of the
remaining patches via DRM tree (on top of that immutable branch).

Best regards,
Marek Szyprowski


References:

[1] https://lkml.org/lkml/2020/3/27/555
[2] https://lkml.org/lkml/2020/3/29/65
[3] Documentation/DMA-API-HOWTO.txt
[4] https://lore.kernel.org/linux-iommu/20200512121931.GD20393@lst.de/T/#ma18c958a48c3b241d5409517fa7d192eef87459b

Changelog:

v5:
- fixed some minor style issues and typos
- fixed lack of the attrs argument in ion, dmabuf, rapidio, fastrpc and
  vfio patches

v4: https://lore.kernel.org/linux-iommu/20200512121931.GD20393@lst.de/T/
- added for_each_sgtable_* wrappers and applied where possible
- added drm_prime_get_contiguous_size() and applied where possible
- applied drm_prime_sg_to_page_addr_arrays() where possible to remove page
  extraction from sg_table objects
- added documentation for the introduced wrappers
- improved patches description a bit

v3: https://lore.kernel.org/dri-devel/20200505083926.28503-1-m.szyprowski@samsung.com/
- introduce dma_*_sgtable_* wrappers and use them in all patches

v2: https://lore.kernel.org/linux-iommu/c01c9766-9778-fd1f-f36e-2dc7bd376ba4@arm.com/T/
- dropped most of the changes to drm/i915
- added fixes for rcar-du, xen, media and ion
- fixed a few issues pointed by kbuild test robot
- added wide cc: list for each patch

v1: https://lore.kernel.org/linux-iommu/c01c9766-9778-fd1f-f36e-2dc7bd376ba4@arm.com/T/
- initial version


Patch summary:

Marek Szyprowski (38):
  dma-mapping: add generic helpers for mapping sgtable objects
  scatterlist: add generic wrappers for iterating over sgtable objects
  iommu: add generic helper for mapping sgtable objects
  drm: prime: add common helper to check scatterlist contiguity
  drm: prime: use sgtable iterators in
    drm_prime_sg_to_page_addr_arrays()
  drm: core: fix common struct sg_table related issues
  drm: amdgpu: fix common struct sg_table related issues
  drm: armada: fix common struct sg_table related issues
  drm: etnaviv: fix common struct sg_table related issues
  drm: exynos: use common helper for a scatterlist contiguity check
  drm: exynos: fix common struct sg_table related issues
  drm: i915: fix common struct sg_table related issues
  drm: lima: fix common struct sg_table related issues
  drm: mediatek: use common helper for a scatterlist contiguity check
  drm: mediatek: use common helper for extracting pages array
  drm: msm: fix common struct sg_table related issues
  drm: omapdrm: use common helper for extracting pages array
  drm: omapdrm: fix common struct sg_table related issues
  drm: panfrost: fix common struct sg_table related issues
  drm: radeon: fix common struct sg_table related issues
  drm: rockchip: use common helper for a scatterlist contiguity check
  drm: rockchip: fix common struct sg_table related issues
  drm: tegra: fix common struct sg_table related issues
  drm: v3d: fix common struct sg_table related issues
  drm: virtio: fix common struct sg_table related issues
  drm: vmwgfx: fix common struct sg_table related issues
  xen: gntdev: fix common struct sg_table related issues
  drm: host1x: fix common struct sg_table related issues
  drm: rcar-du: fix common struct sg_table related issues
  dmabuf: fix common struct sg_table related issues
  staging: ion: remove dead code
  staging: ion: fix common struct sg_table related issues
  staging: tegra-vde: fix common struct sg_table related issues
  misc: fastrpc: fix common struct sg_table related issues
  rapidio: fix common struct sg_table related issues
  samples: vfio-mdev/mbochs: fix common struct sg_table related issues
  media: pci: fix common ALSA DMA-mapping related codes
  videobuf2: use sgtable-based scatterlist wrappers

 drivers/dma-buf/heaps/heap-helpers.c               | 13 ++--
 drivers/dma-buf/udmabuf.c                          |  7 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c        |  6 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c            |  9 +--
 drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c       |  8 +-
 drivers/gpu/drm/armada/armada_gem.c                | 12 +--
 drivers/gpu/drm/drm_cache.c                        |  2 +-
 drivers/gpu/drm/drm_gem_cma_helper.c               | 23 +-----
 drivers/gpu/drm/drm_gem_shmem_helper.c             | 14 ++--
 drivers/gpu/drm/drm_prime.c                        | 86 ++++++++++++----------
 drivers/gpu/drm/etnaviv/etnaviv_gem.c              | 12 ++-
 drivers/gpu/drm/etnaviv/etnaviv_mmu.c              | 13 +---
 drivers/gpu/drm/exynos/exynos_drm_g2d.c            | 10 +--
 drivers/gpu/drm/exynos/exynos_drm_gem.c            | 23 +-----
 drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c         | 11 +--
 drivers/gpu/drm/i915/gem/selftests/mock_dmabuf.c   |  7 +-
 drivers/gpu/drm/lima/lima_gem.c                    | 11 ++-
 drivers/gpu/drm/lima/lima_vm.c                     |  5 +-
 drivers/gpu/drm/mediatek/mtk_drm_gem.c             | 34 ++-------
 drivers/gpu/drm/msm/msm_gem.c                      | 13 ++--
 drivers/gpu/drm/msm/msm_gpummu.c                   | 14 ++--
 drivers/gpu/drm/msm/msm_iommu.c                    |  2 +-
 drivers/gpu/drm/omapdrm/omap_gem.c                 | 20 ++---
 drivers/gpu/drm/panfrost/panfrost_gem.c            |  4 +-
 drivers/gpu/drm/panfrost/panfrost_mmu.c            |  7 +-
 drivers/gpu/drm/radeon/radeon_ttm.c                | 11 ++-
 drivers/gpu/drm/rcar-du/rcar_du_vsp.c              |  3 +-
 drivers/gpu/drm/rockchip/rockchip_drm_gem.c        | 42 +++--------
 drivers/gpu/drm/tegra/gem.c                        | 27 +++----
 drivers/gpu/drm/tegra/plane.c                      | 15 ++--
 drivers/gpu/drm/v3d/v3d_mmu.c                      | 17 ++---
 drivers/gpu/drm/virtio/virtgpu_object.c            | 36 +++++----
 drivers/gpu/drm/virtio/virtgpu_vq.c                | 12 ++-
 drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c         | 17 +----
 drivers/gpu/host1x/job.c                           | 22 ++----
 .../media/common/videobuf2/videobuf2-dma-contig.c  | 41 +++++------
 drivers/media/common/videobuf2/videobuf2-dma-sg.c  | 32 +++-----
 drivers/media/common/videobuf2/videobuf2-vmalloc.c | 12 +--
 drivers/media/pci/cx23885/cx23885-alsa.c           |  2 +-
 drivers/media/pci/cx25821/cx25821-alsa.c           |  2 +-
 drivers/media/pci/cx88/cx88-alsa.c                 |  2 +-
 drivers/media/pci/saa7134/saa7134-alsa.c           |  2 +-
 drivers/media/platform/vsp1/vsp1_drm.c             |  8 +-
 drivers/misc/fastrpc.c                             |  4 +-
 drivers/rapidio/devices/rio_mport_cdev.c           |  8 +-
 drivers/staging/android/ion/ion.c                  | 25 +++----
 drivers/staging/android/ion/ion.h                  |  1 -
 drivers/staging/android/ion/ion_heap.c             | 53 ++++---------
 drivers/staging/android/ion/ion_system_heap.c      |  2 +-
 drivers/staging/media/tegra-vde/iommu.c            |  4 +-
 drivers/xen/gntdev-dmabuf.c                        | 13 ++--
 include/drm/drm_prime.h                            |  2 +
 include/linux/dma-mapping.h                        | 78 ++++++++++++++++++++
 include/linux/iommu.h                              | 16 ++++
 include/linux/scatterlist.h                        | 50 ++++++++++++-
 samples/vfio-mdev/mbochs.c                         |  3 +-
 56 files changed, 451 insertions(+), 477 deletions(-)

-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Comments

Marek Szyprowski May 26, 2020, 7:01 a.m. UTC | #1
Hi

On 13.05.2020 15:47, Christoph Hellwig wrote:
> I've pushed out a branch with the first three patches here:

>

>     git://git.infradead.org/users/hch/dma-mapping.git dma-sg_table-helper

>

> Gitweb:

>

>     http://git.infradead.org/users/hch/dma-mapping.git/shortlog/refs/heads/dma-sg_table-helper

>

> and merged it into the dma-mapping for-next tree.  Unless someone shouts

> the branch should be considered immutable in 24 hours.


David & Daniel: could you merge all the DRM related changes on top of 
the provided branch? Merging those changes separately would take a lots 
of time because of the dependencies on the sgtable helpers and changes 
in the DRM core.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Daniel Vetter May 26, 2020, 11:31 a.m. UTC | #2
On Tue, May 26, 2020 at 9:01 AM Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
>

> Hi

>

> On 13.05.2020 15:47, Christoph Hellwig wrote:

> > I've pushed out a branch with the first three patches here:

> >

> >     git://git.infradead.org/users/hch/dma-mapping.git dma-sg_table-helper

> >

> > Gitweb:

> >

> >     http://git.infradead.org/users/hch/dma-mapping.git/shortlog/refs/heads/dma-sg_table-helper

> >

> > and merged it into the dma-mapping for-next tree.  Unless someone shouts

> > the branch should be considered immutable in 24 hours.

>

> David & Daniel: could you merge all the DRM related changes on top of

> the provided branch? Merging those changes separately would take a lots

> of time because of the dependencies on the sgtable helpers and changes

> in the DRM core.


We generally freeze drm for big rework past -rc6 (small drivers tend
to be a bit later). I think simpler if we just land this in the merge
window and then smash the drm patches on top for 5.9 merge window.

Or all in in the same topic branch, but feels a bit late for that and
making sure nothing breaks.
-Daniel

>

> Best regards

> --

> Marek Szyprowski, PhD

> Samsung R&D Institute Poland

>



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel