On Wed, Apr 15, 2020 at 03:45:50PM +0300, Laurent Pinchart wrote:
> Hi Tomi,
>
> Thank you for the patch.
>
> On Wed, Apr 15, 2020 at 12:20:06PM +0300, Tomi Valkeinen wrote:
> > tidss uses devm_kzalloc to allocate DRM plane, encoder and crtc objects.
> > This is not correct as the lifetime of those objects should be longer
> > than the underlying device's.
> >
> > When unloading tidss module, the devm_kzalloc'ed objects have already
> > been freed when tidss_release() is called, and the driver will accesses
> > freed memory possibly causing a crash, a kernel WARN, or other undefined
> > behavior, and also KASAN will give a bug.
> >
> > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@xxxxxx>
> > ---
> > drivers/gpu/drm/tidss/tidss_crtc.c | 16 +++++++++++++---
> > drivers/gpu/drm/tidss/tidss_encoder.c | 14 +++++++++++---
> > drivers/gpu/drm/tidss/tidss_plane.c | 24 ++++++++++++++++++------
> > 3 files changed, 42 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/tidss/tidss_crtc.c b/drivers/gpu/drm/tidss/tidss_crtc.c
> > index d4ce9bab8c7e..3221a707e073 100644
> > --- a/drivers/gpu/drm/tidss/tidss_crtc.c
> > +++ b/drivers/gpu/drm/tidss/tidss_crtc.c
> > @@ -379,9 +379,17 @@ static struct drm_crtc_state *tidss_crtc_duplicate_state(struct drm_crtc *crtc)
> > return &state->base;
> > }
> >
> > +static void tidss_crtc_destroy(struct drm_crtc *crtc)
> > +{
> > + struct tidss_crtc *tcrtc = to_tidss_crtc(crtc);
> > +
> > + drm_crtc_cleanup(crtc);
> > + kfree(tcrtc);
>
> I would personally store the CRTC pointers, or embed the CRTC instances
> in the tidss_device structure, and free everything in the top-level
> tidss_release() handler, to avoid spreading the release code all around
> the driver. Same for planes and encoders. It may be a matter of personal
> taste though, but it would allow dropping the kfree() calls in
> individual error paths and centralize them in a single place if you
> store the allocated pointer in tidss_device right after allocation.
I'm working (well plan to at least) on some nice infrastructure so that
all this can be garbage collected again. I think embeddeding into the
top-level structure is only neat if you have a very simple device (and
then maybe just embed the drm_simple_kms thing). tidss didn't look quite
that simple, but maybe I'm missing the big picture ...
Oh and on the patch:
Acked-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
-Daniel
>
> > +}
> > +
> > static const struct drm_crtc_funcs tidss_crtc_funcs = {
> > .reset = tidss_crtc_reset,
> > - .destroy = drm_crtc_cleanup,
> > + .destroy = tidss_crtc_destroy,
> > .set_config = drm_atomic_helper_set_config,
> > .page_flip = drm_atomic_helper_page_flip,
> > .atomic_duplicate_state = tidss_crtc_duplicate_state,
> > @@ -400,7 +408,7 @@ struct tidss_crtc *tidss_crtc_create(struct tidss_device *tidss,
> > bool has_ctm = tidss->feat->vp_feat.color.has_ctm;
> > int ret;
> >
> > - tcrtc = devm_kzalloc(tidss->dev, sizeof(*tcrtc), GFP_KERNEL);
> > + tcrtc = kzalloc(sizeof(*tcrtc), GFP_KERNEL);
> > if (!tcrtc)
> > return ERR_PTR(-ENOMEM);
> >
> > @@ -411,8 +419,10 @@ struct tidss_crtc *tidss_crtc_create(struct tidss_device *tidss,
> >
> > ret = drm_crtc_init_with_planes(&tidss->ddev, crtc, primary,
> > NULL, &tidss_crtc_funcs, NULL);
> > - if (ret < 0)
> > + if (ret < 0) {
> > + kfree(tcrtc);
> > return ERR_PTR(ret);
> > + }
> >
> > drm_crtc_helper_add(crtc, &tidss_crtc_helper_funcs);
> >
> > diff --git a/drivers/gpu/drm/tidss/tidss_encoder.c b/drivers/gpu/drm/tidss/tidss_encoder.c
> > index 83785b0a66a9..30bf2a65949c 100644
> > --- a/drivers/gpu/drm/tidss/tidss_encoder.c
> > +++ b/drivers/gpu/drm/tidss/tidss_encoder.c
> > @@ -55,12 +55,18 @@ static int tidss_encoder_atomic_check(struct drm_encoder *encoder,
> > return 0;
> > }
> >
> > +static void tidss_encoder_destroy(struct drm_encoder *encoder)
> > +{
> > + drm_encoder_cleanup(encoder);
> > + kfree(encoder);
> > +}
> > +
> > static const struct drm_encoder_helper_funcs encoder_helper_funcs = {
> > .atomic_check = tidss_encoder_atomic_check,
> > };
> >
> > static const struct drm_encoder_funcs encoder_funcs = {
> > - .destroy = drm_encoder_cleanup,
> > + .destroy = tidss_encoder_destroy,
> > };
> >
> > struct drm_encoder *tidss_encoder_create(struct tidss_device *tidss,
> > @@ -69,7 +75,7 @@ struct drm_encoder *tidss_encoder_create(struct tidss_device *tidss,
> > struct drm_encoder *enc;
> > int ret;
> >
> > - enc = devm_kzalloc(tidss->dev, sizeof(*enc), GFP_KERNEL);
> > + enc = kzalloc(sizeof(*enc), GFP_KERNEL);
> > if (!enc)
> > return ERR_PTR(-ENOMEM);
> >
> > @@ -77,8 +83,10 @@ struct drm_encoder *tidss_encoder_create(struct tidss_device *tidss,
> >
> > ret = drm_encoder_init(&tidss->ddev, enc, &encoder_funcs,
> > encoder_type, NULL);
> > - if (ret < 0)
> > + if (ret < 0) {
> > + kfree(enc);
> > return ERR_PTR(ret);
> > + }
> >
> > drm_encoder_helper_add(enc, &encoder_helper_funcs);
> >
> > diff --git a/drivers/gpu/drm/tidss/tidss_plane.c b/drivers/gpu/drm/tidss/tidss_plane.c
> > index ff99b2dd4a17..798488948fc5 100644
> > --- a/drivers/gpu/drm/tidss/tidss_plane.c
> > +++ b/drivers/gpu/drm/tidss/tidss_plane.c
> > @@ -141,6 +141,14 @@ static void tidss_plane_atomic_disable(struct drm_plane *plane,
> > dispc_plane_enable(tidss->dispc, tplane->hw_plane_id, false);
> > }
> >
> > +static void drm_plane_destroy(struct drm_plane *plane)
> > +{
> > + struct tidss_plane *tplane = to_tidss_plane(plane);
> > +
> > + drm_plane_cleanup(plane);
> > + kfree(tplane);
> > +}
> > +
> > static const struct drm_plane_helper_funcs tidss_plane_helper_funcs = {
> > .atomic_check = tidss_plane_atomic_check,
> > .atomic_update = tidss_plane_atomic_update,
> > @@ -151,7 +159,7 @@ static const struct drm_plane_funcs tidss_plane_funcs = {
> > .update_plane = drm_atomic_helper_update_plane,
> > .disable_plane = drm_atomic_helper_disable_plane,
> > .reset = drm_atomic_helper_plane_reset,
> > - .destroy = drm_plane_cleanup,
> > + .destroy = drm_plane_destroy,
> > .atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
> > .atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
> > };
> > @@ -175,7 +183,7 @@ struct tidss_plane *tidss_plane_create(struct tidss_device *tidss,
> > BIT(DRM_MODE_BLEND_COVERAGE));
> > int ret;
> >
> > - tplane = devm_kzalloc(tidss->dev, sizeof(*tplane), GFP_KERNEL);
> > + tplane = kzalloc(sizeof(*tplane), GFP_KERNEL);
> > if (!tplane)
> > return ERR_PTR(-ENOMEM);
> >
> > @@ -190,7 +198,7 @@ struct tidss_plane *tidss_plane_create(struct tidss_device *tidss,
> > formats, num_formats,
> > NULL, type, NULL);
> > if (ret < 0)
> > - return ERR_PTR(ret);
> > + goto err;
> >
> > drm_plane_helper_add(&tplane->plane, &tidss_plane_helper_funcs);
> >
> > @@ -203,15 +211,19 @@ struct tidss_plane *tidss_plane_create(struct tidss_device *tidss,
> > default_encoding,
> > default_range);
> > if (ret)
> > - return ERR_PTR(ret);
> > + goto err;
> >
> > ret = drm_plane_create_alpha_property(&tplane->plane);
> > if (ret)
> > - return ERR_PTR(ret);
> > + goto err;
> >
> > ret = drm_plane_create_blend_mode_property(&tplane->plane, blend_modes);
> > if (ret)
> > - return ERR_PTR(ret);
> > + goto err;
> >
> > return tplane;
> > +
> > +err:
> > + kfree(tplane);
> > + return ERR_PTR(ret);
> > }
>
> --
> Regards,
>
> Laurent Pinchart
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
