DiscardableHandle Implementation

gpu/command_buffer/common/discardable_handle.cc

+// Copyright (c) 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "gpu/command_buffer/common/discardable_handle.h"
+#include "base/atomicops.h"
+
+namespace gpu {
+namespace {
+const uint32_t kHandleDeleted = 0;
+const uint32_t kHandleUnlocked = 1;
+const uint32_t kHandleLockedStart = 2;
+
+}  // namespace
+
+void DiscardableHandle::InitializeWithNewData(scoped_refptr<Buffer> buffer,
+                                              uint32_t byte_offset,
+                                              int32_t shm_id) {
+  buffer_ = std::move(buffer);
+  byte_offset_ = byte_offset;
+  shm_id_ = shm_id;
+
+  // Handle always starts locked.
+  base::subtle::NoBarrier_Store(AsAtomic(), kHandleLockedStart);
+}
+
+void DiscardableHandle::InitializeWithExistingData(scoped_refptr<Buffer> buffer,
+                                                   uint32_t byte_offset,
+                                                   int32_t shm_id) {
+  buffer_ = std::move(buffer);
+  byte_offset_ = byte_offset;
+  shm_id_ = shm_id;
+}
+
+void DiscardableHandle::Unlock() {
+  DCHECK(base::subtle::NoBarrier_Load(AsAtomic()) >= kHandleLockedStart);

[piman, 2017/03/20 23:51:44]

Will this be called by the service? If so, a malicious renderer can break this
DCHECK, making it invalid. Do we need other defenses?

(Either way, the DCHECK is racy, since the value can change between here and the
next line. An option is to take the return value and DCHECK/validate that).

[ericrk, 2017/03/27 22:58:13]

True, I wasn't so concerned with the DCHECK, as it's more of just a sanity check
- if it ever fires, we have a problem. I agree that it can miss cases. I've
moved this to use the return value from AtomicIncrement.

I assume it's not a security concern that the client can trigger a DCHECK (as
DCHECKs will be compiled out)?

[piman, 2017/03/28 00:43:00]

Generally, I prefer avoiding service-side DCHECKs, because they may give the
wrong impression. In this case, it says "The atomic can't be <
kHandleLockedStart". A reader might think "aha, so I'm safe to assume this is
true" and add code based on that assumption (e.g. look up in a table for a text
description of the value for debug purpose), therefore introducing a security
issue, because the assumption is not true in the case of a malicious client.
Even further, the DCHECK actively prevents one from writing a test that ensures
we behave correctly in the malicious client case (since it would trigger the
DCHECK).

Also, my question is more: if this condition is not true, what could happen?
E.g. could we end up in a situation where we double-delete a texture? Or leak
it? Do we need to keep a "deleted" flag on the service side (that's not exposed
to the client)?

[ericrk, 2017/03/28 01:12:10]

Fair enough, I'll remove the DCHECK. It was more there as an "FYI we have a code
bug" for me while developing, rather than as any sort of protection.

