[RFC PATCH v1 0/2] Fix the v7.3-rc4 DMABUF_DEBUG regression breaking drm/msm hardware video decode

[RFC PATCH v1 0/2] Fix the v7.3-rc4 DMABUF_DEBUG regression breaking drm/msm hardware video decode

[Jianfeng Liu]

Hardware video decode in clapper and chromium (V4L2 decoder output
buffers imported into drm/msm for rendering and scanout) breaks on
v7.3-rc4 with arm-smmu translation faults:

  gpu fault: ttbr0=000000088a889000 iova=000000010741c000 dir=READ
  type=TRANSLATION source=UCHE

v7.3-rc3 works fine. Bisecting between the two points at
143755bdabaa9 ("dma-buf: Make DMABUF_DEBUG default to y on
DEBUG_KERNEL kernels"), which fixed a dangling reference in the
DMABUF_DEBUG default and thereby silently enabled the option - and
with it the page-stripping sg_table wrapper that
dma_buf_map_attachment() hands to importers - on every kernel with
DEBUG_KERNEL=y, i.e. virtually every distro kernel.

drm/msm is affected in two places. It fills the page array of
imported GEM objects through the deprecated
drm_prime_sg_to_page_array(), and it maps the attachment sg_table
into the GPU's own pagetables with iommu_map_sgtable(). Both need
the struct page of the sg_table, which the debug wrapper removes
(and it zeroes sg->length, so the page iterator yields nothing while
the uninitialized page array is kept, with the helper still
returning success).

When such an import is used for rendering, the VM_BIND map job then
fails asynchronously after userspace has already enqueued GPU work
referencing the mapping, which surfaces as the UCHE translation
fault above instead of a clean error.

Patch 1 restores the DMABUF_DEBUG default to n until msm can be
converted to build its GPU mappings from the attachment's DMA
addresses. Patch 2 replaces the deprecated helper in msm with an
explicit loop that rejects page-less sg_tables at import time, so
userspace gets a clean -EINVAL and can fall back instead of
crashing the GPU.

Tested on a Snapdragon laptop with an Adreno GPU and arm-smmu
(v7.3-rc4):

- DMABUF_DEBUG off: hardware video decode works as on v7.3-rc3
- DMABUF_DEBUG on, without patch 2: GPU faults as above
- DMABUF_DEBUG on, with patch 2: imports are rejected cleanly
  ("import of dmabuf from 'videobuf2_dma_contig' rejected: sg_table
  has no/misaligned struct page info"), no GPU faults. clapper falls
  back to a working display path; chromium shows a black window as
  it has no fallback for a failed zero-copy import.

A full fix for DMABUF_DEBUG=y requires msm to map imported buffers
from their DMA addresses rather than struct pages; that conversion
is left as future work.

Comments welcome.


Jianfeng Liu (2):
  dma-buf: keep DMABUF_DEBUG off by default
  drm/msm: reject dma-buf imports without struct page info

 drivers/dma-buf/Kconfig       |  9 ++++++++-
 drivers/gpu/drm/msm/msm_gem.c | 31 ++++++++++++++++++++++++++++---
 2 files changed, 36 insertions(+), 4 deletions(-)

---
base-commit: 93f51579e7df248780214094418f205253383cc5
branch: fix/dmabuf-debug-msm-import

-- 
2.47.3

[RFC PATCH v1 1/2] dma-buf: keep DMABUF_DEBUG off by default

[Jianfeng Liu]

Commit 143755bdabaa9 ("dma-buf: Make DMABUF_DEBUG default to y on
DEBUG_KERNEL kernels") fixed a dangling reference in the DMABUF_DEBUG
default, which had the side effect of enabling the option (and with it
the page-stripping sg_table wrapper handed to importers) on every
kernel with DEBUG_KERNEL=y - i.e. virtually every distro kernel.

drm/msm is broken by this: it maps imported dma-bufs into the GPU's
own pagetables with iommu_map_sgtable(), which needs the struct page
of the attachment sg_table, and it fills the GEM object's page array
through drm_prime_sg_to_page_array().  With the debug wrapper in
place both silently produce garbage (the wrapper zeroes sg->length,
so the page iterator yields nothing and an uninitialized array is
kept).  The VM_BIND map job then fails asynchronously after userspace
has already enqueued GPU work referencing the mapping, which shows up
as an arm-smmu translation fault from UCHE, e.g.:

  gpu fault: ttbr0=000000088a889000 iova=000000010741c000 dir=READ
  type=TRANSLATION source=UCHE

This breaks hardware video decode (clapper, chromium) on Adreno
systems; bisected on a Snapdragon laptop as v7.3-rc3 good,
v7.3-rc4 bad, culprit 143755bdabaa9.

Revert the default until importers that legitimately need to build
phys-based mappings have been converted.

Fixes: 143755bdabaa9 ("dma-buf: Make DMABUF_DEBUG default to y on DEBUG_KERNEL kernels")
Signed-off-by: Jianfeng Liu <liujianfeng1994@gmail.com>
---

 drivers/dma-buf/Kconfig | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/dma-buf/Kconfig b/drivers/dma-buf/Kconfig
index e4f078a326a41..b3c581ef4c987 100644
--- a/drivers/dma-buf/Kconfig
+++ b/drivers/dma-buf/Kconfig
@@ -43,7 +43,14 @@ config UDMABUF
 config DMABUF_DEBUG
 	bool "DMA-BUF debug checks"
 	depends on DMA_SHARED_BUFFER
-	default y if DEBUG_KERNEL
+	# NOTE: keep this default n.  The page-stripping sg_table wrapper that
+	# this option installs for importers breaks drivers that build a
+	# second-stage IOMMU mapping (phys -> iova) from the attachment sg_table
+	# and therefore still need the struct page, e.g. drm/msm with its
+	# per-process GPU pagetables.  Until those importers are fixed, making
+	# this default y breaks hardware video decode and GPU workloads out of
+	# the box on affected systems.
+	default n
 	help
 	  This option enables additional checks for DMA-BUF importers and
 	  exporters. Specifically it validates that importers do not peek at the
-- 
2.47.3

Re: [RFC PATCH v1 1/2] dma-buf: keep DMABUF_DEBUG off by default

[Christian König]

On 9/23/26 09:42, Jianfeng Liu wrote:
> [Sie erhalten nicht häufig E-Mails von liujianfeng1994@gmail.com. Weitere Informationen, warum dies wichtig ist, finden Sie unter https://aka.ms/LearnAboutSenderIdentification ]
> 
> Commit 143755bdabaa9 ("dma-buf: Make DMABUF_DEBUG default to y on
> DEBUG_KERNEL kernels") fixed a dangling reference in the DMABUF_DEBUG
> default, which had the side effect of enabling the option (and with it
> the page-stripping sg_table wrapper handed to importers) on every
> kernel with DEBUG_KERNEL=y - i.e. virtually every distro kernel.
> 
> drm/msm is broken by this: it maps imported dma-bufs into the GPU's
> own pagetables with iommu_map_sgtable(), which needs the struct page
> of the attachment sg_table, and it fills the GEM object's page array
> through drm_prime_sg_to_page_array().  With the debug wrapper in
> place both silently produce garbage (the wrapper zeroes sg->length,

Interesting point, we should probably change that.

> so the page iterator yields nothing and an uninitialized array is
> kept).  The VM_BIND map job then fails asynchronously after userspace
> has already enqueued GPU work referencing the mapping, which shows up
> as an arm-smmu translation fault from UCHE, e.g.:
> 
>   gpu fault: ttbr0=000000088a889000 iova=000000010741c000 dir=READ
>   type=TRANSLATION source=UCHE
> 
> This breaks hardware video decode (clapper, chromium) on Adreno
> systems; bisected on a Snapdragon laptop as v7.3-rc3 good,
> v7.3-rc4 bad, culprit 143755bdabaa9.
> 
> Revert the default until importers that legitimately need to build
> phys-based mappings have been converted.

Well that won't work like this, pointing those things out is exactly what DMABUF_DEBUG is made for.

What MSM is doing here is not allowed at all and can break badly. We gave drivers 5 years to get that fixed and I'm now pushing for completely deprecating that hack.

See patch 84335675f2223cbd25d0de7d38ecc7d40b95bd4a:

Author: Simona Vetter <simona.vetter@ffwll.ch>
Date:   Fri Jan 15 17:47:39 2021 +0100

    dma-buf: Add debug option

When MSM needs the struct page then it must import a shmemfd and not a DMA-buf. What we could do is to either fix MSM or mark it as broken.

Regards,
Christian.

> 
> Fixes: 143755bdabaa9 ("dma-buf: Make DMABUF_DEBUG default to y on DEBUG_KERNEL kernels")
> Signed-off-by: Jianfeng Liu <liujianfeng1994@gmail.com>
> ---
> 
>  drivers/dma-buf/Kconfig | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/dma-buf/Kconfig b/drivers/dma-buf/Kconfig
> index e4f078a326a41..b3c581ef4c987 100644
> --- a/drivers/dma-buf/Kconfig
> +++ b/drivers/dma-buf/Kconfig
> @@ -43,7 +43,14 @@ config UDMABUF
>  config DMABUF_DEBUG
>         bool "DMA-BUF debug checks"
>         depends on DMA_SHARED_BUFFER
> -       default y if DEBUG_KERNEL
> +       # NOTE: keep this default n.  The page-stripping sg_table wrapper that
> +       # this option installs for importers breaks drivers that build a
> +       # second-stage IOMMU mapping (phys -> iova) from the attachment sg_table
> +       # and therefore still need the struct page, e.g. drm/msm with its
> +       # per-process GPU pagetables.  Until those importers are fixed, making
> +       # this default y breaks hardware video decode and GPU workloads out of
> +       # the box on affected systems.
> +       default n
>         help
>           This option enables additional checks for DMA-BUF importers and
>           exporters. Specifically it validates that importers do not peek at the
> --
> 2.47.3
> 

[RFC PATCH v1 2/2] drm/msm: reject dma-buf imports without struct page info

[Jianfeng Liu]

msm_gem_import() fills the GEM object's page array with the deprecated
drm_prime_sg_to_page_array() and stores the attachment sg_table for
later mapping into the GPU's own pagetables via iommu_map_sgtable().
Both need the struct page of the sg_table:

 - iommu_map_sg() maps sg_phys() of each entry, and
 - drm_prime_sg_to_page_array() iterates with for_each_sgtable_page,
   which walks sg->length.

When CONFIG_DMABUF_DEBUG=y, dma_buf_map_attachment() hands importers
a copy of the sg_table with the page pointers stripped and sg->length
zeroed.  In that case drm_prime_sg_to_page_array() "succeeds" while
filling zero entries, leaving msm_obj->pages uninitialized garbage
(kvmalloc_objs() does not zero).  The buffer is imported anyway, and
the first VM_BIND map of it fails asynchronously in the scheduler job
run - after userspace has already enqueued GPU work referencing the
mapping.  Userspace then observes arm-smmu translation faults from
UCHE, e.g. hardware video decode in clapper/chromium:

  gpu fault: ttbr0=000000088a889000 iova=000000010741c000 dir=READ
  type=TRANSLATION source=UCHE

Replace the deprecated helper with an explicit loop so that a missing
or short page list is detected at import time and rejected with
-EINVAL.  This turns the silent memory corruption into a clean import
error, letting userspace fall back instead of crashing the GPU.

Note that msm fundamentally cannot map a page-less sg_table into its
per-process GPU pagetables (it needs the physical addresses), so
imports of such buffers can never work until msm is converted to
build its GPU mappings from the attachment's DMA addresses.

Signed-off-by: Jianfeng Liu <liujianfeng1994@gmail.com>

---

 drivers/gpu/drm/msm/msm_gem.c | 31 ++++++++++++++++++++++++++++---
 1 file changed, 28 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
index c4cff3d53d81b..0d5a91181d05b 100644
--- a/drivers/gpu/drm/msm/msm_gem.c
+++ b/drivers/gpu/drm/msm/msm_gem.c
@@ -1307,7 +1307,8 @@ struct drm_gem_object *msm_gem_import(struct drm_device *dev,
 	struct msm_gem_object *msm_obj;
 	struct drm_gem_object *obj;
 	struct dma_buf *dmabuf = attach->dmabuf;
-	size_t size, npages;
+	struct sg_page_iter piter;
+	size_t size, npages, filled = 0;
 	int ret;
 
 	size = PAGE_ALIGN(dmabuf->size);
@@ -1333,8 +1334,32 @@ struct drm_gem_object *msm_gem_import(struct drm_device *dev,
 		goto fail;
 	}
 
-	ret = drm_prime_sg_to_page_array(sgt, msm_obj->pages, npages);
-	if (ret) {
+	/*
+	 * Fill the page array ourselves instead of using the deprecated
+	 * drm_prime_sg_to_page_array(), so that we can detect sg_tables
+	 * that carry no struct page at all.  Those must be rejected:
+	 * msm maps imported buffers into the GPU's own pagetables with
+	 * iommu_map_sgtable(), which needs the physical pages, so an
+	 * import without page information could never be mapped.  The
+	 * most prominent case is the page-stripping sg_table wrapper that
+	 * dma_buf_map_attachment() hands out when CONFIG_DMABUF_DEBUG=y.
+	 *
+	 * drm_prime_sg_to_page_array() would "succeed" with zero entries
+	 * filled in that case and leave msm_obj->pages uninitialized,
+	 * which later blows up as arm-smmu translation faults from UCHE.
+	 */
+	for_each_sgtable_page(sgt, &piter, 0) {
+		if (WARN_ON(filled >= npages)) {
+			ret = -EINVAL;
+			goto fail;
+		}
+		msm_obj->pages[filled++] = sg_page_iter_page(&piter);
+	}
+	if (filled != npages) {
+		DRM_DEV_ERROR(dev->dev,
+			      "import of dmabuf from '%s' rejected: sg_table has no/misaligned struct page info\n",
+			      dmabuf->exp_name ?: "?");
+		ret = -EINVAL;
 		goto fail;
 	}
 
-- 
2.47.3