Re: [PATCH net v5 02/10] rxrpc: Fix packet encryption error handling

[Simon Horman <horms@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev/#/patchset/20260729160108.2031453-2-dhowells@redhat.com?part=2
---
--- Patch [2]: [PATCH net v5 02/10] rxrpc: Fix packet encryption error handling ---
commit 59ddcd5fc94e6b2d9346d2e5e32c548184c0070b
Author: David Howells <dhowells@redhat.com>

rxrpc: Fix packet encryption error handling

This commit changes the error handling in rxrpc_send_data() when
secure_packet() fails. It adds logic to rewind the buffer by one byte on
retryable -ENOMEM errors and sets a new RXRPC_CALL_TX_ERROR flag for terminal
errors to prevent further sends.

Closes: https://sashiko.dev/#/patchset/20260702144919.172295-1-dhowells%40redhat.com

> diff --git a/net/rxrpc/sendmsg.c b/net/rxrpc/sendmsg.c
> index 4c754f78ece9a..d117a25b031dd 100644
> --- a/net/rxrpc/sendmsg.c
> +++ b/net/rxrpc/sendmsg.c
> @@ -330,12 +330,18 @@ static int rxrpc_send_data(struct rxrpc_sock *rx,
>  	bool more = msg->msg_flags & MSG_MORE;
>  	int ret, copied = 0;
>  
> -	if (test_bit(RXRPC_CALL_TX_NO_MORE, &call->flags)) {
> +	if (unlikely(test_bit(RXRPC_CALL_TX_NO_MORE, &call->flags))) {
>  		trace_rxrpc_abort(call->debug_id, rxrpc_sendmsg_late_send,
>  				  call->cid, call->call_id, call->rx_consumed,
>  				  0, -EPROTO);
>  		return -EPROTO;
>  	}
> +	if (unlikely(test_bit(RXRPC_CALL_TX_ERROR, &call->flags))) {
> +		trace_rxrpc_abort(call->debug_id, rxrpc_sendmsg_tx_error,
> +				  call->cid, call->call_id, call->rx_consumed,
> +				  0, -EIO);
> +		return -EIO;
> +	}

[Severity: High]
Does this check get bypassed after waiting for transmit space?

If the transmit window is full, rxrpc_send_data() jumps to wait_for_space
where it drops call->user_mutex and sleeps:

	ret = rxrpc_wait_for_tx_window(rx, call, &timeo,
				       msg->msg_flags & MSG_WAITALL);
    ...
	if (call->interruptibility == RXRPC_INTERRUPTIBLE) {
    ...
	} else {
		mutex_lock(&call->user_mutex);
	}
	goto reload;

When it reacquires the lock, it jumps to the reload label, which is located
after the RXRPC_CALL_TX_ERROR check.

If another thread encounters a terminal crypto error during the lock-drop
window and sets RXRPC_CALL_TX_ERROR, won't this thread blindly reuse the
corrupted call->tx_pending buffer when it wakes up?

[ ... ]

> @@ -440,12 +446,21 @@ static int rxrpc_send_data(struct rxrpc_sock *rx,
>  		/* add the packet to the send queue if it's now full */
>  		if (!txb->space ||
>  		    (msg_data_left(msg) == 0 && !more)) {
> -			if (msg_data_left(msg) == 0 && !more)
> -				txb->flags |= RXRPC_LAST_PACKET;
> -
> +			/* Do any required crypto.  If this fails, it could
> +			 * have corrupted the txbuf content with a partial
> +			 * encrypt.  Assume that ENOMEM is retryable, but
> +			 * everything else is terminal.
> +			 */
>  			ret = call->security->secure_packet(call, txb);
> -			if (ret < 0)
> +			if (ret < 0) {
> +				if (ret == -ENOMEM)
> +					goto maybe_error_rewind;

[ ... ]

> @@ -463,6 +478,20 @@ static int rxrpc_send_data(struct rxrpc_sock *rx,
>  	_leave(" = %d", call->error);
>  	return call->error;
>  
> +maybe_error_rewind:
> +	/* If we got a retryable error after copying all the supplied data into
> +	 * the last packet, we need to rewind the buffer by one byte so the
> +	 * caller knows they need to retry.
> +	 */
> +	if (copied && !more && !msg_data_left(msg)) {
> +		txb->space  += 1;
> +		txb->len    -= 1;
> +		txb->offset -= 1;
> +		copied      -= 1;
> +		if (call->tx_total_len != -1)
> +			call->tx_total_len += 1;
> +		iov_iter_revert(&msg->msg_iter, 1);
> +	}
>  maybe_error:

[Severity: High]
Does this error path drop the -ENOMEM error code and incorrectly return
success?

When secure_packet() fails with -ENOMEM on the final packet, it jumps to
maybe_error_rewind, and falls through to maybe_error.

If data was copied prior to the failure (copied > 0), maybe_error jumps
to success:

maybe_error:
	if (copied) {
		if (rxrpc_call_is_complete(call) &&
		    call->error < 0) {
			ret = call->error;
			goto out_txb;
		}
		goto success;
	}

Will this cause kernel callers like AFS (which use MSG_WAITALL) to see a
positive return value, assume the send succeeded, and hang forever waiting
for a server reply that will never arrive because the final packet was
dropped?

>  	if (copied) {
>  		if (rxrpc_call_is_complete(call) &&
>  		    call->error < 0) {