The worst case scenario if this is false is:
a) Texture becomes un-deletable by discardable system, increasing memory
pressure. (Although it will still be deleted if the client (renderer) is
destroyed or the client calls glDeleteTexture). This doesn't seem much worse
than the existing problem where a client (renderer) can allocate huge amounts of
textures.
b) Texture is deleted while "in use" by the client (note that if this is shared
with other clients, the server ID won't be deleted, just the client ID). This
can already happen if a compromised client issues glDeleteTexture commands for a
texture while it is still using it.

If we wanted to be super thorough, we could detect these cases and put the
object in an "invalid state", which would basically take it out of the
discardable pool, minimizing its impact on other things in the pool - not sure
this is necessary as a compromised client can already hammer the GPU process
pretty hard?

[ericrk, 2017/03/28 01:19:44]

hmm, plus, a malicious client could already just do the valid behavior of
allocating and locking a large number of textures to put pressure on the pool,
so not sure that the "invalid state" defense really buys us much.

[piman, 2017/03/28 21:47:25]

Right, if the worse that could happen is that the (malicious) client can do
things that it already can do (leak textures, access deleted textures), I'm
perfectly fine. It's just a bit hard to reason about with just that CL - I
assume we'll have to look closer at further CLs that use this.
Just note that accessing a deleted texture has well-defined behavior today
(DeleteTextures unbinds the texture if it's currently bound, BindTexture either
recreates it if bind_generates_resources is true, or generates
GL_INVALID_OPERATION otherwise), and we'll need to either be consistent with
that or define our own semantics (e.g. unbinding a texture asynchronously as a
side effect of eviction could be surprising; OTOH if you liked it you should
have put a lock on it - either way, let's define it).

+  // Barrier prevents gl operations from being re-ordered across Unlock.
+  base::subtle::Barrier_AtomicIncrement(AsAtomic(), -1);

[piman, 2017/03/20 23:51:44]

Here and other places where we use barriers (and/or acquire/release semantics):
I'm not exactly sure what reordering they are protecting against, because there
is otherwise no shared state that I can think of that would matter relative to
it, or it's already well-ordered.

For client writes to be visible by the service, there is already a barrier when
we Flush (or OrderingBarrier) command buffers, from the locks we take.

For service writes to be visible to the client, the only state that I think
could matter is the gpu::SharedState to communicate the advance of the get
pointer (which we need to be consistent with the unlock state), but that one
already has barriers (both acquire and release, actually) when writing. I guess
queries too, that happen after the Unlock, but they also use Release_Store so
we'd be good there. Otherwise, IPC replies (WaitForGetOffset/Token) include
stronger barriers as well.


I just want to make sure I fully understand the logic, maybe I missed something.

[ericrk, 2017/03/27 22:58:13]

I may have been over-thinking things. I was sort of thinking about this
construct abstractly (without the command buffer) when I added these
Acquire/Release semantics - trying to make it work for an arbitrarily threaded
program. 

Changed to NoBarrier and added comments.

+}
+
+bool DiscardableHandle::Delete() {
+  // Acquire semantics prevent GL operations after Delete from being re-ordered
+  // before.
+  return kHandleUnlocked == base::subtle::Acquire_CompareAndSwap(
+                                AsAtomic(), kHandleUnlocked, kHandleDeleted);
+}
+
+bool DiscardableHandle::Lock() {
+  while (true) {

[piman, 2017/03/20 23:51:44]

Will this be called on the service side? If so, can a malicious renderer
live-lock this loop?
(from reading the doc, it looks like it's only called from the client side, but
double-checking)

[ericrk, 2017/03/27 22:58:13]

This should be called client-side only. I'll update comments.

+    base::subtle::Atomic32 current_value =
+        base::subtle::NoBarrier_Load(AsAtomic());
+    if (current_value == kHandleDeleted) {
+      // Once a handle is deleted, it cannot be modified further.
+      return false;
+    }
+    base::subtle::Atomic32 new_value = current_value + 1;
+    // Acquire semantics prevent gl operations after Lock from being re-ordered
+    // before.
+    base::subtle::Atomic32 previous_value =
+        base::subtle::Acquire_CompareAndSwap(AsAtomic(), current_value,
+                                             new_value);
+    if (current_value == previous_value) {
+      return true;
+    }
+  }
+}
+
+bool DiscardableHandle::IsDeleted() const {
+  return kHandleDeleted == base::subtle::Acquire_Load(AsAtomic());

[piman, 2017/03/20 23:51:44]

Which side is planning to call this?
From the client side, it's racy in the sense that if it returns false, it tells
you nothing (because the service side may have deleted the texture right after
you loaded the value)
From the service side, it is looking at an untrusted value, making it possibly
dangerous to rely on?

[ericrk, 2017/03/27 22:58:13]

We only call this from the client side, to check if it's safe to re-use a
buffer.

When the client tells the service to delete a texture, we wait for IsDeleted to
be true before re-using that texture's discardable handle memory. (We don't want
to re-use the handle until after the service actually deletes it). We don't
really care about the case where it returns false (it's fine to narrowly miss
re-using it). We just care that it eventually returns true.

I've changed the name to CanBeReUsed to make this clearer. Shouldn't be an
ordering issue here, as after we delete a texture on the GPU thread and re-use
it on the client, the next request to re-use the data on the GPU thread will
have come over the command buffer.

+}
+
+bool DiscardableHandle::IsLockedForTesting() {
+  return kHandleLockedStart <= base::subtle::Acquire_Load(AsAtomic());
+}
+
+volatile base::subtle::Atomic32* DiscardableHandle::AsAtomic() const {
+  return reinterpret_cast<volatile base::subtle::Atomic32*>(
+      buffer_->GetDataAddress(byte_offset_, sizeof(base::subtle::Atomic32)));
+}
+
+}  // namespace gpu