Re: [PATCH net v10 07/14] rxrpc: Fix packet encryption error handling

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:

Critical: 0 · High: 1 · Medium: 0 · Low: 1 (2)

- [High] rxrpc_send_data() leaks call->user_mutex on the newly added
  terminal crypto-error path.
- [Low] Return-value documentation vs behaviour in rxrpc_send_data(): the
  reworked comment (net/rxrpc/sendmsg.c:492-521) adds item '(3) If an…

Pre-existing issues:
- [Low] Documentation/networking/rxrpc.rst (in the 'Send data through a
  call' section that this patch edits) declares the callback type as…
- [Low] The paragraph reworded by this patch in
  Documentation/networking/rxrpc.rst:881-884 states 'msg must not…

--

rxrpc: Fix packet encryption error handling

The patch reworks the ->secure_packet() failure handling in
rxrpc_send_data() so that -ENOMEM is treated as retryable (rewinding the
last packet and the iterator), while any other error marks the call with a
new RXRPC_CALL_TX_ERROR flag so subsequent sendmsg() attempts return -EIO.
The documentation and return-value comment are updated to match.

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

> diff --git a/Documentation/networking/rxrpc.rst b/Documentation/networking/rxrpc.rst
> index 01ad12fdf3058..9b7eeae841485 100644
> --- a/Documentation/networking/rxrpc.rst
> +++ b/Documentation/networking/rxrpc.rst
> @@ -878,8 +878,10 @@ The kernel interface functions are as follows:
>       exclusively to in-kernel virtual addresses.  msg.msg_flags may be given
>       MSG_MORE if there will be subsequent data sends for this call.
>  
> -     The msg must not specify a destination address, control data or any flags
> -     other than MSG_MORE.
> +     msg must not specify a destination address, control data or any flags
> +     other than MSG_MORE.  The last-packet flag will only be set on the
> +     outgoing packet if MSG_MORE is not set and all the data in the iterator is
> +     buffered.

[Severity: Low]
This isn't a bug introduced by this patch, but while this paragraph is being
reworded: is the "any flags other than MSG_MORE" restriction still accurate?

The only in-tree caller, afs_make_call() in fs/afs/rxrpc.c, does:

	msg.msg_flags		= MSG_WAITALL | (call->write_iter ? MSG_MORE : 0);

and rxrpc_send_data() itself looks at MSG_WAITALL on the wait-for-space
path:

	ret = rxrpc_wait_for_tx_window(rx, call, &timeo,
				       msg->msg_flags & MSG_WAITALL);

Should MSG_WAITALL be mentioned as permitted here?

[Severity: Low]
This is a pre-existing documentation issue rather than something this patch
introduces, but it is in the same "Send data through a call" block being
edited.  The documented callback prototype is:

	typedef void (*rxrpc_notify_end_tx_t)(struct sock *sk,
					      unsigned long user_call_ID,
					      struct sk_buff *skb);

whereas include/net/af_rxrpc.h has:

	typedef void (*rxrpc_notify_end_tx_t)(struct sock *, struct rxrpc_call *,
					      unsigned long);

and rxrpc_notify_end_tx() in net/rxrpc/sendmsg.c invokes it as
notify_end_tx(&rx->sk, call, call->user_call_ID).  Could the documented
prototype be updated while this section is being touched?

[ ... ]

> diff --git a/net/rxrpc/sendmsg.c b/net/rxrpc/sendmsg.c
> index 1012d2d337a55..ed7ff32da1847 100644
> --- a/net/rxrpc/sendmsg.c
> +++ b/net/rxrpc/sendmsg.c

[ ... ]

> @@ -352,6 +353,13 @@ static int rxrpc_send_data(struct rxrpc_sock *rx,
>  		ret = -EPROTO;
>  		goto out_unlock;
>  	}
> +	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);
> +		ret = -EIO;
> +		goto out_unlock;
> +	}
>  
>  	ret = -EPIPE;
>  	if (sk->sk_shutdown & SEND_SHUTDOWN)

[ ... ]

