dri-devel Archive on lore.kernel.org
 help / color / Atom feed
From: Marek Szyprowski <m.szyprowski@samsung.com>
To: dri-devel@lists.freedesktop.org,
	iommu@lists.linux-foundation.org, linaro-mm-sig@lists.linaro.org,
	linux-kernel@vger.kernel.org
Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>,
	David Airlie <airlied@linux.ie>,
	Robin Murphy <robin.murphy@arm.com>,
	Christoph Hellwig <hch@lst.de>,
	linux-arm-kernel@lists.infradead.org,
	Marek Szyprowski <m.szyprowski@samsung.com>
Subject: [PATCH v3 00/25] DRM: fix struct sg_table nents vs. orig_nents misuse
Date: Tue,  5 May 2020 10:43:00 +0200
Message-ID: <20200505083926.28503-1-m.szyprowski@samsung.com> (raw)
In-Reply-To: <CGME20200505084308eucas1p1aa040c3ae325a6c7d92f956b1f5aad0d@eucas1p1.samsung.com>

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 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 dma-mapping wrappers, which operate directly on the
sg_table objects.

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-20200504.

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


Changelog:

v3:
- 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 (25):
  dma-mapping: add generic helpers for mapping sgtable objects
  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: 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: msm: 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: fix common struct sg_table related issues
  drm: tegra: 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: 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

 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/armada/armada_gem.c              | 10 ++++----
 drivers/gpu/drm/drm_cache.c                      |  2 +-
 drivers/gpu/drm/drm_gem_shmem_helper.c           | 14 +++++++----
 drivers/gpu/drm/drm_prime.c                      | 13 +++++-----
 drivers/gpu/drm/etnaviv/etnaviv_gem.c            | 12 ++++-----
 drivers/gpu/drm/exynos/exynos_drm_g2d.c          |  9 +++----
 drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c       | 13 ++++------
 drivers/gpu/drm/i915/gem/selftests/mock_dmabuf.c |  7 +++---
 drivers/gpu/drm/lima/lima_gem.c                  | 11 +++++---
 drivers/gpu/drm/msm/msm_gem.c                    | 13 ++++------
 drivers/gpu/drm/msm/msm_iommu.c                  |  2 +-
 drivers/gpu/drm/panfrost/panfrost_gem.c          |  4 +--
 drivers/gpu/drm/panfrost/panfrost_mmu.c          |  5 ++--
 drivers/gpu/drm/radeon/radeon_ttm.c              | 11 ++++----
 drivers/gpu/drm/rockchip/rockchip_drm_gem.c      | 26 +++++++++----------
 drivers/gpu/drm/tegra/gem.c                      | 27 ++++++++------------
 drivers/gpu/drm/tegra/plane.c                    | 15 ++++-------
 drivers/gpu/drm/virtio/virtgpu_object.c          | 17 ++++++-------
 drivers/gpu/drm/virtio/virtgpu_vq.c              | 10 +++-----
 drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c       | 17 +++----------
 drivers/gpu/host1x/job.c                         | 22 ++++++----------
 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           |  9 ++++---
 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_heap.c           |  6 ++---
 drivers/staging/android/ion/ion_system_heap.c    |  2 +-
 drivers/staging/media/tegra-vde/iommu.c          |  4 +--
 drivers/xen/gntdev-dmabuf.c                      |  7 +++---
 include/linux/dma-mapping.h                      | 32 ++++++++++++++++++++++++
 include/linux/iommu.h                            |  6 +++++
 samples/vfio-mdev/mbochs.c                       |  3 ++-
 40 files changed, 202 insertions(+), 207 deletions(-)

-- 
1.9.1

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

       reply index

Thread overview: 34+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <CGME20200505084308eucas1p1aa040c3ae325a6c7d92f956b1f5aad0d@eucas1p1.samsung.com>
2020-05-05  8:43 ` Marek Szyprowski [this message]
     [not found]   ` <CGME20200505084624eucas1p2a9a5c4d2aece2c1555a5480c19c2e050@eucas1p2.samsung.com>
