[PATCH 0/4] drm: Ignore non-existing color planes in helpers

[PATCH 0/4] drm: Ignore non-existing color planes in helpers

[Thomas Zimmermann]

Some DRM helpers assume that all potential color planes of a framebuffer
are available; even if the color format didn't specify them. Non-existing
planes are silently ignored. This behavior is inconsistent with other
helpers and apparently leads to subtle bugs with uninitialized GEM buffer
mappings. [1]

Change all affected code to look at the framebuffer format's number of
color planes and only process these planes. The update has been discussed
in [2] before.

Tested with GEM SHMEM helpers on simpledrm.

[1] https://lore.kernel.org/dri-devel/20210730183511.20080-1-tzimmermann@suse.de/T/#md0172b10bb588d8f20f4f456e304f08d2a4505f7
[2] https://lore.kernel.org/dri-devel/877dc0d9-c6c6-022c-20d8-14b33e863934@suse.de/

Thomas Zimmermann (4):
  drm/gem: Share code between drm_gem_fb_{begin,end}_cpu_access()
  drm/gem: Ignore color planes that are unused by framebuffer format
  drm/gem-vram: Ignore planes that are unused by framebuffer format
  drm/gem: Warn on trying to use a non-existing framebuffer plane

 drivers/gpu/drm/drm_gem_atomic_helper.c      |   6 +-
 drivers/gpu/drm/drm_gem_framebuffer_helper.c | 103 +++++++++----------
 drivers/gpu/drm/drm_gem_vram_helper.c        |  36 ++++---
 include/drm/drm_gem_framebuffer_helper.h     |  10 +-
 4 files changed, 81 insertions(+), 74 deletions(-)


base-commit: b0b93865a24c910fcbfa6e6fa0955fae930a56d3
-- 
2.36.0

[PATCH 1/4] drm/gem: Share code between drm_gem_fb_{begin, end}_cpu_access()

[Thomas Zimmermann]

The error-recovery code in drm_gem_fb_begin() is the same pattern
as drm_gem_fb_end(). Implement both this an internal helper. No
functional changes.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_gem_framebuffer_helper.c | 62 ++++++++------------
 1 file changed, 26 insertions(+), 36 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
index f4619803acd0..2fcacab9f812 100644
--- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c
+++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
@@ -403,6 +403,27 @@ void drm_gem_fb_vunmap(struct drm_framebuffer *fb,
 }
 EXPORT_SYMBOL(drm_gem_fb_vunmap);
 
+static void __drm_gem_fb_end_cpu_access(struct drm_framebuffer *fb, enum dma_data_direction dir,
+					unsigned int num_planes)
+{
+	struct dma_buf_attachment *import_attach;
+	struct drm_gem_object *obj;
+	int ret;
+
+	while (num_planes) {
+		--num_planes;
+		obj = drm_gem_fb_get_obj(fb, num_planes);
+		if (!obj)
+			continue;
+		import_attach = obj->import_attach;
+		if (!import_attach)
+			continue;
+		ret = dma_buf_end_cpu_access(import_attach->dmabuf, dir);
+		if (ret)
+			drm_err(fb->dev, "dma_buf_end_cpu_access() failed: %d\n", ret);
+	}
+}
+
 /**
  * drm_gem_fb_begin_cpu_access - prepares GEM buffer objects for CPU access
  * @fb: the framebuffer
@@ -422,7 +443,7 @@ int drm_gem_fb_begin_cpu_access(struct drm_framebuffer *fb, enum dma_data_direct
 	struct dma_buf_attachment *import_attach;
 	struct drm_gem_object *obj;
 	size_t i;
-	int ret, ret2;
+	int ret;
 
 	for (i = 0; i < ARRAY_SIZE(fb->obj); ++i) {
 		obj = drm_gem_fb_get_obj(fb, i);
@@ -433,28 +454,13 @@ int drm_gem_fb_begin_cpu_access(struct drm_framebuffer *fb, enum dma_data_direct
 			continue;
 		ret = dma_buf_begin_cpu_access(import_attach->dmabuf, dir);
 		if (ret)
-			goto err_dma_buf_end_cpu_access;
+			goto err___drm_gem_fb_end_cpu_access;
 	}
 
 	return 0;
 
-err_dma_buf_end_cpu_access:
-	while (i) {
-		--i;
-		obj = drm_gem_fb_get_obj(fb, i);
-		if (!obj)
-			continue;
-		import_attach = obj->import_attach;
-		if (!import_attach)
-			continue;
-		ret2 = dma_buf_end_cpu_access(import_attach->dmabuf, dir);
-		if (ret2) {
-			drm_err(fb->dev,
-				"dma_buf_end_cpu_access() failed during error handling: %d\n",
-				ret2);
-		}
-	}
-
+err___drm_gem_fb_end_cpu_access:
+	__drm_gem_fb_end_cpu_access(fb, dir, i);
 	return ret;
 }
 EXPORT_SYMBOL(drm_gem_fb_begin_cpu_access);
@@ -472,23 +478,7 @@ EXPORT_SYMBOL(drm_gem_fb_begin_cpu_access);
  */
 void drm_gem_fb_end_cpu_access(struct drm_framebuffer *fb, enum dma_data_direction dir)
 {
-	size_t i = ARRAY_SIZE(fb->obj);
-	struct dma_buf_attachment *import_attach;
-	struct drm_gem_object *obj;
-	int ret;
-
-	while (i) {
-		--i;
-		obj = drm_gem_fb_get_obj(fb, i);
-		if (!obj)
-			continue;
-		import_attach = obj->import_attach;
-		if (!import_attach)
-			continue;
-		ret = dma_buf_end_cpu_access(import_attach->dmabuf, dir);
-		if (ret)
-			drm_err(fb->dev, "dma_buf_end_cpu_access() failed: %d\n", ret);
-	}
+	__drm_gem_fb_end_cpu_access(fb, dir, ARRAY_SIZE(fb->obj));
 }
 EXPORT_SYMBOL(drm_gem_fb_end_cpu_access);
 
-- 
2.36.0

[PATCH 2/4] drm/gem: Ignore color planes that are unused by framebuffer format

[Thomas Zimmermann]

Only handle color planes that exist in a framebuffer's color format.
Ignore non-existing planes.

So far, several helpers assumed that all 4 planes are available and
silently ignored non-existing planes. This lead to subtil bugs with
uninitialized data in instances of struct iosys_map. [1]

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Link: https://lore.kernel.org/dri-devel/20210730183511.20080-1-tzimmermann@suse.de/T/#md0172b10bb588d8f20f4f456e304f08d2a4505f7 # 1
---
 drivers/gpu/drm/drm_gem_atomic_helper.c      |  6 ++--
 drivers/gpu/drm/drm_gem_framebuffer_helper.c | 37 +++++++++++---------
 include/drm/drm_gem_framebuffer_helper.h     | 10 ++----
 3 files changed, 27 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem_atomic_helper.c b/drivers/gpu/drm/drm_gem_atomic_helper.c
