2019-03-09 04:37:30

by Kangjie Lu

Subject: [PATCH] drm: vkms: check status of alloc_ordered_workqueue

alloc_ordered_workqueue may fail and return NULL.
The fix returns ENOMEM when it fails to avoid potential NULL
pointer dereference.

Signed-off-by: Kangjie Lu <[email protected]>
---
drivers/gpu/drm/vkms/vkms_crtc.c | 2 ++
1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
index 8a9aeb0a9ea8..bb66dbcd5e3f 100644
--- a/drivers/gpu/drm/vkms/vkms_crtc.c
+++ b/drivers/gpu/drm/vkms/vkms_crtc.c
@@ -219,6 +219,8 @@ 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)
+ return -ENOMEM;

return ret;
}
--
2.17.1

2019-03-23 02:33:45

by Kangjie Lu

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

> On Mar 8, 2019, at 10:36 PM, Kangjie Lu <[email protected]> wrote:
>
> alloc_ordered_workqueue may fail and return NULL.
> The fix returns ENOMEM when it fails to avoid potential NULL
> pointer dereference.
>
> Signed-off-by: Kangjie Lu <[email protected]>
> ---
> drivers/gpu/drm/vkms/vkms_crtc.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
> index 8a9aeb0a9ea8..bb66dbcd5e3f 100644
> --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> @@ -219,6 +219,8 @@ 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)
> + return -ENOMEM;

Is this a reasonable patch?

>
> return ret;
> }
> --
> 2.17.1
>

2019-03-23 10:06:29

by Mukesh Ojha

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

On 3/9/2019 10:06 AM, Kangjie Lu wrote:
> alloc_ordered_workqueue may fail and return NULL.
> The fix returns ENOMEM when it fails to avoid potential NULL
> pointer dereference.
>
> Signed-off-by: Kangjie Lu <[email protected]>
> ---
> drivers/gpu/drm/vkms/vkms_crtc.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
> index 8a9aeb0a9ea8..bb66dbcd5e3f 100644
> --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> @@ -219,6 +219,8 @@ 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)
> + return -ENOMEM;
>
> return ret;
> }


Check the clean up path more carefully, you have undo which you have
done successfully in drm_crtc_init_with_planes.


Thanks,
 Mukesh

2019-03-23 21:44:50

by Kangjie Lu

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

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 <[email protected]>
---
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:
+ if (!IS_ERR_OR_NULL(cursor))
+ drm_plane_cleanup(cursor);
+ if (!IS_ERR(primary))
+ drm_plane_cleanup(primary);
return ret;
}
--
2.17.1

2019-03-25 08:33:42

by Daniel Vetter

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

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 <[email protected]>
> ---
> 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

2019-03-25 08:36:35

by Daniel Vetter

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

On Fri, Mar 22, 2019 at 09:32:07PM -0500, Kangjie Lu wrote:
>
>
> > On Mar 8, 2019, at 10:36 PM, Kangjie Lu <[email protected]> wrote:
> >
> > alloc_ordered_workqueue may fail and return NULL.
> > The fix returns ENOMEM when it fails to avoid potential NULL
> > pointer dereference.
> >
> > Signed-off-by: Kangjie Lu <[email protected]>
> > ---
> > drivers/gpu/drm/vkms/vkms_crtc.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
> > index 8a9aeb0a9ea8..bb66dbcd5e3f 100644
> > --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> > +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> > @@ -219,6 +219,8 @@ 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)
> > + return -ENOMEM;
>
> Is this a reasonable patch?

lgtm, applied and thanks for your patch.
-Daniel

>
> >
> > return ret;
> > }
> > --
> > 2.17.1
> >
>

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