2020-05-05  8:45     ` [PATCH v3 01/25] dma-mapping: add generic helpers for mapping sgtable objects Marek Szyprowski
     [not found]       ` <CGME20200505084625eucas1p1a3c25fd171f360e0aab2f76700699454@eucas1p1.samsung.com>
2020-05-05  8:45         ` [PATCH v3 02/25] drm: core: fix common struct sg_table related issues Marek Szyprowski
     [not found]           ` <20200505101508.GA14860@lst.de>
2020-05-05 10:51             ` Marek Szyprowski
     [not found]               ` <20200505110950.GA19067@lst.de>
2020-05-08  7:12                 ` Marek Szyprowski
     [not found]                   ` <20200508071609.GA31690@lst.de>
2020-05-12  9:05                     ` Marek Szyprowski
     [not found]       ` <CGME20200505084625eucas1p2b8ca16ff91ba9d6655f525ef85915d00@eucas1p2.samsung.com>
2020-05-05  8:45         ` [PATCH v3 03/25] drm: amdgpu: " Marek Szyprowski
     [not found]       ` <CGME20200505084626eucas1p20753456727333c09718253ca5c32d98c@eucas1p2.samsung.com>
2020-05-05  8:45         ` [PATCH v3 04/25] drm: armada: " Marek Szyprowski
     [not found]       ` <CGME20200505084626eucas1p20abe79e406f60ae92fec252072befc5a@eucas1p2.samsung.com>
2020-05-05  8:45         ` [PATCH v3 05/25] drm: etnaviv: " Marek Szyprowski
     [not found]       ` <CGME20200505084627eucas1p119c77fbf28532627f27382efc51b0aaa@eucas1p1.samsung.com>
2020-05-05  8:45         ` [PATCH v3 06/25] drm: exynos: " Marek Szyprowski
     [not found]       ` <CGME20200505084627eucas1p199eed52198b4409da1fa8e2256f5bb62@eucas1p1.samsung.com>
2020-05-05  8:45         ` [PATCH v3 07/25] drm: i915: " Marek Szyprowski
     [not found]       ` <CGME20200505084628eucas1p2c87aae2f471b716675559debbf680c46@eucas1p2.samsung.com>
2020-05-05  8:45         ` [PATCH v3 08/25] drm: lima: " Marek Szyprowski
     [not found]       ` <CGME20200505084629eucas1p12e882329da88edd155ba9f9f952889a0@eucas1p1.samsung.com>
2020-05-05  8:45         ` [PATCH v3 09/25] drm: msm: " Marek Szyprowski
     [not found]       ` <CGME20200505084629eucas1p23d2d6a53451e67e2b0a3544eb696008b@eucas1p2.samsung.com>
2020-05-05  8:45         ` [PATCH v3 10/25] drm: panfrost: " Marek Szyprowski
2020-05-11 15:51           ` Steven Price
     [not found]       ` <CGME20200505084630eucas1p1c74cd5d287e1080b85d98edde405a577@eucas1p1.samsung.com>
2020-05-05  8:46         ` [PATCH v3 11/25] drm: radeon: " Marek Szyprowski
     [not found]       ` <CGME20200505084630eucas1p2199401486591b681b84a4b24496295fb@eucas1p2.samsung.com>
2020-05-05  8:46         ` [PATCH v3 12/25] drm: rockchip: " Marek Szyprowski
     [not found]       ` <CGME20200505084631eucas1p11121f9373a1d282f0262d1faa33f35fb@eucas1p1.samsung.com>
2020-05-05  8:46         ` [PATCH v3 13/25] drm: tegra: " Marek Szyprowski
     [not found]       ` <CGME20200505084632eucas1p2e37c536205c057984c5f0355f6ffe1c2@eucas1p2.samsung.com>
2020-05-05  8:46         ` [PATCH v3 14/25] drm: virtio: " Marek Szyprowski
     [not found]       ` <CGME20200505084632eucas1p231212e9cea88e755da8eaf1fb012d2c6@eucas1p2.samsung.com>
2020-05-05  8:46         ` [PATCH v3 15/25] drm: vmwgfx: " Marek Szyprowski
2020-05-12  0:19           ` [Linux-graphics-maintainer] " Roland Scheidegger
     [not found]       ` <CGME20200505084633eucas1p26a6a3f44c64955aadec834bed027e522@eucas1p2.samsung.com>
