Abort operation pruning only if it fails to make space.

keystore service's begin operation may sometimes encounter a situation
where the underlying device's begin operation fails because of too
many operations in progress. In that case, keystore attempts to prune
the oldest pruneable operation by invoking the underlying device's
abort operation. Regardless of whether the abort operation fails,
keystore then removes the operation from the list of in-progress
prunable operations.

The issue is that when the underlying device's abort operation fails,
keystore fails the begin operation that caused all this prunining.
This is despite the fact that keystore has managed to make space for
one more operation.

The fix is to fail the begin operation only if the pruning attempt
did not make space for a a new operation.

Bug: 22040842
Change-Id: Id98b2c6690de3cfb2a7b1d3bdd10742cc59ecbfa

3 files changed, 17 insertions, 3 deletions

diff --git a/keystore/keystore.cpp b/keystore/keystore.cpp
index b36f65f..cd28969 100644
--- a/keystore/keystore.cpp
+++ b/keystore/keystore.cpp
@@ -2506,7 +2506,16 @@ public:
         while (err == KM_ERROR_TOO_MANY_OPERATIONS && mOperationMap.hasPruneableOperation()) {
             sp<IBinder> oldest = mOperationMap.getOldestPruneableOperation();
             ALOGD("Ran out of operation handles, trying to prune %p", oldest.get());
-            if (abort(oldest) != ::NO_ERROR) {
+
+            // We mostly ignore errors from abort() below because all we care about is whether at
+            // least one pruneable operation has been removed.
+            size_t op_count_before = mOperationMap.getPruneableOperationCount();
+            int abort_error = abort(oldest);
+            size_t op_count_after = mOperationMap.getPruneableOperationCount();
+            if (op_count_after >= op_count_before) {
+                // Failed to create space for a new operation. Bail to avoid an infinite loop.
+                ALOGE("Failed to remove pruneable operation %p, error: %d",
+                      oldest.get(), abort_error);
                 break;
             }
             err = dev->begin(dev, purpose, &key, &inParams, &outParams, &handle);
diff --git a/keystore/operation.cpp b/keystore/operation.cpp
index aa37101..4a71922 100644
--- a/keystore/operation.cpp
+++ b/keystore/operation.cpp
@@ -103,10 +103,14 @@ void OperationMap::removeOperationTracking(sp<IBinder> token, sp<IBinder> appTok
     }
 }
 
-bool OperationMap::hasPruneableOperation() {
+bool OperationMap::hasPruneableOperation() const {
     return mLru.size() != 0;
 }
 
+size_t OperationMap::getPruneableOperationCount() const {
+    return mLru.size();
+}
+
 sp<IBinder> OperationMap::getOldestPruneableOperation() {
     if (!hasPruneableOperation()) {
         return sp<IBinder>(NULL);
diff --git a/keystore/operation.h b/keystore/operation.h
index 6806388..01c4dbe 100644
--- a/keystore/operation.h
+++ b/keystore/operation.h
@@ -56,7 +56,8 @@ public:
                       const keymaster1_device_t** outDev,
                       const keymaster_key_characteristics_t** outCharacteristics);
     bool removeOperation(sp<IBinder> token);
-    bool hasPruneableOperation();
+    bool hasPruneableOperation() const;
+    size_t getPruneableOperationCount() const;
     bool getOperationAuthToken(sp<IBinder> token, const hw_auth_token_t** outToken);
     bool setOperationAuthToken(sp<IBinder> token, const hw_auth_token_t* authToken);
     sp<IBinder> getOldestPruneableOperation();