diff options
| author | John Harrison <John.C.Harrison@Intel.com> | 2016-04-01 15:22:57 +0100 |
|---|---|---|
| committer | John Harrison <John.C.Harrison@Intel.com> | 2016-05-06 14:12:43 +0100 |
| commit | aedfea71d24ad95ea120871045263a080f69c65b (patch) | |
| tree | 0c1d00a7e90bdf47dfcba8bde3741c1f383a10f8 | |
| parent | e80d58939935d29ed029933a527bda41b8a3d5d6 (diff) | |
drm/i915: Fix clean up of file client list on execbuff failure
If an execbuff IOCTL call fails for some reason, it would leave the
request in the client list. The request clean up code would remove
this but only later on and only after the reference count has dropped
to zero. The entire sequence is contained within the driver mutex
lock. However, there is still a hole such that any code which does not
require the mutex lock could still find the request on the client list
and start using it. That would lead to broken reference counts, use of
dangling pointers and all sorts of other nastiness.
The throttle IOCTL in particular does not acquire the mutex and does
process the client list. And the likely situation of the execbuff
IOCTL failing is when the system is busy with lots of work
outstanding. That is exactly the situation where the throttle IOCTL
would try to wait on a request.
Currently, this hole is tiny - the gap between the reference count
dropping to zero and the free function being called in response.
However the next patch in this series enlarges that gap considerably
by deferring the free function (to remove the need for the mutex lock
when unreferencing requests).
v7: New patch in series.
Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
| -rw-r--r-- | drivers/gpu/drm/i915/i915_drv.h | 1 | ||||
| -rw-r--r-- | drivers/gpu/drm/i915/i915_gem.c | 5 | ||||
| -rw-r--r-- | drivers/gpu/drm/i915/i915_gem_execbuffer.c | 6 |
3 files changed, 8 insertions, 4 deletions
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 1c3a1caf4f40..707bf0f742f7 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2358,6 +2358,7 @@ static inline bool i915_gem_request_completed(struct drm_i915_gem_request *req) int i915_gem_request_add_to_client(struct drm_i915_gem_request *req, struct drm_file *file); +void i915_gem_request_remove_from_client(struct drm_i915_gem_request *request); static inline uint32_t i915_gem_request_get_seqno(struct drm_i915_gem_request *req) diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 5f4bc4e41312..26891a8d237b 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -1399,8 +1399,7 @@ int i915_gem_request_add_to_client(struct drm_i915_gem_request *req, return 0; } -static inline void -i915_gem_request_remove_from_client(struct drm_i915_gem_request *request) +void i915_gem_request_remove_from_client(struct drm_i915_gem_request *request) { struct drm_i915_file_private *file_priv = request->file_priv; @@ -2735,7 +2734,7 @@ static void i915_gem_request_free(struct fence *req_fence) WARN_ON(!mutex_is_locked(&req->engine->dev->struct_mutex)); - if (req->file_priv) + if (WARN_ON(req->file_priv)) i915_gem_request_remove_from_client(req); if (ctx) { diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c index 6ee4f00f620c..7c4c0b69f3e4 100644 --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -1662,8 +1662,12 @@ err: * must be freed again. If it was submitted then it is being tracked * on the active request list and no clean up is required here. */ - if (ret && !IS_ERR_OR_NULL(req)) + if (ret && !IS_ERR_OR_NULL(req)) { + if (req->file_priv) + i915_gem_request_remove_from_client(req); + i915_gem_request_cancel(req); + } mutex_unlock(&dev->struct_mutex); |