Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 25 Mar 2019 09:31:19 +0100
From:   Daniel Vetter <daniel@ffwll.ch>
To:     Kangjie Lu <kjlu@umn.edu>
Cc:     pakki001@umn.edu, Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>,
        Haneen Mohammed <hamohammed.sa@gmail.com>,
        Daniel Vetter <daniel@ffwll.ch>,
        David Airlie <airlied@linux.ie>,
        dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] drm: vkms: check status of alloc_ordered_workqueue
Message-ID: <20190325083119.GB2665@phenom.ffwll.local>
Mail-Followup-To: Kangjie Lu <kjlu@umn.edu>, pakki001@umn.edu,
        Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>,
        Haneen Mohammed <hamohammed.sa@gmail.com>,
        David Airlie <airlied@linux.ie>, dri-devel@lists.freedesktop.org,
        linux-kernel@vger.kernel.org
References: <955fbabe-e5f2-cfff-8dfd-1730243b0fff@codeaurora.org>
 <20190323214216.10824-1-kjlu@umn.edu>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20190323214216.10824-1-kjlu@umn.edu>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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