2020-05-05  8:46         ` [PATCH v3 16/25] xen: gntdev: " Marek Szyprowski
     [not found]       ` <CGME20200505084633eucas1p19798e1fb42c9430a93d668bc585e58da@eucas1p1.samsung.com>
2020-05-05  8:46         ` [PATCH v3 17/25] drm: host1x: " Marek Szyprowski
     [not found]       ` <CGME20200505084634eucas1p1e0ea160dd77afbf6d2f7e6154ded40d0@eucas1p1.samsung.com>
2020-05-05  8:46         ` [PATCH v3 18/25] drm: rcar-du: " Marek Szyprowski
2020-05-05  9:24           ` Geert Uytterhoeven
     [not found]       ` <CGME20200505084634eucas1p105456f28d9a7935190478546e566975f@eucas1p1.samsung.com>
2020-05-05  8:46         ` [PATCH v3 19/25] dmabuf: " Marek Szyprowski
     [not found]       ` <CGME20200505084635eucas1p14800a1e2598364d168adecd57b94225c@eucas1p1.samsung.com>
2020-05-05  8:46         ` [PATCH v3 20/25] staging: ion: " Marek Szyprowski
     [not found]       ` <CGME20200505084636eucas1p23a33d0b83ca284692713745d004f93ea@eucas1p2.samsung.com>
2020-05-05  8:46         ` [PATCH v3 21/25] staging: tegra-vde: " Marek Szyprowski
     [not found]       ` <CGME20200505084637eucas1p20390fa3c010bde00e438cce1b48d209c@eucas1p2.samsung.com>
2020-05-05  8:46         ` [PATCH v3 22/25] misc: fastrpc: " Marek Szyprowski
     [not found]       ` <CGME20200505084637eucas1p2c6d4b880698e8db97a8a9468692befe1@eucas1p2.samsung.com>
2020-05-05  8:46         ` [PATCH v3 23/25] rapidio: " Marek Szyprowski
     [not found]       ` <CGME20200505084638eucas1p24f356b441a3589e9528d239c0b9ac666@eucas1p2.samsung.com>
2020-05-05  8:46         ` [PATCH v3 24/25] samples: vfio-mdev/mbochs: " Marek Szyprowski
     [not found]       ` <CGME20200505084638eucas1p2d4add214063543248d81c0977e3f1823@eucas1p2.samsung.com>
2020-05-05  8:46         ` [PATCH v3 25/25] media: pci: fix common ALSA DMA-mapping related codes Marek Szyprowski
     [not found]       ` <20200505102234.GA15038@lst.de>
2020-05-05 10:44         ` [PATCH v3 01/25] dma-mapping: add generic helpers for mapping sgtable objects Marek Szyprowski
2020-05-07  8:47       ` Hans Verkuil

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200505083926.28503-1-m.szyprowski@samsung.com \
    --to=m.szyprowski@samsung.com \
    --cc=airlied@linux.ie \
    --cc=b.zolnierkie@samsung.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=hch@lst.de \
    --cc=iommu@lists.linux-foundation.org \
    --cc=linaro-mm-sig@lists.linaro.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=robin.murphy@arm.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

dri-devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/dri-devel/0 dri-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dri-devel dri-devel/ https://lore.kernel.org/dri-devel \
		dri-devel@lists.freedesktop.org
	public-inbox-index dri-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.freedesktop.lists.dri-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git