[RFC 1/7] drm/omap: Use devm_kzalloc() to allocate omap_drm_private

[Laurent Pinchart]

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 at 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();

-- 
Regards,

Laurent Pinchart