[PATCH v2 0/2] drm/vkms: check plane_composer->map[0] before using it

[PATCH v2 0/2] drm/vkms: check plane_composer->map[0] before using it

[Tales Lelo da Aparecida]

Hi, this is a follow-up of my last patch. Thanks for the reviews!

Changes from v1:

Edit the first commit message as suggested by Melissa and add a commit to enhance the
error handling (André)

Tales Lelo da Aparecida (2):
  drm/vkms: check plane_composer->map[0] before using it
  drm/vkms: return early if compose_plane fails

 drivers/gpu/drm/vkms/vkms_composer.c | 32 ++++++++++++++++++----------
 1 file changed, 21 insertions(+), 11 deletions(-)

-- 
2.35.1

[PATCH v2 1/2] drm/vkms: check plane_composer->map[0] before using it

[Tales Lelo da Aparecida]

Fix a copypasta error. The caller of compose_plane() already checks
primary_composer->map. In contrast, plane_composer->map is never
verified here before handling.

Fixes: 7938f4218168 ("dma-buf-map: Rename to iosys-map")
Reviewed-by: André Almeida <andrealmeid@riseup.net>
Signed-off-by: Tales Lelo da Aparecida <tales.aparecida@gmail.com>
---
v2: detail the commit message with more information

 drivers/gpu/drm/vkms/vkms_composer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
index c6a1036bf2ea..b47ac170108c 100644
--- a/drivers/gpu/drm/vkms/vkms_composer.c
+++ b/drivers/gpu/drm/vkms/vkms_composer.c
@@ -157,7 +157,7 @@ static void compose_plane(struct vkms_composer *primary_composer,
 	void *vaddr;
 	void (*pixel_blend)(const u8 *p_src, u8 *p_dst);
 
-	if (WARN_ON(iosys_map_is_null(&primary_composer->map[0])))
+	if (WARN_ON(iosys_map_is_null(&plane_composer->map[0])))
 		return;
 
 	vaddr = plane_composer->map[0].vaddr;
-- 
2.35.1

[PATCH v2 2/2] drm/vkms: return early if compose_plane fails

[Tales Lelo da Aparecida]

Do not exit quietly from compose_plane. If any plane has an invalid
map, propagate the error value upwards. While here, add log messages
for the invalid index.

Signed-off-by: Tales Lelo da Aparecida <tales.aparecida@gmail.com>
---
 drivers/gpu/drm/vkms/vkms_composer.c | 30 ++++++++++++++++++----------
 1 file changed, 20 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
index b47ac170108c..c0a3b53cd155 100644
--- a/drivers/gpu/drm/vkms/vkms_composer.c
+++ b/drivers/gpu/drm/vkms/vkms_composer.c
@@ -149,16 +149,16 @@ static void blend(void *vaddr_dst, void *vaddr_src,
 	}
 }
 
