greybus: operation: fix connection tear down

Fix connection tear down, which was done in an unsafe way that could
result in use-after-free as the per-connection list of operations was
iterated without any locking or refcounting.

Specifically, the operations list was iterated without holding any locks or
operation refcounts even though operations were being both removed from
the list and deallocated during per-operation cancellation. Any
operation completing during tear down could also cause corruption.

Change the per-connection operation list to only contain active
operations and use the recently introduced active counter to maintain
the list.

Add new helper that is called on connection tear down to cancel all
outstanding operations in a safe way by using proper locks and making
sure to hold a reference to any operation being cancelled.

Note that by verifying the connection state before incrementing the
active count we can make sure that all active operations have been
cancelled and that no new ones have been started when the helper
returns.

Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>

1 files changed, 30 insertions, 7 deletions

diff --git a/drivers/staging/greybus/connection.c b/drivers/staging/greybus/connection.c
index abc1f861ea28..2d19082a7f35 100644
--- a/drivers/staging/greybus/connection.c
+++ b/drivers/staging/greybus/connection.c
@@ -222,22 +222,45 @@ struct gb_connection *gb_connection_create(struct gb_bundle *bundle,
 }
 
 /*
+ * Cancel all active operations on a connection.
+ *
+ * Should only be called during connection tear down.
+ */
+static void gb_connection_cancel_operations(struct gb_connection *connection,
+						int errno)
+{
+	struct gb_operation *operation;
+
+	spin_lock_irq(&connection->lock);
+
+	WARN_ON(!list_empty(&connection->operations));
+
+	while (!list_empty(&connection->operations)) {
+		operation = list_last_entry(&connection->operations,
+						struct gb_operation, links);
+		gb_operation_get(operation);
+		spin_unlock_irq(&connection->lock);
+
+		gb_operation_cancel(operation, errno);
+		gb_operation_put(operation);
+
+		spin_lock_irq(&connection->lock);
+	}
+	spin_unlock_irq(&connection->lock);
+}
+
+/*
  * Tear down a previously set up connection.
  */
 void gb_connection_destroy(struct gb_connection *connection)
 {
-	struct gb_operation *operation;
-	struct gb_operation *next;
 	struct ida *id_map;
 
 	if (WARN_ON(!connection))
 		return;
 
-	if (WARN_ON(!list_empty(&connection->operations))) {
-		list_for_each_entry_safe(operation, next,
-					 &connection->operations, links)
-			gb_operation_cancel(operation, -ESHUTDOWN);
-	}
+	gb_connection_cancel_operations(connection, -ESHUTDOWN);
+
 	spin_lock_irq(&gb_connections_lock);
 	list_del(&connection->bundle_links);
 	list_del(&connection->hd_links);