| Message ID | 20200415092006.26675-1-tomi.valkeinen@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/tidss: fix crash related to accessing freed memory | expand |
(Adding Jyri, forgot to add him) On 15/04/2020 12:20, 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@ti.com> > --- > 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); > +} > + > 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); > } >
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@ti.com> > --- > 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. > +} > + > 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); > }
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@ti.com> > > --- > > 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@ffwll.ch> -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
Hi Daniel, On Wed, Apr 15, 2020 at 02:52:43PM +0200, Daniel Vetter wrote: > On Wed, Apr 15, 2020 at 03:45:50PM +0300, Laurent Pinchart wrote: > > 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@ti.com> > >> --- > >> 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 ... I think embedding is the best option when you have a fixed number of CRTCs, encoders and planes. If they're variable but reasonably bounded, embedding will waste a bit of memory, but massively simplify the code. Even with the helpers you're working on, it will save memory as devres allocates memory to track objects, and will certainly save CPU time too, so it could be a net gain in the end. That's the method I recommend if it can be used, but it may be a matter of taste. > Oh and on the patch: > > Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > >> +} > >> + > >> 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); > >> }
Hi, On 15/04/2020 15:45, Laurent Pinchart wrote: >> +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. This seemed the easiest way to fix this for 5.7-rcs, without doing too many changes all around that might cause conflicts. The allocs and frees are close to each other, in the same files, although there's repetition of course. Tomi
On Wed, Apr 15, 2020 at 04:03:44PM +0300, Laurent Pinchart wrote: > Hi Daniel, > > On Wed, Apr 15, 2020 at 02:52:43PM +0200, Daniel Vetter wrote: > > On Wed, Apr 15, 2020 at 03:45:50PM +0300, Laurent Pinchart wrote: > > > 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@ti.com> > > >> --- > > >> 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 ... > > I think embedding is the best option when you have a fixed number of > CRTCs, encoders and planes. If they're variable but reasonably bounded, > embedding will waste a bit of memory, but massively simplify the code. > Even with the helpers you're working on, it will save memory as devres > allocates memory to track objects, and will certainly save CPU time too, > so it could be a net gain in the end. That's the method I recommend if > it can be used, but it may be a matter of taste. For small drivers it doesn't really matter, but my experience with big drivers is that if you smash everything into one place, you end up with spaghetti code. In i915 we're working really hard to pull stuff out of the massive i915 driver structure, simply to force better organization of the code. Separate allocations force a bit more thought about encapsulating objects and putting code to the right place at least. That's kinda why I prefer it. At the end it's a tradeoff, so *shrug* -Daniel > > > Oh and on the patch: > > > > Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > > > >> +} > > >> + > > >> 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
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); +} + 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); }
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@ti.com> --- 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(-)