-static void compose_plane(struct vkms_composer *primary_composer,
-			  struct vkms_composer *plane_composer,
-			  void *vaddr_out)
+static int compose_plane(struct vkms_composer *primary_composer,
+			 struct vkms_composer *plane_composer,
+			 void *vaddr_out)
 {
 	struct drm_framebuffer *fb = &plane_composer->fb;
 	void *vaddr;
 	void (*pixel_blend)(const u8 *p_src, u8 *p_dst);
 
 	if (WARN_ON(iosys_map_is_null(&plane_composer->map[0])))
-		return;
+		return -EINVAL;
 
 	vaddr = plane_composer->map[0].vaddr;
 
@@ -168,6 +168,8 @@ static void compose_plane(struct vkms_composer *primary_composer,
 		pixel_blend = &x_blend;
 
 	blend(vaddr_out, vaddr, primary_composer, plane_composer, pixel_blend);
+
+	return 0;
 }
 
 static int compose_active_planes(void **vaddr_out,
@@ -177,7 +179,7 @@ static int compose_active_planes(void **vaddr_out,
 	struct drm_framebuffer *fb = &primary_composer->fb;
 	struct drm_gem_object *gem_obj = drm_gem_fb_get_obj(fb, 0);
 	const void *vaddr;
-	int i;
+	int i, ret;
 
 	if (!*vaddr_out) {
 		*vaddr_out = kzalloc(gem_obj->size, GFP_KERNEL);
@@ -187,8 +189,10 @@ static int compose_active_planes(void **vaddr_out,
 		}
 	}
 
-	if (WARN_ON(iosys_map_is_null(&primary_composer->map[0])))
+	if (WARN_ON(iosys_map_is_null(&primary_composer->map[0]))) {
+		DRM_DEBUG_DRIVER("Failed to compose. Invalid map in the primary plane.");
 		return -EINVAL;
+	}
 
 	vaddr = primary_composer->map[0].vaddr;
 
@@ -198,10 +202,16 @@ static int compose_active_planes(void **vaddr_out,
 	 * planes should be in z-order and compose them associatively:
 	 * ((primary <- overlay) <- cursor)
 	 */
-	for (i = 1; i < crtc_state->num_active_planes; i++)
-		compose_plane(primary_composer,
-			      crtc_state->active_planes[i]->composer,
-			      *vaddr_out);
+	for (i = 1; i < crtc_state->num_active_planes; i++) {
+		ret = compose_plane(primary_composer,
+				    crtc_state->active_planes[i]->composer,
+				    *vaddr_out);
+		if (ret) {
+			DRM_DEBUG_DRIVER("Failed to compose. Invalid map in the active_planes[%d].",
+					 i);
+			return ret;
+		}
+	}
 
 	return 0;
 }
-- 
2.35.1

Re: [PATCH v2 2/2] drm/vkms: return early if compose_plane fails

[Melissa Wen]

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

On 04/15, Tales Lelo da Aparecida wrote:
> Do not exit quietly from compose_plane. If any plane has an invalid
> map, propagate the error value upwards. While here, add log messages
> for the invalid index.
> 
> Signed-off-by: Tales Lelo da Aparecida <tales.aparecida@gmail.com>
> ---
>  drivers/gpu/drm/vkms/vkms_composer.c | 30 ++++++++++++++++++----------
>  1 file changed, 20 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
> index b47ac170108c..c0a3b53cd155 100644
> --- a/drivers/gpu/drm/vkms/vkms_composer.c
> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> @@ -149,16 +149,16 @@ static void blend(void *vaddr_dst, void *vaddr_src,
>  	}
>  }
>  
> -static void compose_plane(struct vkms_composer *primary_composer,
> -			  struct vkms_composer *plane_composer,
> -			  void *vaddr_out)
> +static int compose_plane(struct vkms_composer *primary_composer,
> +			 struct vkms_composer *plane_composer,
> +			 void *vaddr_out)
>  {
>  	struct drm_framebuffer *fb = &plane_composer->fb;
>  	void *vaddr;
>  	void (*pixel_blend)(const u8 *p_src, u8 *p_dst);
>  
>  	if (WARN_ON(iosys_map_is_null(&plane_composer->map[0])))
> -		return;
> +		return -EINVAL;
>  
>  	vaddr = plane_composer->map[0].vaddr;
>  
> @@ -168,6 +168,8 @@ static void compose_plane(struct vkms_composer *primary_composer,
>  		pixel_blend = &x_blend;
>  
>  	blend(vaddr_out, vaddr, primary_composer, plane_composer, pixel_blend);
> +
> +	return 0;
>  }
>  
>  static int compose_active_planes(void **vaddr_out,
> @@ -177,7 +179,7 @@ static int compose_active_planes(void **vaddr_out,
>  	struct drm_framebuffer *fb = &primary_composer->fb;
>  	struct drm_gem_object *gem_obj = drm_gem_fb_get_obj(fb, 0);
>  	const void *vaddr;
> -	int i;
> +	int i, ret;
>  
>  	if (!*vaddr_out) {
>  		*vaddr_out = kzalloc(gem_obj->size, GFP_KERNEL);
> @@ -187,8 +189,10 @@ static int compose_active_planes(void **vaddr_out,
>  		}
>  	}
>  
> -	if (WARN_ON(iosys_map_is_null(&primary_composer->map[0])))
> +	if (WARN_ON(iosys_map_is_null(&primary_composer->map[0]))) {
> +		DRM_DEBUG_DRIVER("Failed to compose. Invalid map in the primary plane.");
>  		return -EINVAL;
yes, good to have a debug msg here. I would say `mapping`
> +	}
>  
>  	vaddr = primary_composer->map[0].vaddr;
>  
> @@ -198,10 +202,16 @@ static int compose_active_planes(void **vaddr_out,
>  	 * planes should be in z-order and compose them associatively:
>  	 * ((primary <- overlay) <- cursor)
>  	 */
> -	for (i = 1; i < crtc_state->num_active_planes; i++)
> -		compose_plane(primary_composer,
> -			      crtc_state->active_planes[i]->composer,
> -			      *vaddr_out);
> +	for (i = 1; i < crtc_state->num_active_planes; i++) {
> +		ret = compose_plane(primary_composer,
> +				    crtc_state->active_planes[i]->composer,
> +				    *vaddr_out);

tbh, I'm not sure on changing compose_plane behaviour here to
invalidate all composition in case of a invalid active plane mapping.

Warning about an inconsistent composition makes sense to me, but it
would be better if we can prevent all resources consumption around each plane
composition by checking the issue as soon as possible. One idea would be doing
this validation at the time of active_plane selection. Another would be just
check all active_plane mapping right before this loop.

What do you think?

> +		if (ret) {
> +			DRM_DEBUG_DRIVER("Failed to compose. Invalid map in the active_planes[%d].",
> +					 i);
> +			return ret;
> +		}
> +	}
>  
>  	return 0;
>  }
> -- 
> 2.35.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 2/2] drm/vkms: return early if compose_plane fails

[Melissa Wen]

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

On 04/15, Tales Lelo da Aparecida wrote:
> Do not exit quietly from compose_plane. If any plane has an invalid
> map, propagate the error value upwards. While here, add log messages
> for the invalid index.
> 
> Signed-off-by: Tales Lelo da Aparecida <tales.aparecida@gmail.com>
> ---
>  drivers/gpu/drm/vkms/vkms_composer.c | 30 ++++++++++++++++++----------
>  1 file changed, 20 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
> index b47ac170108c..c0a3b53cd155 100644
> --- a/drivers/gpu/drm/vkms/vkms_composer.c
> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> @@ -149,16 +149,16 @@ static void blend(void *vaddr_dst, void *vaddr_src,
>  	}
>  }
>  
> -static void compose_plane(struct vkms_composer *primary_composer,
> -			  struct vkms_composer *plane_composer,
> -			  void *vaddr_out)
> +static int compose_plane(struct vkms_composer *primary_composer,
> +			 struct vkms_composer *plane_composer,
> +			 void *vaddr_out)
>  {
>  	struct drm_framebuffer *fb = &plane_composer->fb;
>  	void *vaddr;
>  	void (*pixel_blend)(const u8 *p_src, u8 *p_dst);
>  
>  	if (WARN_ON(iosys_map_is_null(&plane_composer->map[0])))
> -		return;
> +		return -EINVAL;
>  
>  	vaddr = plane_composer->map[0].vaddr;
>  
> @@ -168,6 +168,8 @@ static void compose_plane(struct vkms_composer *primary_composer,
>  		pixel_blend = &x_blend;
>  
>  	blend(vaddr_out, vaddr, primary_composer, plane_composer, pixel_blend);
> +
> +	return 0;
>  }
>  
>  static int compose_active_planes(void **vaddr_out,
> @@ -177,7 +179,7 @@ static int compose_active_planes(void **vaddr_out,
>  	struct drm_framebuffer *fb = &primary_composer->fb;
>  	struct drm_gem_object *gem_obj = drm_gem_fb_get_obj(fb, 0);
>  	const void *vaddr;
> -	int i;
> +	int i, ret;
>  
>  	if (!*vaddr_out) {
>  		*vaddr_out = kzalloc(gem_obj->size, GFP_KERNEL);
> @@ -187,8 +189,10 @@ static int compose_active_planes(void **vaddr_out,
>  		}
>  	}
>  
> -	if (WARN_ON(iosys_map_is_null(&primary_composer->map[0])))
> +	if (WARN_ON(iosys_map_is_null(&primary_composer->map[0]))) {
> +		DRM_DEBUG_DRIVER("Failed to compose. Invalid map in the primary plane.");
>  		return -EINVAL;
yes, good to have a debug msg here. I would say `mapping`
> +	}
>  
>  	vaddr = primary_composer->map[0].vaddr;
>  
> @@ -198,10 +202,16 @@ static int compose_active_planes(void **vaddr_out,
>  	 * planes should be in z-order and compose them associatively:
>  	 * ((primary <- overlay) <- cursor)
>  	 */
> -	for (i = 1; i < crtc_state->num_active_planes; i++)
> -		compose_plane(primary_composer,
> -			      crtc_state->active_planes[i]->composer,
> -			      *vaddr_out);
> +	for (i = 1; i < crtc_state->num_active_planes; i++) {
> +		ret = compose_plane(primary_composer,
> +				    crtc_state->active_planes[i]->composer,
> +				    *vaddr_out);

tbh, I'm not sure on changing compose_plane behaviour here to
invalidate all composition in case of a invalid active plane mapping.

Warning about an inconsistent composition makes sense to me, but it
would be better if we can prevent all resources consumption around each plane
composition by checking the issue as soon as possible. One idea would be doing
this validation at the time of active_plane selection. Another would be just
check all active_plane mapping right before this loop.

What do you think?

> +		if (ret) {
> +			DRM_DEBUG_DRIVER("Failed to compose. Invalid map in the active_planes[%d].",
> +					 i);
> +			return ret;
> +		}
> +	}
>  
>  	return 0;
>  }
> -- 
> 2.35.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/2] drm/vkms: check plane_composer->map[0] before using it

[Melissa Wen]

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

On 04/15, Tales Lelo da Aparecida wrote:
> Fix a copypasta error. The caller of compose_plane() already checks
> primary_composer->map. In contrast, plane_composer->map is never
> verified here before handling.
> 
> Fixes: 7938f4218168 ("dma-buf-map: Rename to iosys-map")
> Reviewed-by: André Almeida <andrealmeid@riseup.net>
> Signed-off-by: Tales Lelo da Aparecida <tales.aparecida@gmail.com>
> ---
> v2: detail the commit message with more information
> 
>  drivers/gpu/drm/vkms/vkms_composer.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
> index c6a1036bf2ea..b47ac170108c 100644
> --- a/drivers/gpu/drm/vkms/vkms_composer.c
> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> @@ -157,7 +157,7 @@ static void compose_plane(struct vkms_composer *primary_composer,
>  	void *vaddr;
>  	void (*pixel_blend)(const u8 *p_src, u8 *p_dst);
>  
> -	if (WARN_ON(iosys_map_is_null(&primary_composer->map[0])))
> +	if (WARN_ON(iosys_map_is_null(&plane_composer->map[0])))
>  		return;

I cherry-picked this one and applied to drm-misc-next.

Thanks,

Melissa

>  
>  	vaddr = plane_composer->map[0].vaddr;
> -- 
> 2.35.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/2] drm/vkms: check plane_composer->map[0] before using it

[Melissa Wen]

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

On 04/15, Tales Lelo da Aparecida wrote:
> Fix a copypasta error. The caller of compose_plane() already checks
> primary_composer->map. In contrast, plane_composer->map is never
> verified here before handling.
> 
> Fixes: 7938f4218168 ("dma-buf-map: Rename to iosys-map")
> Reviewed-by: André Almeida <andrealmeid@riseup.net>
> Signed-off-by: Tales Lelo da Aparecida <tales.aparecida@gmail.com>
> ---
> v2: detail the commit message with more information
> 
>  drivers/gpu/drm/vkms/vkms_composer.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
> index c6a1036bf2ea..b47ac170108c 100644
> --- a/drivers/gpu/drm/vkms/vkms_composer.c
> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> @@ -157,7 +157,7 @@ static void compose_plane(struct vkms_composer *primary_composer,
>  	void *vaddr;
>  	void (*pixel_blend)(const u8 *p_src, u8 *p_dst);
>  
> -	if (WARN_ON(iosys_map_is_null(&primary_composer->map[0])))
> +	if (WARN_ON(iosys_map_is_null(&plane_composer->map[0])))
>  		return;

I cherry-picked this one and applied to drm-misc-next.

Thanks,

Melissa

>  
>  	vaddr = plane_composer->map[0].vaddr;
> -- 
> 2.35.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]