> @@ -440,14 +449,29 @@ static int rxrpc_send_data(struct rxrpc_sock *rx,
>  		/* add the packet to the send queue if it's now full */
>  		if (!txb->space ||
>  		    (len == 0 && !more)) {
> +			/* 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) {
> +				/* Assume that ENOMEM here means that the
> +				 * encryption hasn't happened yet.  The data is
> +				 * aligned to avoid the need for slow buffering
> +				 * in the crypto walk.
> +				 */
> +				if (ret == -ENOMEM)
> +					goto maybe_error_rewind;
> +				set_bit(RXRPC_CALL_TX_ERROR, &call->flags);
> +				goto out;
                                        ^^^^

[Severity: High]
Should this be goto out_unlock?  The out: label sits after the unlock in
rxrpc_send_data():

	ret = copied;
out_unlock:
	mutex_unlock(&call->user_mutex);
out:

The previous code used "goto out_unlock" here, and the function is annotated
__releases(&call->user_mutex).  Does this new path return with
call->user_mutex still held?

For the userspace path, rxrpc_do_sendmsg() does:

	case RXRPC_CMD_SEND_DATA:
		ret = rxrpc_send_data(rx, call, msg, len, NULL);
		goto error_put;

which skips its own mutex_unlock(&call->user_mutex), so sendmsg() would
return to userspace owning the mutex, and any later sendmsg()/recvmsg() on
that call would block on it.

For in-kernel senders, rxrpc_kernel_send_data() takes the mutex on each
iteration and breaks out on ret < 0 without unlocking:

	for (;;) {
		mutex_lock(&call->user_mutex);

		ret = rxrpc_send_data(rx, call, msg, ...);
		if (ret < 0)
			break;

and fs/afs then goes to error_do_abort, which calls
rxrpc_kernel_abort_call() -> mutex_lock(&call->user_mutex) in the same
task.  Can that self-deadlock?

The error is reachable from the security layers without -ENOMEM being
involved, e.g. rxkad_secure_packet():

	ret = key_validate(call->conn->key);
	if (ret < 0)
		return ret;

which can return -EKEYEXPIRED/-EKEYREVOKED, and rxgk_secure_packet():

	gk = rxgk_get_key(call->conn, NULL);
	if (IS_ERR(gk))
		return PTR_ERR(gk) == -ESTALE ? -EKEYREJECTED : PTR_ERR(gk);

plus -EPERM and crypto errors.

One further consequence: since the sendmsg that sets RXRPC_CALL_TX_ERROR
never drops call->user_mutex, does the new -EIO gate at the reload: label
ever become reachable for that call?

This same "goto out" is still present at the tip of the series
(619b79dff7d2).

> +			}
> +
>  			if (len == 0 && !more)
>  				txb->flags |= RXRPC_LAST_PACKET;
> -
> -			ret = call->security->secure_packet(call, txb);
> -			if (ret < 0)
> -				goto out_unlock;
>  			rxrpc_queue_packet(rx, call, txb, notify_end_tx);
>  			call->tx_pending = NULL;
> +			rewind_by = 0;
>  
>  			/* At this point, if that was the last packet, it may
>  			 * have been transmitted and the reply (client call) or
> @@ -474,19 +498,22 @@ static int rxrpc_send_data(struct rxrpc_sock *rx,
>  	 *
>  	 * (2) If another sendmsg() has already queued the last packet: -EPROTO.
>  	 *
> -	 * (3) If the send side of the socket is shut down, -EPIPE.
> +	 * (3) If an error caused it to be impossible to continue with the
> +	 *     call: -EIO.

[Severity: Low]
Does item (3) describe what the failing sendmsg() actually returns?  The
sendmsg() in which ->secure_packet() fails returns the raw error from the
security layer (-EKEYEXPIRED, -EKEYREVOKED, -EKEYREJECTED, -EPERM, or a
crypto errno), since the new terminal path leaves ret untouched:

	set_bit(RXRPC_CALL_TX_ERROR, &call->flags);
	goto out;

-EIO only appears on a subsequent sendmsg() via the new RXRPC_CALL_TX_ERROR
test at the reload: label.

That path also bypasses maybe_error:, so when copied > 0 the error is
returned rather than the byte count, which is the opposite of the stated
preference of item (7) over item (8).  Could the comment say that -EIO is
returned on subsequent sendmsg() calls, and that the first failure reports
the security layer's error even if data was consumed from msg_iter?

> +	 *
> +	 * (4) If the send side of the socket is shut down, -EPIPE.
>  	 *
> -	 * (4) If the call is in the wrong state to transmit: -EPROTO.
> +	 * (5) If the call is in the wrong state to transmit: -EPROTO.
>  	 *
> -	 * (5) If the call has terminated early, likely due to an external
> +	 * (6) If the call has terminated early, likely due to an external
>  	 *     event such as being remotely aborted: -ESHUTDOWN.
>  	 *
> -	 * (6) If some data has been copied by this call: the amount copied
> +	 * (7) If some data has been copied by this call: the amount copied
>  	 *     (which will be greater than zero).
>  	 *
> -	 * (7) Any other error.
> +	 * (8) Any other error.
>  	 *
> -	 * For (2)-(5), there's no point in continuing with the sendmsg().  The
> +	 * For (2)-(6), there's no point in continuing with the sendmsg().  The
>  	 * app should abort the call (just in case the error came from
>  	 * somewhere else) and then use recvmsg() to collect the final result
>  	 * of the call.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260914151340.3227501-1-dhowells%40redhat.com