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+rene...@ideasonboard.com>
>
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+rene...@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;
> >
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
