Re: [PATCH net v11 09/17] rxrpc: Fix packet encryption error handling

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The patch renumbers the return-value comment block in
  rxrpc_send_data() (net/rxrpc/sendmsg.c) and adds a new item (5) for…
- [Low] The patch adds a new userspace-visible sendmsg() behaviour on
  AF_RXRPC sockets that Documentation/networking/rxrpc.rst never…

Pre-existing issues:
- [Low] This problem wasn't introduced by this patch, but the
  rxrpc_kernel_send_data documentation block the patch edits…

--

rxrpc: Fix packet encryption error handling

This changes rxrpc_send_data() so that -ENOMEM from ->secure_packet() is
treated as retryable. The data just added to the last packet is removed
again and the iterator is rewound. Any other encryption error sets a new
sticky RXRPC_CALL_TX_ERROR flag, and after that every data send on the
call returns -EIO until the application aborts the call.

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.
>  
>       notify_end_rx can be NULL or it can be used to specify a function to be
>       called when the call changes state to end the Tx phase.  This function is

[Severity: Low]
This isn't a bug introduced by this patch, but it is in the same
rxrpc_kernel_send_data() block that is being edited. The prototype
documented just above this hunk doesn't match the header:

Documentation/networking/rxrpc.rst:
	typedef void (*rxrpc_notify_end_tx_t)(struct sock *sk,
					      unsigned long user_call_ID,
					      struct sk_buff *skb);

include/net/af_rxrpc.h:
typedef void (*rxrpc_notify_end_tx_t)(struct sock *, struct rxrpc_call *,
				      unsigned long);

rxrpc_notify_end_tx() in net/rxrpc/sendmsg.c calls it as
notify_end_tx(&rx->sk, call, call->user_call_ID), which matches the header.
So the documented version has the wrong argument types and order, and an
sk_buff argument that doesn't exist.

The documentation also calls the parameter notify_end_rx, but the header
calls it notify_end_tx.

Could the documented prototype and parameter name be updated to match
include/net/af_rxrpc.h?

[ ... ]

> diff --git a/net/rxrpc/sendmsg.c b/net/rxrpc/sendmsg.c
> index 80c6afc647870..e3b6347888bfe 100644
> --- a/net/rxrpc/sendmsg.c
> +++ b/net/rxrpc/sendmsg.c

[ ... ]

> @@ -372,6 +373,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;
> +	}

[Severity: Low]
This changes what userspace sees from sendmsg() on AF_RXRPC sockets. If
->secure_packet() fails with anything other than -ENOMEM,
rxrpc_send_data() sets RXRPC_CALL_TX_ERROR. From then on, every data send
on that call returns -EIO here until the application aborts the call.

Should this be described in Documentation/networking/rxrpc.rst? The only
change there is to the rxrpc_kernel_send_data() paragraph.

The "Notes on sendmsg" section and the client/server walkthroughs don't
say that a call can become unusable for sending, that it then returns
-EIO, or that it has to be aborted explicitly. Right now the only place
this is described is item (5) of the comment in rxrpc_send_data().

The ENOMEM rewind can make a !MSG_MORE sendmsg() return a short count, but
the existing text about returning the amount of data consumed already
covers that. The missing part is mainly the sticky -EIO state and the need
to abort.

[ ... ]

> @@ -481,22 +505,44 @@ static int rxrpc_send_data(struct rxrpc_sock *rx,
>  	 *
>  	 * (4) If another sendmsg() has already queued the last packet: -EPROTO.
>  	 *
> -	 * (5) If we queue the last packet: the amount copied (which may be
> +	 * (5) If an error occurs that may have corrupted the transmission
> +	 *     buffer (e.g. crypto failure) or unusable crypto was encountered:
> +	 *     the error given (and RXRPC_CALL_TX_ERROR is set to cause -EIO to
> +	 *     be returned from further calls).
> +	 *
> +	 * (6) If we queue the last packet: the amount copied (which may be
>  	 *     zero).  recvmsg() should be used to collect the result.
>  	 *
> -	 * (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 (1)-(4), 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.
> +	 * For (1)-(6), there's no point in continuing with the sendmsg() and
> +	 * we no longer care how much has been queued as the call is no longer
> +	 * viable.  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.

[Severity: Low]
Should this range be (1)-(5)?

After the renumbering, item (6) is the case where the last packet was
queued. That is the normal, successful end of the Tx phase.

Saying the call is no longer viable and should be aborted contradicts
item (6) itself, which says recvmsg() should be used to collect the
result.

It also doesn't match rxrpc_kernel_send_data(), which treats consuming
all the data as success:

	if (msg_data_left(msg) == 0) {
		ret = 0;
		break;
	}

Documentation/networking/rxrpc.rst agrees, saying the function "returns 0
if all the data is queued".

None of the later patches in the series touch net/rxrpc/sendmsg.c, so the
wrong range would stay in the final state.

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