| Message ID | 20210120123535.40226-3-paul@crapouillou.net |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
On Wed, Jan 20, 2021 at 1:36 PM Paul Cercueil <paul@crapouillou.net> wrote: > > Since the encoders have been devm-allocated, they will be freed way > before drm_mode_config_cleanup() is called. To avoid use-after-free > conditions, we then must ensure that drm_encoder_cleanup() is called > before the encoders are freed. > > v2: Use the new __drmm_simple_encoder_alloc() function > > Fixes: c369cb27c267 ("drm/ingenic: Support multiple panels/bridges") > Cc: <stable@vger.kernel.org> # 5.8+ > Signed-off-by: Paul Cercueil <paul@crapouillou.net> > --- > > Notes: > Use the V1 of this patch to fix v5.11 and older kernels. This V2 only > applies on the current drm-misc-next branch. > > drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 16 +++++++--------- > 1 file changed, 7 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c > index 7bb31fbee29d..158433b4c084 100644 > --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c > +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c > @@ -1014,20 +1014,18 @@ static int ingenic_drm_bind(struct device *dev, bool has_components) > bridge = devm_drm_panel_bridge_add_typed(dev, panel, > DRM_MODE_CONNECTOR_DPI); > > - encoder = devm_kzalloc(dev, sizeof(*encoder), GFP_KERNEL); > - if (!encoder) > - return -ENOMEM; > + encoder = __drmm_simple_encoder_alloc(drm, sizeof(*encoder), 0, Please don't use the __ prefixed functions, those are the internal ones. The official one comes with type checking and all that included. Otherwise lgtm. -Daniel > + DRM_MODE_ENCODER_DPI); > + if (IS_ERR(encoder)) { > + ret = PTR_ERR(encoder); > + dev_err(dev, "Failed to init encoder: %d\n", ret); > + return ret; > + } > > encoder->possible_crtcs = 1; > > drm_encoder_helper_add(encoder, &ingenic_drm_encoder_helper_funcs); > > - ret = drm_simple_encoder_init(drm, encoder, DRM_MODE_ENCODER_DPI); > - if (ret) { > - dev_err(dev, "Failed to init encoder: %d\n", ret); > - return ret; > - } > - > ret = drm_bridge_attach(encoder, bridge, NULL, 0); > if (ret) { > dev_err(dev, "Unable to attach bridge\n"); > -- > 2.29.2 >
On Wed, Jan 20, 2021 at 2:21 PM Paul Cercueil <paul@crapouillou.net> wrote: > > > > Le mer. 20 janv. 2021 à 14:01, Daniel Vetter <daniel@ffwll.ch> a > écrit : > > On Wed, Jan 20, 2021 at 1:36 PM Paul Cercueil <paul@crapouillou.net> > > wrote: > >> > >> Since the encoders have been devm-allocated, they will be freed way > >> before drm_mode_config_cleanup() is called. To avoid use-after-free > >> conditions, we then must ensure that drm_encoder_cleanup() is called > >> before the encoders are freed. > >> > >> v2: Use the new __drmm_simple_encoder_alloc() function > >> > >> Fixes: c369cb27c267 ("drm/ingenic: Support multiple panels/bridges") > >> Cc: <stable@vger.kernel.org> # 5.8+ > >> Signed-off-by: Paul Cercueil <paul@crapouillou.net> > >> --- > >> > >> Notes: > >> Use the V1 of this patch to fix v5.11 and older kernels. This > >> V2 only > >> applies on the current drm-misc-next branch. > >> > >> drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 16 +++++++--------- > >> 1 file changed, 7 insertions(+), 9 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c > >> b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c > >> index 7bb31fbee29d..158433b4c084 100644 > >> --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c > >> +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c > >> @@ -1014,20 +1014,18 @@ static int ingenic_drm_bind(struct device > >> *dev, bool has_components) > >> bridge = > >> devm_drm_panel_bridge_add_typed(dev, panel, > >> > >> DRM_MODE_CONNECTOR_DPI); > >> > >> - encoder = devm_kzalloc(dev, sizeof(*encoder), > >> GFP_KERNEL); > >> - if (!encoder) > >> - return -ENOMEM; > >> + encoder = __drmm_simple_encoder_alloc(drm, > >> sizeof(*encoder), 0, > > > > Please don't use the __ prefixed functions, those are the internal > > ones. The official one comes with type checking and all that included. > > Otherwise lgtm. > > -Daniel > > The non-prefixed one assumes that I want to allocate a struct that > contains the encoder, not just the drm_encoder itself. Hm, but using the internal one is also a bit too ugly. A drm_plain_simple_enocder_alloc(drm, type) wrapper would be the right thing here I think? Setting the offsets and struct sizes directly in these in drivers really doesn't feel like a good idea. I think simple encoder is the only case where we really have a need for a non-embeddable struct. -Daniel > > -Paul > > >> + > >> DRM_MODE_ENCODER_DPI); > >> + if (IS_ERR(encoder)) { > >> + ret = PTR_ERR(encoder); > >> + dev_err(dev, "Failed to init encoder: > >> %d\n", ret); > >> + return ret; > >> + } > >> > >> encoder->possible_crtcs = 1; > >> > >> drm_encoder_helper_add(encoder, > >> &ingenic_drm_encoder_helper_funcs); > >> > >> - ret = drm_simple_encoder_init(drm, encoder, > >> DRM_MODE_ENCODER_DPI); > >> - if (ret) { > >> - dev_err(dev, "Failed to init encoder: > >> %d\n", ret); > >> - return ret; > >> - } > >> - > >> ret = drm_bridge_attach(encoder, bridge, NULL, 0); > >> if (ret) { > >> dev_err(dev, "Unable to attach bridge\n"); > >> -- > >> 2.29.2 > >> > > > > > > -- > > Daniel Vetter > > Software Engineer, Intel Corporation > > http://blog.ffwll.ch > >
Le mer. 20 janv. 2021 à 15:04, Daniel Vetter <daniel@ffwll.ch> a écrit : > On Wed, Jan 20, 2021 at 2:21 PM Paul Cercueil <paul@crapouillou.net> > wrote: >> >> >> >> Le mer. 20 janv. 2021 à 14:01, Daniel Vetter <daniel@ffwll.ch> a >> écrit : >> > On Wed, Jan 20, 2021 at 1:36 PM Paul Cercueil >> <paul@crapouillou.net> >> > wrote: >> >> >> >> Since the encoders have been devm-allocated, they will be freed >> way >> >> before drm_mode_config_cleanup() is called. To avoid >> use-after-free >> >> conditions, we then must ensure that drm_encoder_cleanup() is >> called >> >> before the encoders are freed. >> >> >> >> v2: Use the new __drmm_simple_encoder_alloc() function >> >> >> >> Fixes: c369cb27c267 ("drm/ingenic: Support multiple >> panels/bridges") >> >> Cc: <stable@vger.kernel.org> # 5.8+ >> >> Signed-off-by: Paul Cercueil <paul@crapouillou.net> >> >> --- >> >> >> >> Notes: >> >> Use the V1 of this patch to fix v5.11 and older kernels. >> This >> >> V2 only >> >> applies on the current drm-misc-next branch. >> >> >> >> drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 16 +++++++--------- >> >> 1 file changed, 7 insertions(+), 9 deletions(-) >> >> >> >> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c >> >> b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c >> >> index 7bb31fbee29d..158433b4c084 100644 >> >> --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c >> >> +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c >> >> @@ -1014,20 +1014,18 @@ static int ingenic_drm_bind(struct >> device >> >> *dev, bool has_components) >> >> bridge = >> >> devm_drm_panel_bridge_add_typed(dev, panel, >> >> >> >> DRM_MODE_CONNECTOR_DPI); >> >> >> >> - encoder = devm_kzalloc(dev, sizeof(*encoder), >> >> GFP_KERNEL); >> >> - if (!encoder) >> >> - return -ENOMEM; >> >> + encoder = __drmm_simple_encoder_alloc(drm, >> >> sizeof(*encoder), 0, >> > >> > Please don't use the __ prefixed functions, those are the internal >> > ones. The official one comes with type checking and all that >> included. >> > Otherwise lgtm. >> > -Daniel >> >> The non-prefixed one assumes that I want to allocate a struct that >> contains the encoder, not just the drm_encoder itself. > > Hm, but using the internal one is also a bit too ugly. A > drm_plain_simple_enocder_alloc(drm, type) wrapper would be the right > thing here I think? Setting the offsets and struct sizes directly in > these in drivers really doesn't feel like a good idea. I think simple > encoder is the only case where we really have a need for a > non-embeddable struct. > -Daniel Alright, I will add a wrapper. Cheers, -Paul >> >> >> + >> >> DRM_MODE_ENCODER_DPI); >> >> + if (IS_ERR(encoder)) { >> >> + ret = PTR_ERR(encoder); >> >> + dev_err(dev, "Failed to init encoder: >> >> %d\n", ret); >> >> + return ret; >> >> + } >> >> >> >> encoder->possible_crtcs = 1; >> >> >> >> drm_encoder_helper_add(encoder, >> >> &ingenic_drm_encoder_helper_funcs); >> >> >> >> - ret = drm_simple_encoder_init(drm, encoder, >> >> DRM_MODE_ENCODER_DPI); >> >> - if (ret) { >> >> - dev_err(dev, "Failed to init encoder: >> >> %d\n", ret); >> >> - return ret; >> >> - } >> >> - >> >> ret = drm_bridge_attach(encoder, bridge, NULL, >> 0); >> >> if (ret) { >> >> dev_err(dev, "Unable to attach >> bridge\n"); >> >> -- >> >> 2.29.2 >> >> >> > >> > >> > -- >> > Daniel Vetter >> > Software Engineer, Intel Corporation >> > http://blog.ffwll.ch >> >> > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c index 7bb31fbee29d..158433b4c084 100644 --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c @@ -1014,20 +1014,18 @@ static int ingenic_drm_bind(struct device *dev, bool has_components) bridge = devm_drm_panel_bridge_add_typed(dev, panel, DRM_MODE_CONNECTOR_DPI); - encoder = devm_kzalloc(dev, sizeof(*encoder), GFP_KERNEL); - if (!encoder) - return -ENOMEM; + encoder = __drmm_simple_encoder_alloc(drm, sizeof(*encoder), 0, + DRM_MODE_ENCODER_DPI); + if (IS_ERR(encoder)) { + ret = PTR_ERR(encoder); + dev_err(dev, "Failed to init encoder: %d\n", ret); + return ret; + } encoder->possible_crtcs = 1; drm_encoder_helper_add(encoder, &ingenic_drm_encoder_helper_funcs); - ret = drm_simple_encoder_init(drm, encoder, DRM_MODE_ENCODER_DPI); - if (ret) { - dev_err(dev, "Failed to init encoder: %d\n", ret); - return ret; - } - ret = drm_bridge_attach(encoder, bridge, NULL, 0); if (ret) { dev_err(dev, "Unable to attach bridge\n");
Since the encoders have been devm-allocated, they will be freed way before drm_mode_config_cleanup() is called. To avoid use-after-free conditions, we then must ensure that drm_encoder_cleanup() is called before the encoders are freed. v2: Use the new __drmm_simple_encoder_alloc() function Fixes: c369cb27c267 ("drm/ingenic: Support multiple panels/bridges") Cc: <stable@vger.kernel.org> # 5.8+ Signed-off-by: Paul Cercueil <paul@crapouillou.net> --- Notes: Use the V1 of this patch to fix v5.11 and older kernels. This V2 only applies on the current drm-misc-next branch. drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-)