From patchwork Thu Apr 9 21:21:12 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Sultan Alsawaf X-Patchwork-Id: 1268867 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=none (no SPF record) smtp.mailfrom=lists.ubuntu.com (client-ip=91.189.94.19; helo=huckleberry.canonical.com; envelope-from=kernel-team-bounces@lists.ubuntu.com; receiver=) Authentication-Results: ozlabs.org; dmarc=fail (p=none dis=none) header.from=canonical.com Received: from huckleberry.canonical.com (huckleberry.canonical.com [91.189.94.19]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 48yvGD642Hz9sSX; Fri, 10 Apr 2020 07:21:24 +1000 (AEST) Received: from localhost ([127.0.0.1] helo=huckleberry.canonical.com) by huckleberry.canonical.com with esmtp (Exim 4.86_2) (envelope-from ) id 1jMebv-00065J-LA; Thu, 09 Apr 2020 21:21:19 +0000 Received: from youngberry.canonical.com ([91.189.89.112]) by huckleberry.canonical.com with esmtp (Exim 4.86_2) (envelope-from ) id 1jMebt-000655-K3 for kernel-team@lists.ubuntu.com; Thu, 09 Apr 2020 21:21:17 +0000 Received: from mail-pj1-f72.google.com ([209.85.216.72]) by youngberry.canonical.com with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.86_2) (envelope-from ) id 1jMebt-0002ef-7m for kernel-team@lists.ubuntu.com; Thu, 09 Apr 2020 21:21:17 +0000 Received: by mail-pj1-f72.google.com with SMTP id v71so80830pjb.6 for ; Thu, 09 Apr 2020 14:21:17 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:subject:date:message-id:mime-version :content-transfer-encoding; bh=whOoNqnm3uKi12Fgxk0DnmYXKo8qciplPratrM90UcQ=; b=mj6wAcAWHoPLhkXfk6wtEH6Oklno6UAQ0kVUYqsySDuT3GfKPowzAhBYDQG29NSDSJ ryeelQj5MmaSGKvxtPg8K9aAdm+x38d2QMZuxTQgg6EzgkGEYwR8j3MaAvhGS++0xjUi RxDmCpXy4fr0VekgiXos7iV9Dm0Pa8blmo4fHH6imYMqXkP3OufBfq3rLRIXCa1O7rra 7XAvFaEasc5mClXg5+XFdZTJ7fK+tMg0XbSJGPtuSzUEq6HiE5d11f912TNBTBaRlJ0e BuF/ejWazxYQ1D1IUQY9zKimTKWoyRJMgqM03d1hVr5eHal6hghA2V6imTlOFFsBcv7W tK0Q== X-Gm-Message-State: AGi0PuZzM3H6GAMGdQ7O99R2F1DajxQOPfcEagIrlpH9Fluczhlz16HT Ssw+EQh0Dbr20Q1v+EfcB8S6qNck2J7yTB1JcTfLwmYd4jNrm5lr+ZUY0Sc4RTb4i1Dfz2UfVcD DQgV+IQHfJRIjwzB1s/KH+WpJjno1aO550TnqUe5PKA== X-Received: by 2002:a63:6486:: with SMTP id y128mr1398734pgb.119.1586467275445; Thu, 09 Apr 2020 14:21:15 -0700 (PDT) X-Google-Smtp-Source: APiQypK4wwR4JqOIY6U6ONf8QfB0fv6ZGkByVJ3Im26tYIvQtW0ovIuXV+DVHU/NGMnD+U1wSi9igw== X-Received: by 2002:a63:6486:: with SMTP id y128mr1398694pgb.119.1586467274910; Thu, 09 Apr 2020 14:21:14 -0700 (PDT) Received: from sultan-box.conference (static-198-54-129-52.cust.tzulo.com. [198.54.129.52]) by smtp.gmail.com with ESMTPSA id w5sm18003pfw.154.2020.04.09.14.21.13 for (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 09 Apr 2020 14:21:14 -0700 (PDT) From: Sultan Alsawaf To: kernel-team@lists.ubuntu.com Subject: [Unstable][PATCH] UBUNTU: SAUCE: drm/i915: Synchronize active and retire callbacks Date: Thu, 9 Apr 2020 14:21:12 -0700 Message-Id: <20200409212112.15182-1-sultan.alsawaf@canonical.com> X-Mailer: git-send-email 2.20.1 MIME-Version: 1.0 X-BeenThere: kernel-team@lists.ubuntu.com X-Mailman-Version: 2.1.20 Precedence: list List-Id: Kernel team discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: kernel-team-bounces@lists.ubuntu.com Sender: "kernel-team" From: Sultan Alsawaf Active and retire callbacks can run simultaneously, causing panics and mayhem. The most notable case is with the intel_context_pin/unpin race that causes ring and page table corruption. In 5.4, this race is more noticeable because intel_ring_unpin() sets ring->vaddr to NULL and causes a clean NULL-pointer-dereference panic, but in newer kernels this race goes unnoticed. Here is an example of a crash caused by this race on 5.4: BUG: unable to handle page fault for address: 0000000000003448 RIP: 0010:gen8_emit_flush_render+0x163/0x190 Call Trace: execlists_request_alloc+0x25/0x40 __i915_request_create+0x1f4/0x2c0 i915_request_create+0x71/0xc0 i915_gem_do_execbuffer+0xb98/0x1a80 ? preempt_count_add+0x68/0xa0 ? _raw_spin_lock+0x13/0x30 ? _raw_spin_unlock+0x16/0x30 i915_gem_execbuffer2_ioctl+0x1de/0x3c0 ? i915_gem_busy_ioctl+0x7f/0x1d0 ? i915_gem_execbuffer_ioctl+0x2d0/0x2d0 drm_ioctl_kernel+0xb2/0x100 drm_ioctl+0x209/0x360 ? i915_gem_execbuffer_ioctl+0x2d0/0x2d0 ksys_ioctl+0x87/0xc0 __x64_sys_ioctl+0x16/0x20 do_syscall_64+0x4e/0x150 entry_SYSCALL_64_after_hwframe+0x44/0xa9 Protect the active and retire callbacks with their own lock to prevent them from running at the same time as one another. Fixes: 12c255b5dad1 ("drm/i915: Provide an i915_active.acquire callback") Cc: Signed-off-by: Sultan Alsawaf Signed-off-by: Sultan Alsawaf --- drivers/gpu/drm/i915/i915_active.c | 52 ++++++++++++++++++++---- drivers/gpu/drm/i915/i915_active.h | 10 ++--- drivers/gpu/drm/i915/i915_active_types.h | 2 + 3 files changed, 50 insertions(+), 14 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_active.c b/drivers/gpu/drm/i915/i915_active.c index b0a499753526..5802233f71ec 100644 --- a/drivers/gpu/drm/i915/i915_active.c +++ b/drivers/gpu/drm/i915/i915_active.c @@ -147,8 +147,22 @@ __active_retire(struct i915_active *ref) spin_unlock_irqrestore(&ref->tree_lock, flags); /* After the final retire, the entire struct may be freed */ - if (ref->retire) - ref->retire(ref); + if (ref->retire) { + if (ref->active) { + bool freed = false; + + /* Don't race with the active callback, and avoid UaF */ + down_write(&ref->rwsem); + ref->freed = &freed; + ref->retire(ref); + if (!freed) { + ref->freed = NULL; + up_write(&ref->rwsem); + } + } else { + ref->retire(ref); + } + } /* ... except if you wait on it, you must manage your own references! */ wake_up_var(ref); @@ -278,7 +292,8 @@ void __i915_active_init(struct i915_active *ref, int (*active)(struct i915_active *ref), void (*retire)(struct i915_active *ref), struct lock_class_key *mkey, - struct lock_class_key *wkey) + struct lock_class_key *wkey, + struct lock_class_key *rkey) { unsigned long bits; @@ -287,8 +302,13 @@ void __i915_active_init(struct i915_active *ref, ref->flags = 0; ref->active = active; ref->retire = ptr_unpack_bits(retire, &bits, 2); - if (bits & I915_ACTIVE_MAY_SLEEP) + ref->freed = NULL; + if (ref->active && ref->retire) { + __init_rwsem(&ref->rwsem, "i915_active.rwsem", rkey); ref->flags |= I915_ACTIVE_RETIRE_SLEEPS; + } else if (bits & I915_ACTIVE_MAY_SLEEP) { + ref->flags |= I915_ACTIVE_RETIRE_SLEEPS; + } spin_lock_init(&ref->tree_lock); ref->tree = RB_ROOT; @@ -417,8 +437,20 @@ int i915_active_acquire(struct i915_active *ref) return err; if (likely(!i915_active_acquire_if_busy(ref))) { - if (ref->active) - err = ref->active(ref); + if (ref->active) { + if (ref->retire) { + /* + * This can be a recursive call, and the mutex + * above already protects from concurrent active + * callbacks, so a read lock fits best. + */ + down_read(&ref->rwsem); + err = ref->active(ref); + up_read(&ref->rwsem); + } else { + err = ref->active(ref); + } + } if (!err) { spin_lock_irq(&ref->tree_lock); /* __active_retire() */ debug_active_activate(ref); @@ -502,16 +534,20 @@ int i915_request_await_active(struct i915_request *rq, struct i915_active *ref) return err; } -#if IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM) void i915_active_fini(struct i915_active *ref) { + if (ref->freed) { + *ref->freed = true; + up_write(&ref->rwsem); + } +#if IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM) debug_active_fini(ref); GEM_BUG_ON(atomic_read(&ref->count)); GEM_BUG_ON(work_pending(&ref->work)); GEM_BUG_ON(!RB_EMPTY_ROOT(&ref->tree)); mutex_destroy(&ref->mutex); -} #endif +} static inline bool is_idle_barrier(struct active_node *node, u64 idx) { diff --git a/drivers/gpu/drm/i915/i915_active.h b/drivers/gpu/drm/i915/i915_active.h index 51e1e854ca55..b684b1fdcc02 100644 --- a/drivers/gpu/drm/i915/i915_active.h +++ b/drivers/gpu/drm/i915/i915_active.h @@ -153,14 +153,16 @@ void __i915_active_init(struct i915_active *ref, int (*active)(struct i915_active *ref), void (*retire)(struct i915_active *ref), struct lock_class_key *mkey, - struct lock_class_key *wkey); + struct lock_class_key *wkey, + struct lock_class_key *rkey); /* Specialise each class of i915_active to avoid impossible lockdep cycles. */ #define i915_active_init(ref, active, retire) do { \ static struct lock_class_key __mkey; \ static struct lock_class_key __wkey; \ + static struct lock_class_key __rkey; \ \ - __i915_active_init(ref, active, retire, &__mkey, &__wkey); \ + __i915_active_init(ref, active, retire, &__mkey, &__wkey, &__rkey); \ } while (0) int i915_active_ref(struct i915_active *ref, @@ -200,11 +202,7 @@ i915_active_is_idle(const struct i915_active *ref) return !atomic_read(&ref->count); } -#if IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM) void i915_active_fini(struct i915_active *ref); -#else -static inline void i915_active_fini(struct i915_active *ref) { } -#endif int i915_active_acquire_preallocate_barrier(struct i915_active *ref, struct intel_engine_cs *engine); diff --git a/drivers/gpu/drm/i915/i915_active_types.h b/drivers/gpu/drm/i915/i915_active_types.h index 6360c3e4b765..aaee2548cb19 100644 --- a/drivers/gpu/drm/i915/i915_active_types.h +++ b/drivers/gpu/drm/i915/i915_active_types.h @@ -32,6 +32,8 @@ struct active_node; struct i915_active { atomic_t count; struct mutex mutex; + struct rw_semaphore rwsem; + bool *freed; spinlock_t tree_lock; struct active_node *cache;