index a5026f617739..f16d60217c6c 100644
--- a/drivers/gpu/drm/drm_gem_atomic_helper.c
+++ b/drivers/gpu/drm/drm_gem_atomic_helper.c
@@ -169,8 +169,10 @@ int drm_gem_plane_helper_prepare_fb(struct drm_plane *plane,
 		struct drm_gem_object *obj = drm_gem_fb_get_obj(state->fb, i);
 		struct dma_fence *new;
 
-		if (WARN_ON_ONCE(!obj))
-			continue;
+		if (!obj) {
+			ret = -EINVAL;
+			goto error;
+		}
 
 		ret = dma_resv_get_singleton(obj->resv, usage, &new);
 		if (ret)
diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
index 2fcacab9f812..09e90e19cd93 100644
--- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c
+++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
@@ -92,9 +92,9 @@ drm_gem_fb_init(struct drm_device *dev,
  */
 void drm_gem_fb_destroy(struct drm_framebuffer *fb)
 {
-	size_t i;
+	unsigned int i;
 
-	for (i = 0; i < ARRAY_SIZE(fb->obj); i++)
+	for (i = 0; i < fb->format->num_planes; i++)
 		drm_gem_object_put(fb->obj[i]);
 
 	drm_framebuffer_cleanup(fb);
@@ -329,24 +329,26 @@ EXPORT_SYMBOL_GPL(drm_gem_fb_create_with_dirty);
  * The argument returns the addresses of the data stored in each BO. This
  * is different from @map if the framebuffer's offsets field is non-zero.
  *
+ * Both, @map and @data, must each refer to arrays with at least
+ * fb->format->num_planes elements.
+ *
  * See drm_gem_fb_vunmap() for unmapping.
  *
  * Returns:
  * 0 on success, or a negative errno code otherwise.
  */
-int drm_gem_fb_vmap(struct drm_framebuffer *fb,
-		    struct iosys_map map[static DRM_FORMAT_MAX_PLANES],
-		    struct iosys_map data[DRM_FORMAT_MAX_PLANES])
+int drm_gem_fb_vmap(struct drm_framebuffer *fb, struct iosys_map *map,
+		    struct iosys_map *data)
 {
 	struct drm_gem_object *obj;
 	unsigned int i;
 	int ret;
 
-	for (i = 0; i < DRM_FORMAT_MAX_PLANES; ++i) {
+	for (i = 0; i < fb->format->num_planes; ++i) {
 		obj = drm_gem_fb_get_obj(fb, i);
 		if (!obj) {
-			iosys_map_clear(&map[i]);
-			continue;
+			ret = -EINVAL;
+			goto err_drm_gem_vunmap;
 		}
 		ret = drm_gem_vmap(obj, &map[i]);
 		if (ret)
@@ -354,7 +356,7 @@ int drm_gem_fb_vmap(struct drm_framebuffer *fb,
 	}
 
 	if (data) {
-		for (i = 0; i < DRM_FORMAT_MAX_PLANES; ++i) {
+		for (i = 0; i < fb->format->num_planes; ++i) {
 			memcpy(&data[i], &map[i], sizeof(data[i]));
 			if (iosys_map_is_null(&data[i]))
 				continue;
@@ -385,10 +387,9 @@ EXPORT_SYMBOL(drm_gem_fb_vmap);
  *
  * See drm_gem_fb_vmap() for more information.
  */
-void drm_gem_fb_vunmap(struct drm_framebuffer *fb,
-		       struct iosys_map map[static DRM_FORMAT_MAX_PLANES])
+void drm_gem_fb_vunmap(struct drm_framebuffer *fb, struct iosys_map *map)
 {
-	unsigned int i = DRM_FORMAT_MAX_PLANES;
+	unsigned int i = fb->format->num_planes;
 	struct drm_gem_object *obj;
 
 	while (i) {
@@ -442,13 +443,15 @@ int drm_gem_fb_begin_cpu_access(struct drm_framebuffer *fb, enum dma_data_direct
 {
 	struct dma_buf_attachment *import_attach;
 	struct drm_gem_object *obj;
-	size_t i;
+	unsigned int i;
 	int ret;
 
-	for (i = 0; i < ARRAY_SIZE(fb->obj); ++i) {
+	for (i = 0; i < fb->format->num_planes; ++i) {
 		obj = drm_gem_fb_get_obj(fb, i);
-		if (!obj)
-			continue;
+		if (!obj) {
+			ret = -EINVAL;
+			goto err___drm_gem_fb_end_cpu_access;
+		}
 		import_attach = obj->import_attach;
 		if (!import_attach)
 			continue;
@@ -478,7 +481,7 @@ EXPORT_SYMBOL(drm_gem_fb_begin_cpu_access);
  */
 void drm_gem_fb_end_cpu_access(struct drm_framebuffer *fb, enum dma_data_direction dir)
 {
-	__drm_gem_fb_end_cpu_access(fb, dir, ARRAY_SIZE(fb->obj));
+	__drm_gem_fb_end_cpu_access(fb, dir, fb->format->num_planes);
 }
 EXPORT_SYMBOL(drm_gem_fb_end_cpu_access);
 
diff --git a/include/drm/drm_gem_framebuffer_helper.h b/include/drm/drm_gem_framebuffer_helper.h
index 1091e4fa08cb..d302521f3dd4 100644
--- a/include/drm/drm_gem_framebuffer_helper.h
+++ b/include/drm/drm_gem_framebuffer_helper.h
@@ -4,8 +4,6 @@
 #include <linux/dma-buf.h>
 #include <linux/iosys-map.h>
 
-#include <drm/drm_fourcc.h>
-
 struct drm_afbc_framebuffer;
 struct drm_device;
 struct drm_fb_helper_surface_size;
@@ -39,11 +37,9 @@ struct drm_framebuffer *
 drm_gem_fb_create_with_dirty(struct drm_device *dev, struct drm_file *file,
 			     const struct drm_mode_fb_cmd2 *mode_cmd);
 
-int drm_gem_fb_vmap(struct drm_framebuffer *fb,
-		    struct iosys_map map[static DRM_FORMAT_MAX_PLANES],
-		    struct iosys_map data[DRM_FORMAT_MAX_PLANES]);
-void drm_gem_fb_vunmap(struct drm_framebuffer *fb,
-		       struct iosys_map map[static DRM_FORMAT_MAX_PLANES]);
+int drm_gem_fb_vmap(struct drm_framebuffer *fb, struct iosys_map *map,
+		    struct iosys_map *data);
+void drm_gem_fb_vunmap(struct drm_framebuffer *fb, struct iosys_map *map);
 int drm_gem_fb_begin_cpu_access(struct drm_framebuffer *fb, enum dma_data_direction dir);
 void drm_gem_fb_end_cpu_access(struct drm_framebuffer *fb, enum dma_data_direction dir);
 
-- 
2.36.0

[PATCH 3/4] drm/gem-vram: Ignore planes that are unused by framebuffer format

[Thomas Zimmermann]

Only handle color planes that exist in a framebuffer's color format.
Ignore non-existing planes.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_gem_vram_helper.c | 36 ++++++++++++++++++---------
 1 file changed, 24 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c
index 123045b58fec..3305541a10ab 100644
--- a/drivers/gpu/drm/drm_gem_vram_helper.c
+++ b/drivers/gpu/drm/drm_gem_vram_helper.c
@@ -9,6 +9,7 @@
 #include <drm/drm_file.h>
 #include <drm/drm_framebuffer.h>
 #include <drm/drm_gem_atomic_helper.h>
+#include <drm/drm_gem_framebuffer_helper.h>
 #include <drm/drm_gem_ttm_helper.h>
 #include <drm/drm_gem_vram_helper.h>
 #include <drm/drm_managed.h>
@@ -648,17 +649,22 @@ int
 drm_gem_vram_plane_helper_prepare_fb(struct drm_plane *plane,
 				     struct drm_plane_state *new_state)
 {
-	size_t i;
+	struct drm_framebuffer *fb = new_state->fb;
 	struct drm_gem_vram_object *gbo;
+	struct drm_gem_object *obj;
+	unsigned int i;
 	int ret;
 
-	if (!new_state->fb)
+	if (!fb)
 		return 0;
 
-	for (i = 0; i < ARRAY_SIZE(new_state->fb->obj); ++i) {
-		if (!new_state->fb->obj[i])
-			continue;
-		gbo = drm_gem_vram_of_gem(new_state->fb->obj[i]);
+	for (i = 0; i < fb->format->num_planes; ++i) {
+		obj = drm_gem_fb_get_obj(fb, i);
+		if (!obj) {
+			ret = -EINVAL;
+			goto err_drm_gem_vram_unpin;
+		}
+		gbo = drm_gem_vram_of_gem(obj);
 		ret = drm_gem_vram_pin(gbo, DRM_GEM_VRAM_PL_FLAG_VRAM);
 		if (ret)
 			goto err_drm_gem_vram_unpin;
@@ -673,7 +679,10 @@ drm_gem_vram_plane_helper_prepare_fb(struct drm_plane *plane,
 err_drm_gem_vram_unpin:
 	while (i) {
 		--i;
-		gbo = drm_gem_vram_of_gem(new_state->fb->obj[i]);
+		obj = drm_gem_fb_get_obj(fb, i);
+		if (!obj)
+			continue;
+		gbo = drm_gem_vram_of_gem(obj);
 		drm_gem_vram_unpin(gbo);
 	}
 	return ret;
@@ -694,16 +703,19 @@ void
 drm_gem_vram_plane_helper_cleanup_fb(struct drm_plane *plane,
 				     struct drm_plane_state *old_state)
 {
-	size_t i;
+	struct drm_framebuffer *fb = old_state->fb;
 	struct drm_gem_vram_object *gbo;
+	struct drm_gem_object *obj;
+	unsigned int i;
 
-	if (!old_state->fb)
+	if (!fb)
 		return;
 
-	for (i = 0; i < ARRAY_SIZE(old_state->fb->obj); ++i) {
-		if (!old_state->fb->obj[i])
+	for (i = 0; i < fb->format->num_planes; ++i) {
+		obj = drm_gem_fb_get_obj(fb, i);
+		if (!obj)
 			continue;
-		gbo = drm_gem_vram_of_gem(old_state->fb->obj[i]);
+		gbo = drm_gem_vram_of_gem(obj);
 		drm_gem_vram_unpin(gbo);
 	}
 }
-- 
2.36.0

[PATCH 4/4] drm/gem: Warn on trying to use a non-existing framebuffer plane

[Thomas Zimmermann]

Warn if callers of drm_gem_fb_get_obj() try to use a GEM buffer for
a non-existing or unset plane.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_gem_framebuffer_helper.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
index 09e90e19cd93..291f9ae4359d 100644
--- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c
+++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
@@ -53,7 +53,11 @@ MODULE_IMPORT_NS(DMA_BUF);
 struct drm_gem_object *drm_gem_fb_get_obj(struct drm_framebuffer *fb,
 					  unsigned int plane)
 {
-	if (plane >= ARRAY_SIZE(fb->obj))
+	struct drm_device *dev = fb->dev;
+
+	if (drm_WARN_ON_ONCE(dev, plane >= ARRAY_SIZE(fb->obj)))
+		return NULL;
+	else if (drm_WARN_ON_ONCE(dev, !fb->obj[plane]))
 		return NULL;
 
 	return fb->obj[plane];
-- 
2.36.0

Re: [PATCH 0/4] drm: Ignore non-existing color planes in helpers

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 1722 bytes --]

pinging for review

Am 09.05.22 um 10:15 schrieb Thomas Zimmermann:
> Some DRM helpers assume that all potential color planes of a framebuffer
> are available; even if the color format didn't specify them. Non-existing
> planes are silently ignored. This behavior is inconsistent with other
> helpers and apparently leads to subtle bugs with uninitialized GEM buffer
> mappings. [1]
> 
> Change all affected code to look at the framebuffer format's number of
> color planes and only process these planes. The update has been discussed
> in [2] before.
> 
> Tested with GEM SHMEM helpers on simpledrm.
> 
> [1] https://lore.kernel.org/dri-devel/20210730183511.20080-1-tzimmermann@suse.de/T/#md0172b10bb588d8f20f4f456e304f08d2a4505f7
> [2] https://lore.kernel.org/dri-devel/877dc0d9-c6c6-022c-20d8-14b33e863934@suse.de/
> 
> Thomas Zimmermann (4):
>    drm/gem: Share code between drm_gem_fb_{begin,end}_cpu_access()
>    drm/gem: Ignore color planes that are unused by framebuffer format
>    drm/gem-vram: Ignore planes that are unused by framebuffer format
>    drm/gem: Warn on trying to use a non-existing framebuffer plane
> 
>   drivers/gpu/drm/drm_gem_atomic_helper.c      |   6 +-
>   drivers/gpu/drm/drm_gem_framebuffer_helper.c | 103 +++++++++----------
>   drivers/gpu/drm/drm_gem_vram_helper.c        |  36 ++++---
>   include/drm/drm_gem_framebuffer_helper.h     |  10 +-
>   4 files changed, 81 insertions(+), 74 deletions(-)
> 
> 
> base-commit: b0b93865a24c910fcbfa6e6fa0955fae930a56d3

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 1/4] drm/gem: Share code between drm_gem_fb_{begin, end}_cpu_access()

[Javier Martinez Canillas]

Hello Thomas,

On 5/9/22 10:15, Thomas Zimmermann wrote:
> The error-recovery code in drm_gem_fb_begin() is the same pattern
> as drm_gem_fb_end(). Implement both this an internal helper. No

Maybe "both of these using using an" or something like that ?

> functional changes.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/drm_gem_framebuffer_helper.c | 62 ++++++++------------
>  1 file changed, 26 insertions(+), 36 deletions(-)
>

Nice cleanup. For the patch:

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

I just have a few minor comments below.

> diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> index f4619803acd0..2fcacab9f812 100644
> --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> @@ -403,6 +403,27 @@ void drm_gem_fb_vunmap(struct drm_framebuffer *fb,
>  }
>  EXPORT_SYMBOL(drm_gem_fb_vunmap);
>  
> +static void __drm_gem_fb_end_cpu_access(struct drm_framebuffer *fb, enum dma_data_direction dir,
> +					unsigned int num_planes)
> +{
> +	struct dma_buf_attachment *import_attach;
> +	struct drm_gem_object *obj;
> +	int ret;
> +
> +	while (num_planes) {
> +		--num_planes;
> +		obj = drm_gem_fb_get_obj(fb, num_planes);
> +		if (!obj)
> +			continue;
> +		import_attach = obj->import_attach;
> +		if (!import_attach)
> +			continue;
> +		ret = dma_buf_end_cpu_access(import_attach->dmabuf, dir);
> +		if (ret)
> +			drm_err(fb->dev, "dma_buf_end_cpu_access() failed: %d\n", ret);

I wonder if would be useful to also print the plane index and access mode here.

> +	}
> +}
> +
>  /**
>   * drm_gem_fb_begin_cpu_access - prepares GEM buffer objects for CPU access
>   * @fb: the framebuffer
> @@ -422,7 +443,7 @@ int drm_gem_fb_begin_cpu_access(struct drm_framebuffer *fb, enum dma_data_direct
>  	struct dma_buf_attachment *import_attach;
>  	struct drm_gem_object *obj;
>  	size_t i;
> -	int ret, ret2;
> +	int ret;
>  
>  	for (i = 0; i < ARRAY_SIZE(fb->obj); ++i) {
>  		obj = drm_gem_fb_get_obj(fb, i);
> @@ -433,28 +454,13 @@ int drm_gem_fb_begin_cpu_access(struct drm_framebuffer *fb, enum dma_data_direct
>  			continue;
>  		ret = dma_buf_begin_cpu_access(import_attach->dmabuf, dir);
>  		if (ret)
> -			goto err_dma_buf_end_cpu_access;
> +			goto err___drm_gem_fb_end_cpu_access;

I wouldn't rename this. As I read it, the goto label doesn't denote what
function is called but rather what action is being taken in an error path.

Since finally what's being done is a dma_buf_end_cpu_access in the helper,
I think that just keeping err_dma_buf_end_cpu_access is enough. Also, the
additional underscores added make it harder to read IMO.

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH 2/4] drm/gem: Ignore color planes that are unused by framebuffer format

[Javier Martinez Canillas]

On 5/9/22 10:16, Thomas Zimmermann wrote:
> Only handle color planes that exist in a framebuffer's color format.
> Ignore non-existing planes.
> 
> So far, several helpers assumed that all 4 planes are available and
> silently ignored non-existing planes. This lead to subtil bugs with
> uninitialized data in instances of struct iosys_map. [1]
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Link: https://lore.kernel.org/dri-devel/20210730183511.20080-1-tzimmermann@suse.de/T/#md0172b10bb588d8f20f4f456e304f08d2a4505f7 # 1
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH 0/4] drm: Ignore non-existing color planes in helpers

[Noralf Trønnes]

Den 09.05.2022 10.15, skrev Thomas Zimmermann:
> Some DRM helpers assume that all potential color planes of a framebuffer
> are available; even if the color format didn't specify them. Non-existing
> planes are silently ignored. This behavior is inconsistent with other
> helpers and apparently leads to subtle bugs with uninitialized GEM buffer
> mappings. [1]
> 
> Change all affected code to look at the framebuffer format's number of
> color planes and only process these planes. The update has been discussed
> in [2] before.
> 
> Tested with GEM SHMEM helpers on simpledrm.
> 

Tested with drm/gud, now there's only one UBSAN warning:

Tested-by: Noralf Trønnes <noralf@tronnes.org>

I was hoping to get some time to give a review, but it doesn't look like
I'm able to do that.

Noralf.


> [1] https://lore.kernel.org/dri-devel/20210730183511.20080-1-tzimmermann@suse.de/T/#md0172b10bb588d8f20f4f456e304f08d2a4505f7
> [2] https://lore.kernel.org/dri-devel/877dc0d9-c6c6-022c-20d8-14b33e863934@suse.de/
> 
> Thomas Zimmermann (4):
>   drm/gem: Share code between drm_gem_fb_{begin,end}_cpu_access()
>   drm/gem: Ignore color planes that are unused by framebuffer format
>   drm/gem-vram: Ignore planes that are unused by framebuffer format
>   drm/gem: Warn on trying to use a non-existing framebuffer plane
> 
>  drivers/gpu/drm/drm_gem_atomic_helper.c      |   6 +-
>  drivers/gpu/drm/drm_gem_framebuffer_helper.c | 103 +++++++++----------
>  drivers/gpu/drm/drm_gem_vram_helper.c        |  36 ++++---
>  include/drm/drm_gem_framebuffer_helper.h     |  10 +-
>  4 files changed, 81 insertions(+), 74 deletions(-)
> 
> 
> base-commit: b0b93865a24c910fcbfa6e6fa0955fae930a56d3

Re: [PATCH 3/4] drm/gem-vram: Ignore planes that are unused by framebuffer format

[Javier Martinez Canillas]

On 5/9/22 10:16, Thomas Zimmermann wrote:
> Only handle color planes that exist in a framebuffer's color format.
> Ignore non-existing planes.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

> @@ -673,7 +679,10 @@ drm_gem_vram_plane_helper_prepare_fb(struct drm_plane *plane,
>  err_drm_gem_vram_unpin:
>  	while (i) {
>  		--i;
> -		gbo = drm_gem_vram_of_gem(new_state->fb->obj[i]);
> +		obj = drm_gem_fb_get_obj(fb, i);
> +		if (!obj)
> +			continue;
> +		gbo = drm_gem_vram_of_gem(obj);
>  		drm_gem_vram_unpin(gbo);

The code in this error path to unpin the GEM vram objects...

>  	}
>  	return ret;
> @@ -694,16 +703,19 @@ void
>  drm_gem_vram_plane_helper_cleanup_fb(struct drm_plane *plane,
>  				     struct drm_plane_state *old_state)
>  {
> -	size_t i;
> +	struct drm_framebuffer *fb = old_state->fb;
>  	struct drm_gem_vram_object *gbo;
> +	struct drm_gem_object *obj;
> +	unsigned int i;
>  
> -	if (!old_state->fb)
> +	if (!fb)
>  		return;
>  
> -	for (i = 0; i < ARRAY_SIZE(old_state->fb->obj); ++i) {
> -		if (!old_state->fb->obj[i])
> +	for (i = 0; i < fb->format->num_planes; ++i) {
> +		obj = drm_gem_fb_get_obj(fb, i);
> +		if (!obj)
>  			continue;
> -		gbo = drm_gem_vram_of_gem(old_state->fb->obj[i]);
> +		gbo = drm_gem_vram_of_gem(obj);
>  		drm_gem_vram_unpin(gbo);

... and this, seems to be basically the same.

So maybe as a follow-up you can do a similar cleanup like you did in patch
1/4 and use a helper function to handle both.

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH 4/4] drm/gem: Warn on trying to use a non-existing framebuffer plane

[Javier Martinez Canillas]

On 5/9/22 10:16, Thomas Zimmermann wrote:
> Warn if callers of drm_gem_fb_get_obj() try to use a GEM buffer for
> a non-existing or unset plane.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH 1/4] drm/gem: Share code between drm_gem_fb_{begin, end}_cpu_access()

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 3416 bytes --]

Hi Javier

Am 16.05.22 um 15:00 schrieb Javier Martinez Canillas:
> Hello Thomas,
> 
> On 5/9/22 10:15, Thomas Zimmermann wrote:
>> The error-recovery code in drm_gem_fb_begin() is the same pattern
>> as drm_gem_fb_end(). Implement both this an internal helper. No
> 
> Maybe "both of these using using an" or something like that ?

Of course.

> 
>> functional changes.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>   drivers/gpu/drm/drm_gem_framebuffer_helper.c | 62 ++++++++------------
>>   1 file changed, 26 insertions(+), 36 deletions(-)
>>
> 
> Nice cleanup. For the patch:
> 
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> 
> I just have a few minor comments below.
> 
>> diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
>> index f4619803acd0..2fcacab9f812 100644
>> --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c
>> +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
>> @@ -403,6 +403,27 @@ void drm_gem_fb_vunmap(struct drm_framebuffer *fb,
>>   }
>>   EXPORT_SYMBOL(drm_gem_fb_vunmap);
>>   
>> +static void __drm_gem_fb_end_cpu_access(struct drm_framebuffer *fb, enum dma_data_direction dir,
>> +					unsigned int num_planes)
>> +{
>> +	struct dma_buf_attachment *import_attach;
>> +	struct drm_gem_object *obj;
>> +	int ret;
>> +
>> +	while (num_planes) {
>> +		--num_planes;
>> +		obj = drm_gem_fb_get_obj(fb, num_planes);
>> +		if (!obj)
>> +			continue;
>> +		import_attach = obj->import_attach;
>> +		if (!import_attach)
>> +			continue;
>> +		ret = dma_buf_end_cpu_access(import_attach->dmabuf, dir);
>> +		if (ret)
>> +			drm_err(fb->dev, "dma_buf_end_cpu_access() failed: %d\n", ret);
> 
> I wonder if would be useful to also print the plane index and access mode here.

Ok.

> 
>> +	}
>> +}
>> +
>>   /**
>>    * drm_gem_fb_begin_cpu_access - prepares GEM buffer objects for CPU access
>>    * @fb: the framebuffer
>> @@ -422,7 +443,7 @@ int drm_gem_fb_begin_cpu_access(struct drm_framebuffer *fb, enum dma_data_direct
>>   	struct dma_buf_attachment *import_attach;
>>   	struct drm_gem_object *obj;
>>   	size_t i;
>> -	int ret, ret2;
>> +	int ret;
>>   
>>   	for (i = 0; i < ARRAY_SIZE(fb->obj); ++i) {
>>   		obj = drm_gem_fb_get_obj(fb, i);
>> @@ -433,28 +454,13 @@ int drm_gem_fb_begin_cpu_access(struct drm_framebuffer *fb, enum dma_data_direct
>>   			continue;
>>   		ret = dma_buf_begin_cpu_access(import_attach->dmabuf, dir);
>>   		if (ret)
>> -			goto err_dma_buf_end_cpu_access;
>> +			goto err___drm_gem_fb_end_cpu_access;
> 
> I wouldn't rename this. As I read it, the goto label doesn't denote what
> function is called but rather what action is being taken in an error path.
> 
> Since finally what's being done is a dma_buf_end_cpu_access in the helper,
> I think that just keeping err_dma_buf_end_cpu_access is enough. Also, the
> additional underscores added make it harder to read IMO.

I usually name the goto labels after the function that comes next. It's 
not really pretty here, but being inconsistent would probably annoy me 
more than the underscores.

Best regards
Thomas

> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 3/4] drm/gem-vram: Ignore planes that are unused by framebuffer format

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 2118 bytes --]

Hi

Am 16.05.22 um 15:34 schrieb Javier Martinez Canillas:
> On 5/9/22 10:16, Thomas Zimmermann wrote:
>> Only handle color planes that exist in a framebuffer's color format.
>> Ignore non-existing planes.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
> 
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> 
>> @@ -673,7 +679,10 @@ drm_gem_vram_plane_helper_prepare_fb(struct drm_plane *plane,
>>   err_drm_gem_vram_unpin:
>>   	while (i) {
>>   		--i;
>> -		gbo = drm_gem_vram_of_gem(new_state->fb->obj[i]);
>> +		obj = drm_gem_fb_get_obj(fb, i);
>> +		if (!obj)
>> +			continue;
>> +		gbo = drm_gem_vram_of_gem(obj);
>>   		drm_gem_vram_unpin(gbo);
> 
> The code in this error path to unpin the GEM vram objects...
> 
>>   	}
>>   	return ret;
>> @@ -694,16 +703,19 @@ void
>>   drm_gem_vram_plane_helper_cleanup_fb(struct drm_plane *plane,
>>   				     struct drm_plane_state *old_state)
>>   {
>> -	size_t i;
>> +	struct drm_framebuffer *fb = old_state->fb;
>>   	struct drm_gem_vram_object *gbo;
>> +	struct drm_gem_object *obj;
>> +	unsigned int i;
>>   
>> -	if (!old_state->fb)
>> +	if (!fb)
>>   		return;
>>   
>> -	for (i = 0; i < ARRAY_SIZE(old_state->fb->obj); ++i) {
>> -		if (!old_state->fb->obj[i])
>> +	for (i = 0; i < fb->format->num_planes; ++i) {
>> +		obj = drm_gem_fb_get_obj(fb, i);
>> +		if (!obj)
>>   			continue;
>> -		gbo = drm_gem_vram_of_gem(old_state->fb->obj[i]);
>> +		gbo = drm_gem_vram_of_gem(obj);
>>   		drm_gem_vram_unpin(gbo);
> 
> ... and this, seems to be basically the same.
> 
> So maybe as a follow-up you can do a similar cleanup like you did in patch
> 1/4 and use a helper function to handle both.

I was thinking the same when I wrote this patch and somehow decided 
against it.  But since you also mention it, I'm going to add this change 
to the patchset.

Best regards
Thomas

> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 4/4] drm/gem: Warn on trying to use a non-existing framebuffer plane

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 18609 bytes --]

CC: llvm(a)lists.linux.dev
CC: kbuild-all(a)lists.01.org
BCC: lkp(a)intel.com
In-Reply-To: <20220509081602.474-5-tzimmermann@suse.de>
References: <20220509081602.474-5-tzimmermann@suse.de>
TO: Thomas Zimmermann <tzimmermann@suse.de>
TO: daniel(a)ffwll.ch
TO: airlied(a)linux.ie
TO: mripard(a)kernel.org
TO: maarten.lankhorst(a)linux.intel.com
TO: noralf(a)tronnes.org
TO: christian.koenig(a)amd.com
CC: Thomas Zimmermann <tzimmermann@suse.de>
CC: dri-devel(a)lists.freedesktop.org

Hi Thomas,

I love your patch! Perhaps something to improve:

[auto build test WARNING on b0b93865a24c910fcbfa6e6fa0955fae930a56d3]

url:    https://github.com/intel-lab-lkp/linux/commits/Thomas-Zimmermann/drm-Ignore-non-existing-color-planes-in-helpers/20220509-161931
base:   b0b93865a24c910fcbfa6e6fa0955fae930a56d3
:::::: branch date: 4 days ago
:::::: commit date: 4 days ago
config: arm-randconfig-c002-20220512 (https://download.01.org/0day-ci/archive/20220513/202205132133.TO6CH9hx-lkp(a)intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 9519dacab7b8afd537811fc2abaceb4d14f4e16a)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install arm cross compiling tool for clang build
        # apt-get install binutils-arm-linux-gnueabi
        # https://github.com/intel-lab-lkp/linux/commit/74626e0c7dc11284a12f3e8c046612eec6e7bc4e
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Thomas-Zimmermann/drm-Ignore-non-existing-color-planes-in-helpers/20220509-161931
        git checkout 74626e0c7dc11284a12f3e8c046612eec6e7bc4e
        # save the config file
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=arm clang-analyzer 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


clang-analyzer warnings: (new ones prefixed by >>)
               ^~~
   lib/mpi/mpi-pow.c:71:2: note: Taking false branch
           if (!mp)
           ^
   lib/mpi/mpi-pow.c:74:6: note: 'mod_shift_cnt' is 32
           if (mod_shift_cnt)
               ^~~~~~~~~~~~~
   lib/mpi/mpi-pow.c:74:2: note: Taking true branch
           if (mod_shift_cnt)
           ^
   lib/mpi/mpi-pow.c:81:6: note: Assuming 'bsize' is <= 'msize'
           if (bsize > msize) {    /* The base is larger than the module. Reduce it. */
               ^~~~~~~~~~~~~
   lib/mpi/mpi-pow.c:81:2: note: Taking false branch
           if (bsize > msize) {    /* The base is larger than the module. Reduce it. */
           ^
   lib/mpi/mpi-pow.c:98:6: note: Assuming 'bsize' is not equal to 0
           if (!bsize) {
               ^~~~~~
   lib/mpi/mpi-pow.c:98:2: note: Taking false branch
           if (!bsize) {
           ^
   lib/mpi/mpi-pow.c:104:6: note: Assuming 'size' is <= field 'alloced'
           if (res->alloced < size) {
               ^~~~~~~~~~~~~~~~~~~
   lib/mpi/mpi-pow.c:104:2: note: Taking false branch
           if (res->alloced < size) {
           ^
   lib/mpi/mpi-pow.c:119:7: note: Assuming 'rp' is not equal to 'bp'
                   if (rp == bp) {
                       ^~~~~~~~
   lib/mpi/mpi-pow.c:119:3: note: Taking false branch
                   if (rp == bp) {
                   ^
   lib/mpi/mpi-pow.c:127:7: note: Assuming 'rp' is not equal to 'ep'
                   if (rp == ep) {
                       ^~~~~~~~
   lib/mpi/mpi-pow.c:127:3: note: Taking false branch
                   if (rp == ep) {
                   ^
   lib/mpi/mpi-pow.c:134:7: note: Assuming 'rp' is not equal to 'mp'
                   if (rp == mp) {
                       ^~~~~~~~
   lib/mpi/mpi-pow.c:134:3: note: Taking false branch
                   if (rp == mp) {
                   ^
   lib/mpi/mpi-pow.c:144:2: note: Assuming '_i' is >= 'bsize'
           MPN_COPY(rp, bp, bsize);
           ^
   lib/mpi/mpi-internal.h:65:16: note: expanded from macro 'MPN_COPY'
                   for (_i = 0; _i < (n); _i++)    \
                                ^~~~~~~~
   lib/mpi/mpi-pow.c:144:2: note: Loop condition is false. Execution continues on line 144
           MPN_COPY(rp, bp, bsize);
           ^
   lib/mpi/mpi-internal.h:65:3: note: expanded from macro 'MPN_COPY'
                   for (_i = 0; _i < (n); _i++)    \
                   ^
   lib/mpi/mpi-pow.c:144:2: note: Loop condition is false.  Exiting loop
           MPN_COPY(rp, bp, bsize);
           ^
   lib/mpi/mpi-internal.h:63:2: note: expanded from macro 'MPN_COPY'
           do {                                    \
           ^
   lib/mpi/mpi-pow.c:156:7: note: Assuming 'xp' is non-null
                   if (!xp)
                       ^~~
   lib/mpi/mpi-pow.c:156:3: note: Taking false branch
                   if (!xp)
                   ^
   lib/mpi/mpi-pow.c:159:22: note: Assuming the condition is false
                   negative_result = (ep[0] & 1) && base->sign;
                                      ^~~~~~~~~
   lib/mpi/mpi-pow.c:159:33: note: Left side of '&&' is false
                   negative_result = (ep[0] & 1) && base->sign;
                                                 ^
   lib/mpi/mpi-pow.c:163:7: note: Calling 'count_leading_zeros'
                   c = count_leading_zeros(e);
                       ^~~~~~~~~~~~~~~~~~~~~~
   include/linux/count_zeros.h:25:2: note: Taking true branch
           if (sizeof(x) == 4)
           ^
   include/linux/count_zeros.h:26:3: note: Returning the value 32
                   return BITS_PER_LONG - fls(x);
                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   lib/mpi/mpi-pow.c:163:7: note: Returning from 'count_leading_zeros'
                   c = count_leading_zeros(e);
                       ^~~~~~~~~~~~~~~~~~~~~~
   lib/mpi/mpi-pow.c:163:3: note: The value 32 is assigned to 'c'
                   c = count_leading_zeros(e);
                   ^~~~~~~~~~~~~~~~~~~~~~~~~~
   lib/mpi/mpi-pow.c:164:10: note: The result of the left shift is undefined due to shifting by '32', which is greater or equal to the width of type 'mpi_limb_t'
                   e = (e << c) << 1;      /* shift the exp bits to the left, lose msb */
                          ^  ~
   Suppressed 37 warnings (37 in non-user code).
   Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
   22 warnings generated.
   Suppressed 22 warnings (22 in non-user code).
   Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
   55 warnings generated.
>> drivers/gpu/drm/drm_gem_framebuffer_helper.c:56:21: warning: Value stored to 'dev' during its initialization is never read [clang-analyzer-deadcode.DeadStores]
           struct drm_device *dev = fb->dev;
                              ^~~   ~~~~~~~
   drivers/gpu/drm/drm_gem_framebuffer_helper.c:56:21: note: Value stored to 'dev' during its initialization is never read
           struct drm_device *dev = fb->dev;
                              ^~~   ~~~~~~~
   drivers/gpu/drm/drm_gem_framebuffer_helper.c:364:4: warning: Call to function 'memcpy' is insecure as it does not provide security checks introduced in the C11 standard. Replace with analogous functions that support length arguments or provides boundary checks such as 'memcpy_s' in case of C11 [clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling]
                           memcpy(&data[i], &map[i], sizeof(data[i]));
                           ^
   include/linux/fortify-string.h:369:26: note: expanded from macro 'memcpy'
   #define memcpy(p, q, s)  __fortify_memcpy_chk(p, q, s,                  \
                            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/fortify-string.h:362:2: note: expanded from macro '__fortify_memcpy_chk'
           __underlying_##op(p, q, __fortify_size);                        \
           ^~~~~~~~~~~~~~~~~
   note: expanded from here
   include/linux/fortify-string.h:45:29: note: expanded from macro '__underlying_memcpy'
   #define __underlying_memcpy     __builtin_memcpy
                                   ^~~~~~~~~~~~~~~~
   drivers/gpu/drm/drm_gem_framebuffer_helper.c:364:4: note: Call to function 'memcpy' is insecure as it does not provide security checks introduced in the C11 standard. Replace with analogous functions that support length arguments or provides boundary checks such as 'memcpy_s' in case of C11
                           memcpy(&data[i], &map[i], sizeof(data[i]));
                           ^
   include/linux/fortify-string.h:369:26: note: expanded from macro 'memcpy'
   #define memcpy(p, q, s)  __fortify_memcpy_chk(p, q, s,                  \
                            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/fortify-string.h:362:2: note: expanded from macro '__fortify_memcpy_chk'
           __underlying_##op(p, q, __fortify_size);                        \
           ^~~~~~~~~~~~~~~~~
   note: expanded from here
   include/linux/fortify-string.h:45:29: note: expanded from macro '__underlying_memcpy'
   #define __underlying_memcpy     __builtin_memcpy
                                   ^~~~~~~~~~~~~~~~
   Suppressed 53 warnings (53 in non-user code).
   Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
   54 warnings generated.
   drivers/gpu/drm/drm_atomic_state_helper.c:134:2: warning: Call to function 'memcpy' is insecure as it does not provide security checks introduced in the C11 standard. Replace with analogous functions that support length arguments or provides boundary checks such as 'memcpy_s' in case of C11 [clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling]
           memcpy(state, crtc->state, sizeof(*state));
           ^
   include/linux/fortify-string.h:369:26: note: expanded from macro 'memcpy'
   #define memcpy(p, q, s)  __fortify_memcpy_chk(p, q, s,                  \
                            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/fortify-string.h:362:2: note: expanded from macro '__fortify_memcpy_chk'
           __underlying_##op(p, q, __fortify_size);                        \
           ^~~~~~~~~~~~~~~~~
   note: expanded from here
   include/linux/fortify-string.h:45:29: note: expanded from macro '__underlying_memcpy'
   #define __underlying_memcpy     __builtin_memcpy
                                   ^~~~~~~~~~~~~~~~
   drivers/gpu/drm/drm_atomic_state_helper.c:134:2: note: Call to function 'memcpy' is insecure as it does not provide security checks introduced in the C11 standard. Replace with analogous functions that support length arguments or provides boundary checks such as 'memcpy_s' in case of C11
           memcpy(state, crtc->state, sizeof(*state));
           ^
   include/linux/fortify-string.h:369:26: note: expanded from macro 'memcpy'
   #define memcpy(p, q, s)  __fortify_memcpy_chk(p, q, s,                  \
                            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/fortify-string.h:362:2: note: expanded from macro '__fortify_memcpy_chk'
           __underlying_##op(p, q, __fortify_size);                        \
           ^~~~~~~~~~~~~~~~~
   note: expanded from here
   include/linux/fortify-string.h:45:29: note: expanded from macro '__underlying_memcpy'
   #define __underlying_memcpy     __builtin_memcpy
                                   ^~~~~~~~~~~~~~~~
   drivers/gpu/drm/drm_atomic_state_helper.c:331:2: warning: Call to function 'memcpy' is insecure as it does not provide security checks introduced in the C11 standard. Replace with analogous functions that support length arguments or provides boundary checks such as 'memcpy_s' in case of C11 [clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling]
           memcpy(state, plane->state, sizeof(*state));
           ^
   include/linux/fortify-string.h:369:26: note: expanded from macro 'memcpy'
   #define memcpy(p, q, s)  __fortify_memcpy_chk(p, q, s,                  \
                            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/fortify-string.h:362:2: note: expanded from macro '__fortify_memcpy_chk'
           __underlying_##op(p, q, __fortify_size);                        \
           ^~~~~~~~~~~~~~~~~
   note: expanded from here
   include/linux/fortify-string.h:45:29: note: expanded from macro '__underlying_memcpy'
   #define __underlying_memcpy     __builtin_memcpy
                                   ^~~~~~~~~~~~~~~~
   drivers/gpu/drm/drm_atomic_state_helper.c:331:2: note: Call to function 'memcpy' is insecure as it does not provide security checks introduced in the C11 standard. Replace with analogous functions that support length arguments or provides boundary checks such as 'memcpy_s' in case of C11
           memcpy(state, plane->state, sizeof(*state));
           ^
   include/linux/fortify-string.h:369:26: note: expanded from macro 'memcpy'
   #define memcpy(p, q, s)  __fortify_memcpy_chk(p, q, s,                  \
                            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/fortify-string.h:362:2: note: expanded from macro '__fortify_memcpy_chk'
           __underlying_##op(p, q, __fortify_size);                        \
           ^~~~~~~~~~~~~~~~~
   note: expanded from here
   include/linux/fortify-string.h:45:29: note: expanded from macro '__underlying_memcpy'
   #define __underlying_memcpy     __builtin_memcpy
                                   ^~~~~~~~~~~~~~~~
   drivers/gpu/drm/drm_atomic_state_helper.c:494:2: warning: Call to function 'memcpy' is insecure as it does not provide security checks introduced in the C11 standard. Replace with analogous functions that support length arguments or provides boundary checks such as 'memcpy_s' in case of C11 [clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling]
           memcpy(state, connector->state, sizeof(*state));
           ^
   include/linux/fortify-string.h:369:26: note: expanded from macro 'memcpy'
   #define memcpy(p, q, s)  __fortify_memcpy_chk(p, q, s,                  \
                            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/fortify-string.h:362:2: note: expanded from macro '__fortify_memcpy_chk'
           __underlying_##op(p, q, __fortify_size);                        \
           ^~~~~~~~~~~~~~~~~
   note: expanded from here
   include/linux/fortify-string.h:45:29: note: expanded from macro '__underlying_memcpy'
   #define __underlying_memcpy     __builtin_memcpy
                                   ^~~~~~~~~~~~~~~~
   drivers/gpu/drm/drm_atomic_state_helper.c:494:2: note: Call to function 'memcpy' is insecure as it does not provide security checks introduced in the C11 standard. Replace with analogous functions that support length arguments or provides boundary checks such as 'memcpy_s' in case of C11

vim +/dev +56 drivers/gpu/drm/drm_gem_framebuffer_helper.c

55f7f72753abdd Andrzej Pietrasiewicz 2020-03-11  29  
4c3dbb2c312c9f Noralf Trønnes        2017-08-13  30  /**
4c3dbb2c312c9f Noralf Trønnes        2017-08-13  31   * DOC: overview
4c3dbb2c312c9f Noralf Trønnes        2017-08-13  32   *
4c3dbb2c312c9f Noralf Trønnes        2017-08-13  33   * This library provides helpers for drivers that don't subclass
2e187b2099034a Noralf Trønnes        2017-09-22  34   * &drm_framebuffer and use &drm_gem_object for their backing storage.
4c3dbb2c312c9f Noralf Trønnes        2017-08-13  35   *
4c3dbb2c312c9f Noralf Trønnes        2017-08-13  36   * Drivers without additional needs to validate framebuffers can simply use
2e187b2099034a Noralf Trønnes        2017-09-22  37   * drm_gem_fb_create() and everything is wired up automatically. Other drivers
2e187b2099034a Noralf Trønnes        2017-09-22  38   * can use all parts independently.
4c3dbb2c312c9f Noralf Trønnes        2017-08-13  39   */
4c3dbb2c312c9f Noralf Trønnes        2017-08-13  40  
4c3dbb2c312c9f Noralf Trønnes        2017-08-13  41  /**
2e187b2099034a Noralf Trønnes        2017-09-22  42   * drm_gem_fb_get_obj() - Get GEM object backing the framebuffer
2e187b2099034a Noralf Trønnes        2017-09-22  43   * @fb: Framebuffer
2e187b2099034a Noralf Trønnes        2017-09-22  44   * @plane: Plane index
4c3dbb2c312c9f Noralf Trønnes        2017-08-13  45   *
2e187b2099034a Noralf Trønnes        2017-09-22  46   * No additional reference is taken beyond the one that the &drm_frambuffer
2e187b2099034a Noralf Trønnes        2017-09-22  47   * already holds.
2e187b2099034a Noralf Trønnes        2017-09-22  48   *
2e187b2099034a Noralf Trønnes        2017-09-22  49   * Returns:
2e187b2099034a Noralf Trønnes        2017-09-22  50   * Pointer to &drm_gem_object for the given framebuffer and plane index or NULL
2e187b2099034a Noralf Trønnes        2017-09-22  51   * if it does not exist.
4c3dbb2c312c9f Noralf Trønnes        2017-08-13  52   */
4c3dbb2c312c9f Noralf Trønnes        2017-08-13  53  struct drm_gem_object *drm_gem_fb_get_obj(struct drm_framebuffer *fb,
4c3dbb2c312c9f Noralf Trønnes        2017-08-13  54  					  unsigned int plane)
4c3dbb2c312c9f Noralf Trønnes        2017-08-13  55  {
74626e0c7dc112 Thomas Zimmermann     2022-05-09 @56  	struct drm_device *dev = fb->dev;
74626e0c7dc112 Thomas Zimmermann     2022-05-09  57  
74626e0c7dc112 Thomas Zimmermann     2022-05-09  58  	if (drm_WARN_ON_ONCE(dev, plane >= ARRAY_SIZE(fb->obj)))
74626e0c7dc112 Thomas Zimmermann     2022-05-09  59  		return NULL;
74626e0c7dc112 Thomas Zimmermann     2022-05-09  60  	else if (drm_WARN_ON_ONCE(dev, !fb->obj[plane]))
4c3dbb2c312c9f Noralf Trønnes        2017-08-13  61  		return NULL;
4c3dbb2c312c9f Noralf Trønnes        2017-08-13  62  
4c3dbb2c312c9f Noralf Trønnes        2017-08-13  63  	return fb->obj[plane];
4c3dbb2c312c9f Noralf Trønnes        2017-08-13  64  }
4c3dbb2c312c9f Noralf Trønnes        2017-08-13  65  EXPORT_SYMBOL_GPL(drm_gem_fb_get_obj);
4c3dbb2c312c9f Noralf Trønnes        2017-08-13  66  

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp