| 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 |
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; >
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; > >
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 --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;
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(-)