From 637bd2a4d1f904cedaf5d88f04e92c3234825343 Mon Sep 17 00:00:00 2001 From: Sunil Khatri Date: Wed, 2 Aug 2017 19:15:26 +0530 Subject: [PATCH] msm: kgsl: Protect the event->handle with spinlock event->handle pointer can be used after free due to the race condition between kgsl_sync_callback and kgsl_sync_fence_async_cancel. Protect the event->handle with a spinlock to avoid concurrent access issues. Change-Id: I3719e401af9ece82ac68b72f2aef784c7fdc1104 Signed-off-by: Sunil Khatri --- drivers/gpu/msm/adreno_debugfs.c | 6 +++++ drivers/gpu/msm/kgsl_drawobj.c | 46 +++++++++++++++++++++++++++++--- drivers/gpu/msm/kgsl_drawobj.h | 4 ++- 3 files changed, 51 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/msm/adreno_debugfs.c b/drivers/gpu/msm/adreno_debugfs.c index 5306303b8d15..2027ac66f737 100644 --- a/drivers/gpu/msm/adreno_debugfs.c +++ b/drivers/gpu/msm/adreno_debugfs.c @@ -131,6 +131,8 @@ typedef void (*reg_read_fill_t)(struct kgsl_device *device, int i, static void sync_event_print(struct seq_file *s, struct kgsl_drawobj_sync_event *sync_event) { + unsigned long flags; + switch (sync_event->type) { case KGSL_CMD_SYNCPOINT_TYPE_TIMESTAMP: { seq_printf(s, "sync: ctx: %d ts: %d", @@ -138,9 +140,13 @@ static void sync_event_print(struct seq_file *s, break; } case KGSL_CMD_SYNCPOINT_TYPE_FENCE: + spin_lock_irqsave(&sync_event->handle_lock, flags); + seq_printf(s, "sync: [%pK] %s", sync_event->handle, (sync_event->handle && sync_event->handle->fence) ? sync_event->handle->fence->name : "NULL"); + + spin_unlock_irqrestore(&sync_event->handle_lock, flags); break; default: seq_printf(s, "sync: type: %d", sync_event->type); diff --git a/drivers/gpu/msm/kgsl_drawobj.c b/drivers/gpu/msm/kgsl_drawobj.c index 8dc2ebd26cf9..fba18231cb72 100644 --- a/drivers/gpu/msm/kgsl_drawobj.c +++ b/drivers/gpu/msm/kgsl_drawobj.c @@ -79,6 +79,7 @@ void kgsl_dump_syncpoints(struct kgsl_device *device, { struct kgsl_drawobj_sync_event *event; unsigned int i; + unsigned long flags; for (i = 0; i < syncobj->numsyncs; i++) { event = &syncobj->synclist[i]; @@ -101,12 +102,16 @@ void kgsl_dump_syncpoints(struct kgsl_device *device, break; } case KGSL_CMD_SYNCPOINT_TYPE_FENCE: + spin_lock_irqsave(&event->handle_lock, flags); + if (event->handle) dev_err(device->dev, " fence: [%pK] %s\n", event->handle->fence, event->handle->name); else dev_err(device->dev, " fence: invalid\n"); + + spin_unlock_irqrestore(&event->handle_lock, flags); break; } } @@ -119,6 +124,7 @@ static void syncobj_timer(unsigned long data) struct kgsl_drawobj *drawobj = DRAWOBJ(syncobj); struct kgsl_drawobj_sync_event *event; unsigned int i; + unsigned long flags; if (syncobj == NULL || drawobj->context == NULL) return; @@ -147,12 +153,16 @@ static void syncobj_timer(unsigned long data) i, event->context->id, event->timestamp); break; case KGSL_CMD_SYNCPOINT_TYPE_FENCE: + spin_lock_irqsave(&event->handle_lock, flags); + if (event->handle != NULL) { dev_err(device->dev, " [%d] FENCE %s\n", i, event->handle->fence ? event->handle->fence->name : "NULL"); kgsl_sync_fence_log(event->handle->fence); } + + spin_unlock_irqrestore(&event->handle_lock, flags); break; } } @@ -231,7 +241,7 @@ static void drawobj_destroy_sparse(struct kgsl_drawobj *drawobj) static void drawobj_destroy_sync(struct kgsl_drawobj *drawobj) { struct kgsl_drawobj_sync *syncobj = SYNCOBJ(drawobj); - unsigned long pending; + unsigned long pending, flags; unsigned int i; /* Zap the canary timer */ @@ -262,8 +272,17 @@ static void drawobj_destroy_sync(struct kgsl_drawobj *drawobj) drawobj_sync_func, event); break; case KGSL_CMD_SYNCPOINT_TYPE_FENCE: - if (kgsl_sync_fence_async_cancel(event->handle)) + spin_lock_irqsave(&event->handle_lock, flags); + + if (kgsl_sync_fence_async_cancel(event->handle)) { + event->handle = NULL; + spin_unlock_irqrestore( + &event->handle_lock, flags); drawobj_put(drawobj); + } else { + spin_unlock_irqrestore( + &event->handle_lock, flags); + } break; } } @@ -325,12 +344,23 @@ EXPORT_SYMBOL(kgsl_drawobj_destroy); static void drawobj_sync_fence_func(void *priv) { + unsigned long flags; struct kgsl_drawobj_sync_event *event = priv; + drawobj_sync_expire(event->device, event); + trace_syncpoint_fence_expire(event->syncobj, event->handle ? event->handle->name : "unknown"); - drawobj_sync_expire(event->device, event); + spin_lock_irqsave(&event->handle_lock, flags); + + /* + * Setting the event->handle to NULL here make sure that + * other function does not dereference a invalid pointer. + */ + event->handle = NULL; + + spin_unlock_irqrestore(&event->handle_lock, flags); drawobj_put(&event->syncobj->base); } @@ -350,6 +380,7 @@ static int drawobj_add_sync_fence(struct kgsl_device *device, struct kgsl_drawobj_sync_event *event; struct sync_fence *fence = NULL; unsigned int id; + unsigned long flags; int ret = 0; fence = sync_fence_fdget(sync->fd); @@ -368,18 +399,23 @@ static int drawobj_add_sync_fence(struct kgsl_device *device, event->device = device; event->context = NULL; + spin_lock_init(&event->handle_lock); set_bit(event->id, &syncobj->pending); trace_syncpoint_fence(syncobj, fence->name); + spin_lock_irqsave(&event->handle_lock, flags); + event->handle = kgsl_sync_fence_async_wait(sync->fd, drawobj_sync_fence_func, event); if (IS_ERR_OR_NULL(event->handle)) { ret = PTR_ERR(event->handle); - clear_bit(event->id, &syncobj->pending); event->handle = NULL; + spin_unlock_irqrestore(&event->handle_lock, flags); + + clear_bit(event->id, &syncobj->pending); drawobj_put(drawobj); @@ -390,6 +426,8 @@ static int drawobj_add_sync_fence(struct kgsl_device *device, */ trace_syncpoint_fence_expire(syncobj, (ret < 0) ? "error" : fence->name); + } else { + spin_unlock_irqrestore(&event->handle_lock, flags); } sync_fence_put(fence); diff --git a/drivers/gpu/msm/kgsl_drawobj.h b/drivers/gpu/msm/kgsl_drawobj.h index fd9d2bc93f41..b208870e4c42 100644 --- a/drivers/gpu/msm/kgsl_drawobj.h +++ b/drivers/gpu/msm/kgsl_drawobj.h @@ -1,4 +1,4 @@ -/* Copyright (c) 2016, The Linux Foundation. All rights reserved. +/* Copyright (c) 2016-2017, The Linux Foundation. All rights reserved. * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License version 2 and @@ -114,6 +114,7 @@ struct kgsl_drawobj_sync { * register this event * @timestamp: Pending timestamp for the event * @handle: Pointer to a sync fence handle + * @handle_lock: Spin lock to protect handle * @device: Pointer to the KGSL device */ struct kgsl_drawobj_sync_event { @@ -123,6 +124,7 @@ struct kgsl_drawobj_sync_event { struct kgsl_context *context; unsigned int timestamp; struct kgsl_sync_fence_waiter *handle; + spinlock_t handle_lock; struct kgsl_device *device; };