| Message ID | 20170829073218.11097-2-peter.ujfalusi@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Peter, Thank you for the patch. On Tuesday, 29 August 2017 10:32:12 EEST Peter Ujfalusi wrote: > It makes the cleanup paths a bit cleaner. But it also goes against a proper implementation of object lifetime management :-( In DRM driver private structures are accessible from file operations, and must thus be reference-counted and only freed when the last userspace user goes away. This may be after the devm_* resources are released right after the .remove() operation is called. Using devm_kzalloc() would thus make it impossible to properly reference-count our data structures. > Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> > --- > drivers/gpu/drm/omapdrm/omap_drv.c | 19 +++++++------------ > 1 file changed, 7 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c > b/drivers/gpu/drm/omapdrm/omap_drv.c index 9b3c36b48356..903510d8054e > 100644 > --- a/drivers/gpu/drm/omapdrm/omap_drv.c > +++ b/drivers/gpu/drm/omapdrm/omap_drv.c > @@ -548,19 +548,17 @@ static int pdev_probe(struct platform_device *pdev) > return ret; > } > > + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL); > + if (!priv) > + return -ENOMEM; > + > omap_crtc_pre_init(); > > ret = omap_connect_dssdevs(); > if (ret) > goto err_crtc_uninit; > > - /* Allocate and initialize the driver private structure. */ > - priv = kzalloc(sizeof(*priv), GFP_KERNEL); > - if (!priv) { > - ret = -ENOMEM; > - goto err_disconnect_dssdevs; > - } > - > + /* Initialize the driver private structure. */ > priv->dispc_ops = dispc_get_ops(); > > soc = soc_device_match(omapdrm_soc_devices); > @@ -574,7 +572,7 @@ static int pdev_probe(struct platform_device *pdev) > ddev = drm_dev_alloc(&omap_drm_driver, &pdev->dev); > if (IS_ERR(ddev)) { > ret = PTR_ERR(ddev); > - goto err_free_priv; > + goto err_destroy_wq; > } > > ddev->dev_private = priv; > @@ -624,10 +622,8 @@ static int pdev_probe(struct platform_device *pdev) > err_free_drm_dev: > omap_gem_deinit(ddev); > drm_dev_unref(ddev); > -err_free_priv: > +err_destroy_wq: > destroy_workqueue(priv->wq); > - kfree(priv); > -err_disconnect_dssdevs: > omap_disconnect_dssdevs(); > err_crtc_uninit: > omap_crtc_pre_uninit(); > @@ -659,7 +655,6 @@ static int pdev_remove(struct platform_device *pdev) > drm_dev_unref(ddev); > > destroy_workqueue(priv->wq); > - kfree(priv); > > omap_disconnect_dssdevs(); > omap_crtc_pre_uninit();
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki On 2017-09-01 14:10, Laurent Pinchart wrote: > Hi Peter, > > Thank you for the patch. > > On Tuesday, 29 August 2017 10:32:12 EEST Peter Ujfalusi wrote: >> It makes the cleanup paths a bit cleaner. > > But it also goes against a proper implementation of object lifetime management > :-( In DRM driver private structures are accessible from file operations, and > must thus be reference-counted and only freed when the last userspace user > goes away. This may be after the devm_* resources are released right after the > .remove() operation is called. Using devm_kzalloc() would thus make it > impossible to properly reference-count our data structures. Hrm, not sure if I follow you on this. Before this patch the 'priv' was freed up in pdev_remove(). With this patch the 'priv' is going to be freed up after the pdev_remove() has returned, so the 'priv' would be available longer than before as we converted it as managed resource. If what you are saying is true, then we already have the same issue and I don't see how this could have been working or need to work. If you remove the driver and you need to keep the private data available after the driver has been unloaded, who is going to free that up? > >> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> >> --- >> drivers/gpu/drm/omapdrm/omap_drv.c | 19 +++++++------------ >> 1 file changed, 7 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c >> b/drivers/gpu/drm/omapdrm/omap_drv.c index 9b3c36b48356..903510d8054e >> 100644 >> --- a/drivers/gpu/drm/omapdrm/omap_drv.c >> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c >> @@ -548,19 +548,17 @@ static int pdev_probe(struct platform_device *pdev) >> return ret; >> } >> >> + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL); >> + if (!priv) >> + return -ENOMEM; >> + >> omap_crtc_pre_init(); >> >> ret = omap_connect_dssdevs(); >> if (ret) >> goto err_crtc_uninit; >> >> - /* Allocate and initialize the driver private structure. */ >> - priv = kzalloc(sizeof(*priv), GFP_KERNEL); >> - if (!priv) { >> - ret = -ENOMEM; >> - goto err_disconnect_dssdevs; >> - } >> - >> + /* Initialize the driver private structure. */ >> priv->dispc_ops = dispc_get_ops(); >> >> soc = soc_device_match(omapdrm_soc_devices); >> @@ -574,7 +572,7 @@ static int pdev_probe(struct platform_device *pdev) >> ddev = drm_dev_alloc(&omap_drm_driver, &pdev->dev); >> if (IS_ERR(ddev)) { >> ret = PTR_ERR(ddev); >> - goto err_free_priv; >> + goto err_destroy_wq; >> } >> >> ddev->dev_private = priv; >> @@ -624,10 +622,8 @@ static int pdev_probe(struct platform_device *pdev) >> err_free_drm_dev: >> omap_gem_deinit(ddev); >> drm_dev_unref(ddev); >> -err_free_priv: >> +err_destroy_wq: >> destroy_workqueue(priv->wq); >> - kfree(priv); >> -err_disconnect_dssdevs: >> omap_disconnect_dssdevs(); >> err_crtc_uninit: >> omap_crtc_pre_uninit(); >> @@ -659,7 +655,6 @@ static int pdev_remove(struct platform_device *pdev) >> drm_dev_unref(ddev); >> >> destroy_workqueue(priv->wq); >> - kfree(priv); >> >> omap_disconnect_dssdevs(); >> omap_crtc_pre_uninit(); > > - Péter
Hi Peter, On Monday, 4 September 2017 12:13:40 EEST Peter Ujfalusi wrote: > On 2017-09-01 14:10, Laurent Pinchart wrote: > > On Tuesday, 29 August 2017 10:32:12 EEST Peter Ujfalusi wrote: > >> It makes the cleanup paths a bit cleaner. > > > > But it also goes against a proper implementation of object lifetime > > management :-( In DRM driver private structures are accessible from file > > operations, and must thus be reference-counted and only freed when the > > last userspace user goes away. This may be after the devm_* resources are > > released right after the .remove() operation is called. Using > > devm_kzalloc() would thus make it impossible to properly reference-count > > our data structures. > > Hrm, not sure if I follow you on this. > Before this patch the 'priv' was freed up in pdev_remove(). With this > patch the 'priv' is going to be freed up after the pdev_remove() has > returned, so the 'priv' would be available longer than before as we > converted it as managed resource. > > If what you are saying is true, then we already have the same issue and > I don't see how this could have been working or need to work. If you > remove the driver and you need to keep the private data available after > the driver has been unloaded, who is going to free that up? At the moment the memory is freed at .remove() time, which can lead to memory corruption if a user has a handle on the device (for instance an open file handle that is then close()d). Fixing this requires moving memory free to the drm_driver::release() handler. devm_kzalloc() goes in the wrong direction. > >> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> > >> --- > >> > >> drivers/gpu/drm/omapdrm/omap_drv.c | 19 +++++++------------ > >> 1 file changed, 7 insertions(+), 12 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c > >> b/drivers/gpu/drm/omapdrm/omap_drv.c index 9b3c36b48356..903510d8054e > >> 100644 > >> --- a/drivers/gpu/drm/omapdrm/omap_drv.c > >> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c > >> @@ -548,19 +548,17 @@ static int pdev_probe(struct platform_device *pdev) > >> > >> return ret; > >> > >> } > >> > >> + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL); > >> + if (!priv) > >> + return -ENOMEM; > >> + > >> > >> omap_crtc_pre_init(); > >> > >> ret = omap_connect_dssdevs(); > >> if (ret) > >> > >> goto err_crtc_uninit; > >> > >> - /* Allocate and initialize the driver private structure. */ > >> - priv = kzalloc(sizeof(*priv), GFP_KERNEL); > >> - if (!priv) { > >> - ret = -ENOMEM; > >> - goto err_disconnect_dssdevs; > >> - } > >> - > >> + /* Initialize the driver private structure. */ > >> > >> priv->dispc_ops = dispc_get_ops(); > >> > >> soc = soc_device_match(omapdrm_soc_devices); > >> > >> @@ -574,7 +572,7 @@ static int pdev_probe(struct platform_device *pdev) > >> > >> ddev = drm_dev_alloc(&omap_drm_driver, &pdev->dev); > >> if (IS_ERR(ddev)) { > >> > >> ret = PTR_ERR(ddev); > >> > >> - goto err_free_priv; > >> + goto err_destroy_wq; > >> > >> } > >> > >> ddev->dev_private = priv; > >> > >> @@ -624,10 +622,8 @@ static int pdev_probe(struct platform_device *pdev) > >> > >> err_free_drm_dev: > >> omap_gem_deinit(ddev); > >> drm_dev_unref(ddev); > >> > >> -err_free_priv: > >> > >> +err_destroy_wq: > >> destroy_workqueue(priv->wq); > >> > >> - kfree(priv); > >> > >> -err_disconnect_dssdevs: > >> omap_disconnect_dssdevs(); > >> > >> err_crtc_uninit: > >> omap_crtc_pre_uninit(); > >> > >> @@ -659,7 +655,6 @@ static int pdev_remove(struct platform_device *pdev) > >> > >> drm_dev_unref(ddev); > >> > >> destroy_workqueue(priv->wq); > >> > >> - kfree(priv); > >> > >> omap_disconnect_dssdevs(); > >> omap_crtc_pre_uninit();
Hi Laurent, Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki On 2017-09-04 12:41, Laurent Pinchart wrote: > At the moment the memory is freed at .remove() time, which can lead to memory > corruption if a user has a handle on the device (for instance an open file > handle that is then close()d). Fixing this requires moving memory free to the > drm_driver::release() handler. devm_kzalloc() goes in the wrong direction. Ah, OK, so the current way is buggy as well. How do you plan to fix that? I think this should work: struct omap_drm_private { /* First member in the private struct! */ + struct drm_device ddev; ... }; Use drm_dev_init(&priv->ddev, ...); to initialize the drm_device instead of drm_dev_alloc() then priv->ddev.dev_private = priv; in this case the drm_dev_unref() would free up our omap_drm_private, right? I think this is what other DRM drivers are doing, not all, but i915 does this at least. But by the description most of the DRM drivers are doing this wrong, right? - Péter
Hi Peter, On Monday, 4 September 2017 14:16:32 EEST Peter Ujfalusi wrote: > On 2017-09-04 12:41, Laurent Pinchart wrote: > > At the moment the memory is freed at .remove() time, which can lead to > > memory corruption if a user has a handle on the device (for instance an > > open file handle that is then close()d). Fixing this requires moving > > memory free to the drm_driver::release() handler. devm_kzalloc() goes in > > the wrong direction. > Ah, OK, so the current way is buggy as well. > > How do you plan to fix that? > I think this should work: > > struct omap_drm_private { > /* First member in the private struct! */ > + struct drm_device ddev; > ... > }; > > Use drm_dev_init(&priv->ddev, ...); to initialize the drm_device instead > of drm_dev_alloc() > > then priv->ddev.dev_private = priv; > > in this case the drm_dev_unref() would free up our omap_drm_private, right? That's the idea, yes. I got a local patch for that in my tree. > I think this is what other DRM drivers are doing, not all, but i915 does > this at least. > > But by the description most of the DRM drivers are doing this wrong, right? Correct, most drivers get it wrong. We'll have to fix it, but given that we have race conditions in the core that prevent proper hot-unplug support at the moment, I didn't want to start pushing for fixing drivers. Once we get the core sorted out, it will be time to address the other side of the issue.
Hi Laurent, Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki On 2017-09-04 17:19, Laurent Pinchart wrote: >> Ah, OK, so the current way is buggy as well. >> >> How do you plan to fix that? >> I think this should work: >> >> struct omap_drm_private { >> /* First member in the private struct! */ >> + struct drm_device ddev; >> ... >> }; >> >> Use drm_dev_init(&priv->ddev, ...); to initialize the drm_device instead >> of drm_dev_alloc() >> >> then priv->ddev.dev_private = priv; >> >> in this case the drm_dev_unref() would free up our omap_drm_private, right? > > That's the idea, yes. I got a local patch for that in my tree. Hrm, so what is the difference between what I do in this patch and the described fix? To be precise what is the difference between: struct drm_device *ddev = platform_get_drvdata(pdev); struct omap_drm_private *priv = ddev->dev_private; ... drm_dev_unref(ddev); ... devm would free priv compared to struct omap_drm_private { /* First member in the private struct! */ struct drm_device ddev; .... }; struct drm_device *ddev = platform_get_drvdata(pdev); struct omap_drm_private *priv = ddev->dev_private; ... /* Here we would free priv since &priv->ddev == ddev */ drm_dev_unref(ddev); The drm_dev_unregister() is provided as a protection for the issue you are describing, the comment suggest that at least: * This should be called first in the device teardown code to make sure * userspace can't access the device instance any more. and we do call it. As the first step. > >> I think this is what other DRM drivers are doing, not all, but i915 does >> this at least. >> >> But by the description most of the DRM drivers are doing this wrong, right? > > Correct, most drivers get it wrong. We'll have to fix it, but given that we > have race conditions in the core that prevent proper hot-unplug support at the > moment, I didn't want to start pushing for fixing drivers. Once we get the > core sorted out, it will be time to address the other side of the issue. > - Péter
diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c index 9b3c36b48356..903510d8054e 100644 --- a/drivers/gpu/drm/omapdrm/omap_drv.c +++ b/drivers/gpu/drm/omapdrm/omap_drv.c @@ -548,19 +548,17 @@ static int pdev_probe(struct platform_device *pdev) return ret; } + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL); + if (!priv) + return -ENOMEM; + omap_crtc_pre_init(); ret = omap_connect_dssdevs(); if (ret) goto err_crtc_uninit; - /* Allocate and initialize the driver private structure. */ - priv = kzalloc(sizeof(*priv), GFP_KERNEL); - if (!priv) { - ret = -ENOMEM; - goto err_disconnect_dssdevs; - } - + /* Initialize the driver private structure. */ priv->dispc_ops = dispc_get_ops(); soc = soc_device_match(omapdrm_soc_devices); @@ -574,7 +572,7 @@ static int pdev_probe(struct platform_device *pdev) ddev = drm_dev_alloc(&omap_drm_driver, &pdev->dev); if (IS_ERR(ddev)) { ret = PTR_ERR(ddev); - goto err_free_priv; + goto err_destroy_wq; } ddev->dev_private = priv; @@ -624,10 +622,8 @@ static int pdev_probe(struct platform_device *pdev) err_free_drm_dev: omap_gem_deinit(ddev); drm_dev_unref(ddev); -err_free_priv: +err_destroy_wq: destroy_workqueue(priv->wq); - kfree(priv); -err_disconnect_dssdevs: omap_disconnect_dssdevs(); err_crtc_uninit: omap_crtc_pre_uninit(); @@ -659,7 +655,6 @@ static int pdev_remove(struct platform_device *pdev) drm_dev_unref(ddev); destroy_workqueue(priv->wq); - kfree(priv); omap_disconnect_dssdevs(); omap_crtc_pre_uninit();
It makes the cleanup paths a bit cleaner. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> --- drivers/gpu/drm/omapdrm/omap_drv.c | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-)