diff mbox series

[4/9] drm: rcar-du: Use DRM-managed allocation for VSP planes

Message ID 20201204220139.15272-5-laurent.pinchart+renesas@ideasonboard.com (mailing list archive)
State Superseded
Delegated to: Kieran Bingham
Headers show
Series drm: rcar-du: Fix LVDS-related crash | expand

Commit Message

Laurent Pinchart Dec. 4, 2020, 10:01 p.m. UTC
devm_kcalloc() is the wrong API to allocate planes, as the lifetime of
the planes is tied to the DRM device, not the device to driver
binding. drmm_kcalloc() isn't a good option either, as it would result
in the planes being freed before being unregistered during the managed
cleanup of the DRM objects. Use a plain kcalloc(), and cleanup the
planes and free the memory in the existing rcar_du_vsp_cleanup()
handler.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

Comments

Kieran Bingham Dec. 14, 2020, 4:20 p.m. UTC | #1
Hi Laurent,

On 04/12/2020 22:01, Laurent Pinchart wrote:
> devm_kcalloc() is the wrong API to allocate planes, as the lifetime of
> the planes is tied to the DRM device, not the device to driver
> binding. drmm_kcalloc() isn't a good option either, as it would result
> in the planes being freed before being unregistered during the managed
> cleanup of the DRM objects. Use a plain kcalloc(), and cleanup the
> planes and free the memory in the existing rcar_du_vsp_cleanup()
> handler.

Managed memory always seems to hurt - which is a shame, because it
should be better throughout.

It's like we need a way to arbitrarily specify the lifetimes of objects
correctly against another object... without being tied to a dev ...

