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

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

[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?

[ ... ]

[ ... ]

[ ... ]

                                        ^^^^

[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).

[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