Re: [PATCH v2] drm: vkms: check status of alloc_ordered_workqueue

[Daniel Vetter <daniel@ffwll.ch>]

On Sat, Mar 23, 2019 at 04:42:16PM -0500, Kangjie Lu wrote:
> alloc_ordered_workqueue may fail and return NULL.
> The fix cleans up drm plans and returns ENOMEM when it fails to
> avoid potential NULL pointer dereference.
> 
> Signed-off-by: Kangjie Lu <kjlu@umn.edu>
> ---
> V2: clean up resources
> ---
>  drivers/gpu/drm/vkms/vkms_crtc.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
> index 8a9aeb0a9ea8..018b52dd953a 100644
> --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> @@ -219,6 +219,17 @@ int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
>  	spin_lock_init(&vkms_out->state_lock);
>  
>  	vkms_out->crc_workq = alloc_ordered_workqueue("vkms_crc_workq", 0);
> +	if (!vkms_out->crc_workq) {
> +		ret = -ENOMEM;
> +		goto cleanup;
> +	}
> +
> +	return ret;
>  
> +cleanup:

Style nit for the future, for clarity I'd label this err:, since this path
is only taken for failures, and not to do cleanup for all cases.

Aside from that, I think your v1 was correct, vkms_crtc_init only sets up
the crtc, it doesn't allocate the cursor/planes. That's done from
vkms_output_init, which already has the cleanup code to handle this case.
-Daniel

> +	if (!IS_ERR_OR_NULL(cursor))
> +		drm_plane_cleanup(cursor);
> +	if (!IS_ERR(primary))
> +		drm_plane_cleanup(primary);
>  	return ret;
>  }
> -- 
> 2.17.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch