On Thu, Apr 23, 2015 at 03:07:56PM +0100, Peter Antoine wrote:
> If an application that has a driver lock created, wants the lock the
> kernel context, it is not allowed to. If the call to drm_lock has a
> context of 0, it is rejected. If you set the context to _DRM_LOCK_CONT
> then call drm lock, it will pass the context == DRM_KERNEL_CONTEXT checks.
> But as the DRM_LOCK_CONT bits are not part of the context id this allows
> operations on the DRM_KERNEL_CONTEXT.
>
> Issue: VIZ-5485
> Signed-off-by: Peter Antoine <peter.antoine@xxxxxxxxx>
> ---
> drivers/gpu/drm/drm_context.c | 6 +++---
> drivers/gpu/drm/drm_lock.c | 4 ++--
> 2 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_context.c b/drivers/gpu/drm/drm_context.c
> index 96350d1..1febcd3 100644
> --- a/drivers/gpu/drm/drm_context.c
> +++ b/drivers/gpu/drm/drm_context.c
> @@ -123,7 +123,7 @@ void drm_legacy_ctxbitmap_flush(struct drm_device *dev, struct drm_file *file)
>
> list_for_each_entry_safe(pos, tmp, &dev->ctxlist, head) {
> if (pos->tag == file &&
> - pos->handle != DRM_KERNEL_CONTEXT) {
> + _DRM_LOCKING_CONTEXT(pos->handle) != DRM_KERNEL_CONTEXT) {
> if (dev->driver->context_dtor)
> dev->driver->context_dtor(dev, pos->handle);
>
> @@ -342,7 +342,7 @@ int drm_legacy_addctx(struct drm_device *dev, void *data,
> struct drm_ctx *ctx = data;
>
> ctx->handle = drm_legacy_ctxbitmap_next(dev);
> - if (ctx->handle == DRM_KERNEL_CONTEXT) {
> + if (_DRM_LOCKING_CONTEXT(ctx->handle) == DRM_KERNEL_CONTEXT) {
> /* Skip kernel's context and get a new one. */
> ctx->handle = drm_legacy_ctxbitmap_next(dev);
> }
> @@ -449,7 +449,7 @@ int drm_legacy_rmctx(struct drm_device *dev, void *data,
> struct drm_ctx *ctx = data;
>
> DRM_DEBUG("%d\n", ctx->handle);
> - if (ctx->handle != DRM_KERNEL_CONTEXT) {
> + if (_DRM_LOCKING_CONTEXT(ctx->handle) != DRM_KERNEL_CONTEXT) {
> if (dev->driver->context_dtor)
> dev->driver->context_dtor(dev, ctx->handle);
> drm_legacy_ctxbitmap_free(dev, ctx->handle);
How about just fixing the end parameter passed to idr_alloc()? AFAICS
that would take care of the context code.
Well, there are a few more issues with the code:
- not properly checking for negative return value from idr_alloc()
- leaking the ctx id on kmalloc() error
- pointless check for idr_alloc() returning 0 even though the min is 1
> diff --git a/drivers/gpu/drm/drm_lock.c b/drivers/gpu/drm/drm_lock.c
> index 070dd5d..94500930 100644
> --- a/drivers/gpu/drm/drm_lock.c
> +++ b/drivers/gpu/drm/drm_lock.c
> @@ -63,7 +63,7 @@ int drm_legacy_lock(struct drm_device *dev, void *data,
>
> ++file_priv->lock_count;
While you're poking around this dungeopn, maybe you can kill lock_count?
We never seem to decrement it, and it's only checked in drm_legacy_i_have_hw_lock().
>
> - if (lock->context == DRM_KERNEL_CONTEXT) {
> + if (_DRM_LOCKING_CONTEXT(lock->context) == DRM_KERNEL_CONTEXT) {
> DRM_ERROR("Process %d using kernel context %d\n",
> task_pid_nr(current), lock->context);
> return -EINVAL;
> @@ -153,7 +153,7 @@ int drm_legacy_unlock(struct drm_device *dev, void *data, struct drm_file *file_
> struct drm_lock *lock = data;
> struct drm_master *master = file_priv->master;
>
> - if (lock->context == DRM_KERNEL_CONTEXT) {
> + if (_DRM_LOCKING_CONTEXT(lock->context) == DRM_KERNEL_CONTEXT) {
> DRM_ERROR("Process %d using kernel context %d\n",
> task_pid_nr(current), lock->context);
> return -EINVAL;
These two changes look OK to me.
> --
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