Anyway,

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>  drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 22 +++++++++++++++++-----
>  1 file changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> index 4dcb1bfbe201..78a886651d9f 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> @@ -21,6 +21,7 @@
>  #include <linux/dma-mapping.h>
>  #include <linux/of_platform.h>
>  #include <linux/scatterlist.h>
> +#include <linux/slab.h>
>  #include <linux/videodev2.h>
>  
>  #include <media/vsp1.h>
> @@ -344,6 +345,15 @@ static const struct drm_plane_funcs rcar_du_vsp_plane_funcs = {
>  static void rcar_du_vsp_cleanup(struct drm_device *dev, void *res)
>  {
>  	struct rcar_du_vsp *vsp = res;
> +	unsigned int i;
> +
> +	for (i = 0; i < vsp->num_planes; ++i) {
> +		struct rcar_du_vsp_plane *plane = &vsp->planes[i];
> +
> +		drm_plane_cleanup(&plane->plane);
> +	}
> +
> +	kfree(vsp->planes);
>  
>  	put_device(vsp->vsp);
>  }
> @@ -354,6 +364,7 @@ int rcar_du_vsp_init(struct rcar_du_vsp *vsp, struct device_node *np,
>  	struct rcar_du_device *rcdu = vsp->dev;
>  	struct platform_device *pdev;
>  	unsigned int num_crtcs = hweight32(crtcs);
> +	unsigned int num_planes;
>  	unsigned int i;
>  	int ret;
>  
> @@ -376,14 +387,13 @@ int rcar_du_vsp_init(struct rcar_du_vsp *vsp, struct device_node *np,
>  	  * The VSP2D (Gen3) has 5 RPFs, but the VSP1D (Gen2) is limited to
>  	  * 4 RPFs.
>  	  */
> -	vsp->num_planes = rcdu->info->gen >= 3 ? 5 : 4;
> +	num_planes = rcdu->info->gen >= 3 ? 5 : 4;
>  
> -	vsp->planes = devm_kcalloc(rcdu->dev, vsp->num_planes,
> -				   sizeof(*vsp->planes), GFP_KERNEL);
> +	vsp->planes = kcalloc(num_planes, sizeof(*vsp->planes), GFP_KERNEL);
>  	if (!vsp->planes)
>  		return -ENOMEM;
>  
> -	for (i = 0; i < vsp->num_planes; ++i) {
> +	for (i = 0; i < num_planes; ++i) {
>  		enum drm_plane_type type = i < num_crtcs
>  					 ? DRM_PLANE_TYPE_PRIMARY
>  					 : DRM_PLANE_TYPE_OVERLAY;
> @@ -409,8 +419,10 @@ int rcar_du_vsp_init(struct rcar_du_vsp *vsp, struct device_node *np,
>  		} else {
>  			drm_plane_create_alpha_property(&plane->plane);
>  			drm_plane_create_zpos_property(&plane->plane, 1, 1,
> -						       vsp->num_planes - 1);
> +						       num_planes - 1);
>  		}
> +
> +		vsp->num_planes++;>  	}
>  
>  	return 0;
>
Laurent Pinchart Dec. 14, 2020, 4:26 p.m. UTC | #2
Hi Kieran,

On Mon, Dec 14, 2020 at 04:20:17PM +0000, Kieran Bingham wrote:
> On 04/12/2020 22:01, Laurent Pinchart wrote:
> > devm_kcalloc() is the wrong API to allocate planes, as the lifetime of
> > the planes is tied to the DRM device, not the device to driver
> > binding. drmm_kcalloc() isn't a good option either, as it would result
> > in the planes being freed before being unregistered during the managed
> > cleanup of the DRM objects. Use a plain kcalloc(), and cleanup the
> > planes and free the memory in the existing rcar_du_vsp_cleanup()
> > handler.
> 
> Managed memory always seems to hurt - which is a shame, because it
> should be better throughout.
> 
> It's like we need a way to arbitrarily specify the lifetimes of objects
> correctly against another object... without being tied to a dev ...

I've been saying for years that devm_kzalloc() is a major regression.
We've traded a memory leak for a use-after-free. The function has its
use cases, there are objects that need to match the lifetime of the
binding between a device and its driver, but that's a small minority.

> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> >  drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 22 +++++++++++++++++-----
> >  1 file changed, 17 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> > index 4dcb1bfbe201..78a886651d9f 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> > @@ -21,6 +21,7 @@
> >  #include <linux/dma-mapping.h>
> >  #include <linux/of_platform.h>
> >  #include <linux/scatterlist.h>
> > +#include <linux/slab.h>
> >  #include <linux/videodev2.h>
> >  
> >  #include <media/vsp1.h>
> > @@ -344,6 +345,15 @@ static const struct drm_plane_funcs rcar_du_vsp_plane_funcs = {
> >  static void rcar_du_vsp_cleanup(struct drm_device *dev, void *res)
> >  {
> >  	struct rcar_du_vsp *vsp = res;
> > +	unsigned int i;
> > +
> > +	for (i = 0; i < vsp->num_planes; ++i) {
> > +		struct rcar_du_vsp_plane *plane = &vsp->planes[i];
> > +
> > +		drm_plane_cleanup(&plane->plane);
> > +	}
> > +
> > +	kfree(vsp->planes);
> >  
> >  	put_device(vsp->vsp);
> >  }
> > @@ -354,6 +364,7 @@ int rcar_du_vsp_init(struct rcar_du_vsp *vsp, struct device_node *np,
> >  	struct rcar_du_device *rcdu = vsp->dev;
> >  	struct platform_device *pdev;
> >  	unsigned int num_crtcs = hweight32(crtcs);
> > +	unsigned int num_planes;
> >  	unsigned int i;
> >  	int ret;
> >  
> > @@ -376,14 +387,13 @@ int rcar_du_vsp_init(struct rcar_du_vsp *vsp, struct device_node *np,
> >  	  * The VSP2D (Gen3) has 5 RPFs, but the VSP1D (Gen2) is limited to
> >  	  * 4 RPFs.
> >  	  */
> > -	vsp->num_planes = rcdu->info->gen >= 3 ? 5 : 4;
> > +	num_planes = rcdu->info->gen >= 3 ? 5 : 4;
> >  
> > -	vsp->planes = devm_kcalloc(rcdu->dev, vsp->num_planes,
> > -				   sizeof(*vsp->planes), GFP_KERNEL);
> > +	vsp->planes = kcalloc(num_planes, sizeof(*vsp->planes), GFP_KERNEL);
> >  	if (!vsp->planes)
> >  		return -ENOMEM;
> >  
> > -	for (i = 0; i < vsp->num_planes; ++i) {
> > +	for (i = 0; i < num_planes; ++i) {
> >  		enum drm_plane_type type = i < num_crtcs
> >  					 ? DRM_PLANE_TYPE_PRIMARY
> >  					 : DRM_PLANE_TYPE_OVERLAY;
> > @@ -409,8 +419,10 @@ int rcar_du_vsp_init(struct rcar_du_vsp *vsp, struct device_node *np,
> >  		} else {
> >  			drm_plane_create_alpha_property(&plane->plane);
> >  			drm_plane_create_zpos_property(&plane->plane, 1, 1,
> > -						       vsp->num_planes - 1);
> > +						       num_planes - 1);
> >  		}
> > +
> > +		vsp->num_planes++;>  	}
> >  
> >  	return 0;
> >
Geert Uytterhoeven Dec. 15, 2020, 3:42 p.m. UTC | #3
Hi Laurent,

On Mon, Dec 14, 2020 at 5:28 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Mon, Dec 14, 2020 at 04:20:17PM +0000, Kieran Bingham wrote:
> > On 04/12/2020 22:01, Laurent Pinchart wrote:
> > > devm_kcalloc() is the wrong API to allocate planes, as the lifetime of
> > > the planes is tied to the DRM device, not the device to driver
> > > binding. drmm_kcalloc() isn't a good option either, as it would result
> > > in the planes being freed before being unregistered during the managed
> > > cleanup of the DRM objects. Use a plain kcalloc(), and cleanup the
> > > planes and free the memory in the existing rcar_du_vsp_cleanup()
> > > handler.
> >
> > Managed memory always seems to hurt - which is a shame, because it
> > should be better throughout.
> >
> > It's like we need a way to arbitrarily specify the lifetimes of objects
> > correctly against another object... without being tied to a dev ...
>
> I've been saying for years that devm_kzalloc() is a major regression.
> We've traded a memory leak for a use-after-free. The function has its
> use cases, there are objects that need to match the lifetime of the
> binding between a device and its driver, but that's a small minority.

https://en.wikipedia.org/wiki/The_law_of_conservation_of_misery

Gr{oetje,eeting}s,

                        Geert
diff mbox series

Patch

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
index 4dcb1bfbe201..78a886651d9f 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
@@ -21,6 +21,7 @@ 
 #include <linux/dma-mapping.h>
 #include <linux/of_platform.h>
 #include <linux/scatterlist.h>
+#include <linux/slab.h>
 #include <linux/videodev2.h>
 
 #include <media/vsp1.h>
@@ -344,6 +345,15 @@  static const struct drm_plane_funcs rcar_du_vsp_plane_funcs = {
 static void rcar_du_vsp_cleanup(struct drm_device *dev, void *res)
 {
 	struct rcar_du_vsp *vsp = res;
+	unsigned int i;
+
+	for (i = 0; i < vsp->num_planes; ++i) {
+		struct rcar_du_vsp_plane *plane = &vsp->planes[i];
+
+		drm_plane_cleanup(&plane->plane);
+	}
+
+	kfree(vsp->planes);
 
 	put_device(vsp->vsp);
 }
@@ -354,6 +364,7 @@  int rcar_du_vsp_init(struct rcar_du_vsp *vsp, struct device_node *np,
 	struct rcar_du_device *rcdu = vsp->dev;
 	struct platform_device *pdev;
 	unsigned int num_crtcs = hweight32(crtcs);
+	unsigned int num_planes;
 	unsigned int i;
 	int ret;
 
@@ -376,14 +387,13 @@  int rcar_du_vsp_init(struct rcar_du_vsp *vsp, struct device_node *np,
 	  * The VSP2D (Gen3) has 5 RPFs, but the VSP1D (Gen2) is limited to
 	  * 4 RPFs.
 	  */
-	vsp->num_planes = rcdu->info->gen >= 3 ? 5 : 4;
+	num_planes = rcdu->info->gen >= 3 ? 5 : 4;
 
-	vsp->planes = devm_kcalloc(rcdu->dev, vsp->num_planes,
-				   sizeof(*vsp->planes), GFP_KERNEL);
+	vsp->planes = kcalloc(num_planes, sizeof(*vsp->planes), GFP_KERNEL);
 	if (!vsp->planes)
 		return -ENOMEM;
 
-	for (i = 0; i < vsp->num_planes; ++i) {
+	for (i = 0; i < num_planes; ++i) {
 		enum drm_plane_type type = i < num_crtcs
 					 ? DRM_PLANE_TYPE_PRIMARY
 					 : DRM_PLANE_TYPE_OVERLAY;
@@ -409,8 +419,10 @@  int rcar_du_vsp_init(struct rcar_du_vsp *vsp, struct device_node *np,
 		} else {
 			drm_plane_create_alpha_property(&plane->plane);
 			drm_plane_create_zpos_property(&plane->plane, 1, 1,
-						       vsp->num_planes - 1);
+						       num_planes - 1);
 		}
+
+		vsp->num_planes++;
 	}
 
 	return 0;