Re: [PATCH v5 1/9] greybus: operation: keep request active across message_send

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] greybus: operation: double put in gb_operation_request_send() on send error
- [Critical] greybus: operation: race between gb_operation_get_active() calls on send
--

commit 1f5c8fb4470157e5c81c1041163bd49efb2ea63b
Author: Ayush Singh <ayush@beagleboard.org>

greybus: operation: keep request active across message_send

This commit takes an extra reference and active count on the Greybus
operation across gb_message_send() in gb_operation_request_send() so that
operation cancellation waits for the host driver's send path to return.

> diff --git a/drivers/greybus/operation.c b/drivers/greybus/operation.c
> index 7e12ffb2dd60d..a63bc2eb164e9 100644
> --- a/drivers/greybus/operation.c
> +++ b/drivers/greybus/operation.c
> @@ -766,7 +766,25 @@ int gb_operation_request_send(struct gb_operation *operation,
>  	if (ret)
>  		goto err_put;
>  
> +	/*
> +	 * Completion can drop the reference and active count above while the
> +	 * host driver is still sending. Hold another of each across the send, so
> +	 * that cancelling the operation waits for the host driver to return.
> +	 */
> +	gb_operation_get(operation);
> +	ret = gb_operation_get_active(operation);
> +	if (ret) {
> +		/*
> +		 * The connection is being torn down, and the operation, which is
> +		 * already on its list, will be cancelled and completed.
> +		 */
> +		gb_operation_put(operation);
> +		return 0;
> +	}

[Severity: Critical]
Because the first gb_operation_get_active(operation) call adds operation->links
to connection->operations with operation->active == 1 and then drops
connection->lock before the second reference and active count are acquired,
can a concurrent teardown race in the window between the two
gb_operation_get_active() calls?

1. If gb_connection_disable() or gb_connection_disable_forced() runs right
after the first gb_operation_get_active() unlocks connection->lock,
gb_connection_cancel_operations() -> gb_operation_cancel() queues
gb_operation_work(). If gb_operation_work() runs before this thread reaches
the second gb_operation_get(operation), it invokes operation->callback(),
decrements operation->active to 0 (waking gb_operation_cancel() before
gb_message_send() has even started), and drops the completion reference. For
asynchronous callers whose callback drops the caller's own reference (such as
gb_loopback_async_operation_callback() in drivers/staging/greybus/loopback.c),
operation->kref can reach 0 and free operation before the second
gb_operation_get() and gb_operation_get_active() touch it.

2. For GB_OPERATION_FLAG_CORE operations, gb_operation_get_active() permits
operations when connection->state is GB_CONNECTION_STATE_DISCONNECTING:

drivers/greybus/operation.c:gb_operation_get_active() {
	...
	case GB_CONNECTION_STATE_DISCONNECTING:
		if (!gb_operation_is_core(operation))
			goto err_unlock;
		break;
	...
}

If a core operation passes the first gb_operation_get_active() in
GB_CONNECTION_STATE_ENABLED, and gb_connection_disable() transitions to
GB_CONNECTION_STATE_DISCONNECTING and cancels/completes the operation in the
window before the second gb_operation_get_active(), won't the second
gb_operation_get_active() still succeed in DISCONNECTING, re-add the already
completed operation to connection->operations, and send it?

Conversely, in drivers/greybus/connection.c, gb_connection_disable() and
_gb_connection_enable() call gb_connection_cancel_operations() when entering
GB_CONNECTION_STATE_DISCONNECTING, but not when subsequently setting
connection->state = GB_CONNECTION_STATE_DISABLED:

drivers/greybus/connection.c:gb_connection_disable() {
	...
	spin_lock_irq(&connection->lock);
	connection->state = GB_CONNECTION_STATE_DISCONNECTING;
	gb_connection_cancel_operations(connection, -ESHUTDOWN);
	spin_unlock_irq(&connection->lock);
	...
	connection->state = GB_CONNECTION_STATE_DISABLED;
}

If a core operation passes the first gb_operation_get_active() while in
GB_CONNECTION_STATE_DISCONNECTING and connection->state transitions to
GB_CONNECTION_STATE_DISABLED before the second gb_operation_get_active(), the
second call returns -ENOTCONN and this path returns 0 without dropping the
first active count or reference and without anyone cancelling the operation,
leaking operation and hanging synchronous callers waiting on
operation->completion.

> +
>  	ret = gb_message_send(operation->request, gfp);
> +	gb_operation_put_active(operation);
> +	gb_operation_put(operation);
>  	if (ret)
>  		goto err_put_active;

[Severity: Critical]
What happens if gb_operation_cancel() cancels the operation while
gb_message_send() is in progress, and gb_message_send() then returns a
non-zero error code?

While gb_message_send() is running, operation is on connection->operations
with operation->errno set to -EINPROGRESS. If a concurrent teardown calls
gb_connection_cancel_operations() -> gb_operation_cancel(operation, -ESHUTDOWN):

drivers/greybus/operation.c:gb_operation_cancel() {
	...
	if (gb_operation_result_set(operation, errno)) {
		gb_message_cancel(operation->request);
		queue_work(gb_operation_completion_wq, &operation->work);
	}
	...
}

gb_operation_work() will run operation->callback(operation) and call
gb_operation_put_active(operation) and gb_operation_put(operation), consuming
the initial completion active count and reference.

If gb_message_send() also returns an error (for example, on disconnect or TX
failure), gb_operation_request_send() drops the send active count and
reference here, and then jumps to err_put_active:

	ret = gb_message_send(operation->request, gfp);
	gb_operation_put_active(operation);
	gb_operation_put(operation);
	if (ret)
		goto err_put_active;
	...
err_put_active:
	gb_operation_put_active(operation);
err_put:
	gb_operation_put(operation);

Wouldn't jumping to err_put_active drop the completion active count and
operation->kref a second time (three puts for two gets), underflowing
operation->active to -1, underflowing operation->kref into a use-after-free,
and running both the asynchronous completion callback and the synchronous
error return path?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103-gb-uart-transport-v5-0-f8e326d6561f@beagleboard.org?part=1