[PATCH net v11 03/17] rxrpc: Fix update of call->tx_pending without holding lock

[PATCH net v11 03/17] rxrpc: Fix update of call->tx_pending without holding lock

[David Howells]

Currently, rxrpc_send_data() updates call->tx_pending just before it
returns - but it won't be holding the call->user_mutex when it does this if
a wait was interrupted by a signal.  This would allow a parallel sendmsg()
to race.

Further, both the callers of rxrpc_send_data() call it with the lock held,
and then it returns an indication through the parameter list to say whether
it has dropped the lock or not - after which the callers both just drop the
lock if it's still held.

Fix this by:

 (1) Moving the release of call->user_mutex down into rxrpc_send_data() and
     get rid of the indicator parameter.  This makes it easier to see where
     the lock is held.

 (2) After waiting, if the attempt to reacquire the mutex is interrupted,
     just return directly there rather than going to out_unlock

 (3) Restricting the txb variable to inside the buffering loop and leaving
     ->tx_pending set until we've queued the buffer.

Note that there's a slight change in behaviour in that wait_for_space
failure now doesn't check for completion because it doesn't hold the call
user_mutex.  The caller, however, should re-issue the send and pick up any
error at a second attempt.

Fixes: b0f571ecd794 ("rxrpc: Fix locking in rxrpc's sendmsg")
Closes: https://sashiko.dev/#/patchset/20260702144919.172295-1-dhowells%40redhat.com
Signed-off-by: David Howells <dhowells@redhat.com>
cc: Marc Dionne <marc.dionne@auristor.com>
cc: Eric Dumazet <edumazet@google.com>
cc: "David S. Miller" <davem@davemloft.net>
cc: Jakub Kicinski <kuba@kernel.org>
cc: Paolo Abeni <pabeni@redhat.com>
cc: Simon Horman <horms@kernel.org>
cc: linux-afs@lists.infradead.org
cc: stable@vger.kernel.org
---
 net/rxrpc/sendmsg.c | 63 +++++++++++++++++++++------------------------
 1 file changed, 29 insertions(+), 34 deletions(-)

diff --git a/net/rxrpc/sendmsg.c b/net/rxrpc/sendmsg.c
index ed2c9a51005a..fb8d48418882 100644
--- a/net/rxrpc/sendmsg.c
+++ b/net/rxrpc/sendmsg.c
@@ -320,10 +320,9 @@ static int rxrpc_alloc_txqueue(struct sock *sk, struct rxrpc_call *call)
 static int rxrpc_send_data(struct rxrpc_sock *rx,
 			   struct rxrpc_call *call,
 			   struct msghdr *msg, size_t len,
-			   rxrpc_notify_end_tx_t notify_end_tx,
-			   bool *_dropped_lock)
+			   rxrpc_notify_end_tx_t notify_end_tx)
+	__releases(&call->user_mutex)
 {
-	struct rxrpc_txbuf *txb;
 	struct sock *sk = &rx->sk;
 	enum rxrpc_call_state state;
 	long timeo;
@@ -334,30 +333,26 @@ static int rxrpc_send_data(struct rxrpc_sock *rx,
 		trace_rxrpc_abort(call->debug_id, rxrpc_sendmsg_late_send,
 				  call->cid, call->call_id, call->rx_consumed,
 				  0, -EPROTO);
-		return -EPROTO;
+		ret = -EPROTO;
+		goto out_unlock;
 	}
 
 	timeo = sock_sndtimeo(sk, msg->msg_flags & MSG_DONTWAIT);
 
 	ret = rxrpc_wait_to_be_connected(call, &timeo);
 	if (ret < 0)
-		return ret;
+		goto out_unlock;
 
 	if (call->conn->state == RXRPC_CONN_CLIENT_UNSECURED) {
 		ret = rxrpc_init_client_conn_security(call->conn);
 		if (ret < 0)
-			return ret;
+			goto out_unlock;
 	}
 
 	/* this should be in poll */
 	sk_clear_bit(SOCKWQ_ASYNC_NOSPACE, sk);
 
 reload:
-	txb = call->tx_pending;
-	call->tx_pending = NULL;
-	if (txb)
-		rxrpc_see_txbuf(txb, rxrpc_txbuf_see_send_more);
-
 	ret = -EPIPE;
 	if (sk->sk_shutdown & SEND_SHUTDOWN)
 		goto maybe_error;
@@ -386,6 +381,8 @@ static int rxrpc_send_data(struct rxrpc_sock *rx,
 	}
 
 	do {
+		struct rxrpc_txbuf *txb = call->tx_pending;
+
 		if (!txb) {
 			size_t remain;
 
@@ -411,6 +408,9 @@ static int rxrpc_send_data(struct rxrpc_sock *rx,
 				ret = -ENOMEM;
 				goto maybe_error;
 			}
+			call->tx_pending = txb;
+		} else {
+			rxrpc_see_txbuf(txb, rxrpc_txbuf_see_send_more);
 		}
 
 		_debug("append");
@@ -445,9 +445,9 @@ static int rxrpc_send_data(struct rxrpc_sock *rx,
 
 			ret = call->security->secure_packet(call, txb);
 			if (ret < 0)
-				goto out;
+				goto out_unlock;
 			rxrpc_queue_packet(rx, call, txb, notify_end_tx);
-			txb = NULL;
+			call->tx_pending = NULL;
 		}
 	} while (msg_data_left(msg) > 0);
 
@@ -456,45 +456,46 @@ static int rxrpc_send_data(struct rxrpc_sock *rx,
 	if (rxrpc_call_is_complete(call) &&
 	    call->error < 0)
 		ret = call->error;
-out:
-	call->tx_pending = txb;
+out_unlock:
+	mutex_unlock(&call->user_mutex);
 	_leave(" = %d", ret);
 	return ret;
 
 call_terminated:
-	rxrpc_put_txbuf(txb, rxrpc_txbuf_put_send_aborted);
-	_leave(" = %d", call->error);
-	return call->error;
+	ret = call->error;
+	goto out_unlock;
 
 maybe_error:
 	if (copied)
 		goto success;
-	goto out;
+	goto out_unlock;
 
 efault:
 	ret = -EFAULT;
-	goto out;
+	goto out_unlock;
 
 wait_for_space:
 	ret = -EAGAIN;
 	if (msg->msg_flags & MSG_DONTWAIT)
 		goto maybe_error;
 	mutex_unlock(&call->user_mutex);
-	*_dropped_lock = true;
+
 	ret = rxrpc_wait_for_tx_window(rx, call, &timeo,
 				       msg->msg_flags & MSG_WAITALL);
 	if (ret < 0)
-		goto maybe_error;
+		goto out_nolock;
 	if (call->interruptibility == RXRPC_INTERRUPTIBLE) {
 		if (mutex_lock_interruptible(&call->user_mutex) < 0) {
 			ret = sock_intr_errno(timeo);
-			goto maybe_error;
+			goto out_nolock;
 		}
 	} else {
 		mutex_lock(&call->user_mutex);
 	}
-	*_dropped_lock = false;
 	goto reload;
+out_nolock:
+	_leave(" = %d [intr]", ret);
+	return copied ?: ret;
 }
 
 /*
@@ -660,7 +661,6 @@ rxrpc_new_client_call_for_sendmsg(struct rxrpc_sock *rx, struct msghdr *msg,
 int rxrpc_do_sendmsg(struct rxrpc_sock *rx, struct msghdr *msg, size_t len)
 {
 	struct rxrpc_call *call;
-	bool dropped_lock = false;
 	int ret;
 
 	struct rxrpc_send_params p = {
@@ -769,16 +769,15 @@ int rxrpc_do_sendmsg(struct rxrpc_sock *rx, struct msghdr *msg, size_t len)
 		ret = 0;
 		break;
 	case RXRPC_CMD_SEND_DATA:
-		ret = rxrpc_send_data(rx, call, msg, len, NULL, &dropped_lock);
-		break;
+		ret = rxrpc_send_data(rx, call, msg, len, NULL);
+		goto error_put;
 	default:
 		ret = -EINVAL;
 		break;
 	}
 
 out_put_unlock:
-	if (!dropped_lock)
-		mutex_unlock(&call->user_mutex);
+	mutex_unlock(&call->user_mutex);
 error_put:
 	rxrpc_put_call(call, rxrpc_call_put_sendmsg);
 	_leave(" = %d", ret);
@@ -808,7 +807,6 @@ int rxrpc_kernel_send_data(struct socket *sock, struct rxrpc_call *call,
 			   struct msghdr *msg, size_t len,
 			   rxrpc_notify_end_tx_t notify_end_tx)
 {
-	bool dropped_lock = false;
 	int ret;
 
 	_enter("{%d},", call->debug_id);
@@ -819,12 +817,9 @@ int rxrpc_kernel_send_data(struct socket *sock, struct rxrpc_call *call,
 	mutex_lock(&call->user_mutex);
 
 	ret = rxrpc_send_data(rxrpc_sk(sock->sk), call, msg, len,
-			      notify_end_tx, &dropped_lock);
+			      notify_end_tx);
 	if (ret == -ESHUTDOWN)
 		ret = call->error;
-
-	if (!dropped_lock)
-		mutex_unlock(&call->user_mutex);
 	_leave(" = %d", ret);
 	return ret;
 }

[PATCH net v11 04/17] rxrpc: Fix lack of short-send handling in rxrpc_kernel_send_data()

[David Howells]

Fix rxrpc_kernel_send_data() to loop around if it detects a short send.
David Laight suggested doing it here rather than wrapping all the calls in
loops.  Further, remove the len argument and use the iterator count instead
and return 0 on success, not the amount copied.

Note this is also a prerequisite for changing the way rxrpc_send_data()
works to return a short send rather than an error if some data was
buffered.

Fixes: 651350d10f93 ("[AF_RXRPC]: Add an interface to the AF_RXRPC module for the AFS filesystem to use")
Link: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260824091645.415423-1-dhowells%40redhat.com
Suggested-by: David Laight <david.laight.linux@gmail.com>
Signed-off-by: David Howells <dhowells@redhat.com>
cc: Marc Dionne <marc.dionne@auristor.com>
cc: Eric Dumazet <edumazet@google.com>
cc: "David S. Miller" <davem@davemloft.net>
cc: Jakub Kicinski <kuba@kernel.org>
cc: Paolo Abeni <pabeni@redhat.com>
cc: Simon Horman <horms@kernel.org>
cc: linux-afs@lists.infradead.org
cc: stable@vger.kernel.org
---
 Documentation/networking/rxrpc.rst |  6 ++++--
 fs/afs/rxrpc.c                     | 32 ++++++++++++------------------
 include/net/af_rxrpc.h             |  5 ++---
 net/rxrpc/rxperf.c                 | 25 ++++++++++-------------
 net/rxrpc/sendmsg.c                | 27 +++++++++++++++++--------
 5 files changed, 48 insertions(+), 47 deletions(-)

diff --git a/Documentation/networking/rxrpc.rst b/Documentation/networking/rxrpc.rst
index 8926dab8e2e6..01ad12fdf305 100644
--- a/Documentation/networking/rxrpc.rst
+++ b/Documentation/networking/rxrpc.rst
@@ -870,7 +870,6 @@ The kernel interface functions are as follows:
 	int rxrpc_kernel_send_data(struct socket *sock,
 				   struct rxrpc_call *call,
 				   struct msghdr *msg,
-				   size_t len,
 				   rxrpc_notify_end_tx_t notify_end_rx);
 
      This is used to supply either the request part of a client call or the
@@ -880,13 +879,16 @@ The kernel interface functions are as follows:
      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.  len is the total amount of data to transmit.
+     other than MSG_MORE.
 
      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
      called with a spinlock held to prevent the last DATA packet from being
      transmitted until the function returns.
 
+     It returns 0 if all the data is queued and a negative error code on
+     failure.
+
  (#) Receive data from a call::
 
 	int rxrpc_kernel_recv_data(struct socket *sock,
diff --git a/fs/afs/rxrpc.c b/fs/afs/rxrpc.c
index d82916657a3d..bcb8bb15d471 100644
--- a/fs/afs/rxrpc.c
+++ b/fs/afs/rxrpc.c
@@ -412,8 +412,7 @@ void afs_make_call(struct afs_call *call, gfp_t gfp)
 	msg.msg_controllen	= 0;
 	msg.msg_flags		= MSG_WAITALL | (call->write_iter ? MSG_MORE : 0);
 
-	ret = rxrpc_kernel_send_data(call->net->socket, rxcall,
-				     &msg, call->request_size,
+	ret = rxrpc_kernel_send_data(call->net->socket, rxcall, &msg,
 				     afs_notify_end_request_tx);
 	if (ret < 0)
 		goto error_do_abort;
@@ -425,7 +424,6 @@ void afs_make_call(struct afs_call *call, gfp_t gfp)
 
 		ret = rxrpc_kernel_send_data(call->net->socket,
 					     call->rxcall, &msg,
-					     iov_iter_count(&msg.msg_iter),
 					     afs_notify_end_request_tx);
 		*call->write_iter = msg.msg_iter;
 
@@ -871,7 +869,7 @@ void afs_send_empty_reply(struct afs_call *call)
 	msg.msg_controllen	= 0;
 	msg.msg_flags		= 0;
 
-	switch (rxrpc_kernel_send_data(net->socket, call->rxcall, &msg, 0,
+	switch (rxrpc_kernel_send_data(net->socket, call->rxcall, &msg,
 				       afs_notify_end_reply_tx)) {
 	case 0:
 		_leave(" [replied]");
@@ -897,7 +895,7 @@ void afs_send_simple_reply(struct afs_call *call, const void *buf, size_t len)
 	struct afs_net *net = call->net;
 	struct msghdr msg;
 	struct kvec iov[1];
-	int n;
+	int ret;
 
 	_enter("");
 
@@ -912,21 +910,17 @@ void afs_send_simple_reply(struct afs_call *call, const void *buf, size_t len)
 	msg.msg_controllen	= 0;
 	msg.msg_flags		= 0;
 
-	n = rxrpc_kernel_send_data(net->socket, call->rxcall, &msg, len,
-				   afs_notify_end_reply_tx);
-	if (n >= 0) {
-		/* Success */
-		_leave(" [replied]");
-		return;
-	}
-
-	if (n == -ENOMEM) {
-		_debug("oom");
-		rxrpc_kernel_abort_call(net->socket, call->rxcall,
-					RXGEN_SS_MARSHAL, -ENOMEM,
-					afs_abort_oom);
+	ret = rxrpc_kernel_send_data(net->socket, call->rxcall, &msg,
+				     afs_notify_end_reply_tx);
+	if (ret < 0) {
+		if (ret == -ENOMEM) {
+			_debug("oom");
+			rxrpc_kernel_abort_call(net->socket, call->rxcall,
+						RXGEN_SS_MARSHAL, -ENOMEM,
+						afs_abort_oom);
+		}
+		_leave(" [error]");
 	}
-	_leave(" [error]");
 }
 
 /*
diff --git a/include/net/af_rxrpc.h b/include/net/af_rxrpc.h
index 0fb4c41c9bbf..f3980348ed34 100644
--- a/include/net/af_rxrpc.h
+++ b/include/net/af_rxrpc.h
@@ -64,9 +64,8 @@ struct rxrpc_call *rxrpc_kernel_begin_call(struct socket *sock,
 					   bool upgrade,
 					   enum rxrpc_interruptibility interruptibility,
 					   unsigned int debug_id);
-int rxrpc_kernel_send_data(struct socket *, struct rxrpc_call *,
-			   struct msghdr *, size_t,
-			   rxrpc_notify_end_tx_t);
+int rxrpc_kernel_send_data(struct socket *sock, struct rxrpc_call *call,
+			   struct msghdr *msg, rxrpc_notify_end_tx_t notify_end_tx);
 int rxrpc_kernel_recv_data(struct socket *, struct rxrpc_call *,
 			   struct iov_iter *, size_t *, bool, u32 *, u16 *);
 bool rxrpc_kernel_abort_call(struct socket *, struct rxrpc_call *,
diff --git a/net/rxrpc/rxperf.c b/net/rxrpc/rxperf.c
index 26f97e1dd388..981c0596c774 100644
--- a/net/rxrpc/rxperf.c
+++ b/net/rxrpc/rxperf.c
@@ -501,8 +501,8 @@ static int rxperf_process_call(struct rxperf_call *call)
 	struct msghdr msg = {};
 	struct bio_vec bv;
 	struct kvec iov[1];
-	ssize_t n;
 	size_t reply_len = call->reply_len, len;
+	int ret;
 
 	rxrpc_kernel_set_tx_length(rxperf_socket, call->rxcall,
 				   reply_len + sizeof(rxperf_magic_cookie));
@@ -512,13 +512,11 @@ static int rxperf_process_call(struct rxperf_call *call)
 		bvec_set_page(&bv, ZERO_PAGE(0), len, 0);
 		iov_iter_bvec(&msg.msg_iter, WRITE, &bv, 1, len);
 		msg.msg_flags = MSG_MORE;
-		n = rxrpc_kernel_send_data(rxperf_socket, call->rxcall, &msg,
-					   len, rxperf_notify_end_reply_tx);
-		if (n < 0)
-			return n;
-		if (n == 0)
-			return -EIO;
-		reply_len -= n;
+		ret = rxrpc_kernel_send_data(rxperf_socket, call->rxcall, &msg,
+					     rxperf_notify_end_reply_tx);
+		if (ret < 0)
+			return ret;
+		reply_len -= len;
 	}
 
 	len = sizeof(rxperf_magic_cookie);
@@ -526,16 +524,13 @@ static int rxperf_process_call(struct rxperf_call *call)
 	iov[0].iov_len	= len;
 	iov_iter_kvec(&msg.msg_iter, WRITE, iov, 1, len);
 	msg.msg_flags = 0;
-	n = rxrpc_kernel_send_data(rxperf_socket, call->rxcall, &msg, len,
-				   rxperf_notify_end_reply_tx);
-	if (n >= 0)
-		return 0; /* Success */
-
-	if (n == -ENOMEM)
+	ret = rxrpc_kernel_send_data(rxperf_socket, call->rxcall, &msg,
+				     rxperf_notify_end_reply_tx);
+	if (ret == -ENOMEM)
 		rxrpc_kernel_abort_call(rxperf_socket, call->rxcall,
 					RXGEN_SS_MARSHAL, -ENOMEM,
 					rxperf_abort_oom);
-	return n;
+	return ret;
 }
 
 /*
diff --git a/net/rxrpc/sendmsg.c b/net/rxrpc/sendmsg.c
index fb8d48418882..393a2dcfda07 100644
--- a/net/rxrpc/sendmsg.c
+++ b/net/rxrpc/sendmsg.c
@@ -793,7 +793,6 @@ int rxrpc_do_sendmsg(struct rxrpc_sock *rx, struct msghdr *msg, size_t len)
  * @sock: The socket the call is on
  * @call: The call to send data through
  * @msg: The data to send
- * @len: The amount of data to send
  * @notify_end_tx: Notification that the last packet is queued.
  *
  * Allow a kernel service to send data on a call.  The call must be in an state
@@ -804,8 +803,7 @@ int rxrpc_do_sendmsg(struct rxrpc_sock *rx, struct msghdr *msg, size_t len)
  * Return: %0 if successful and a negative error code otherwise.
  */
 int rxrpc_kernel_send_data(struct socket *sock, struct rxrpc_call *call,
-			   struct msghdr *msg, size_t len,
-			   rxrpc_notify_end_tx_t notify_end_tx)
+			   struct msghdr *msg, rxrpc_notify_end_tx_t notify_end_tx)
 {
 	int ret;
 
@@ -814,12 +812,25 @@ int rxrpc_kernel_send_data(struct socket *sock, struct rxrpc_call *call,
 	ASSERTCMP(msg->msg_name, ==, NULL);
 	ASSERTCMP(msg->msg_control, ==, NULL);
 
-	mutex_lock(&call->user_mutex);
+	for (;;) {
+		mutex_lock(&call->user_mutex);
+
+		ret = rxrpc_send_data(rxrpc_sk(sock->sk), call, msg,
+				      msg_data_left(msg), notify_end_tx);
+		if (ret == -ESHUTDOWN)
+			ret = call->error;
+		if (ret < 0)
+			break;
+		if (msg_data_left(msg) == 0) {
+			ret = 0;
+			break;
+		}
+		if (ret == 0) {
+			ret = -EIO;
+			break;
+		}
+	}
 
-	ret = rxrpc_send_data(rxrpc_sk(sock->sk), call, msg, len,
-			      notify_end_tx);
-	if (ret == -ESHUTDOWN)
-		ret = call->error;
 	_leave(" = %d", ret);
 	return ret;
 }

Re: [PATCH net v11 04/17] rxrpc: Fix lack of short-send handling in rxrpc_kernel_send_data()

[netdev-bot+sashiko]

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 adds this sentence to
  Documentation/networking/rxrpc.rst: rxrpc_kernel_send_data() "returns…

Pre-existing issues:
- [Low] Documentation/networking/rxrpc.rst says the msg passed to
  rxrpc_kernel_send_data() must not carry "any flags other than…

--

rxrpc: Fix lack of short-send handling in rxrpc_kernel_send_data()

This makes rxrpc_kernel_send_data() keep looping after a short send from
rxrpc_send_data() until the iterator is drained. It also drops the len
argument and uses the iterator count instead. On success the function now
returns 0 rather than the amount copied.

Link: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260824091645.415423-1-dhowells%40redhat.com

> diff --git a/Documentation/networking/rxrpc.rst b/Documentation/networking/rxrpc.rst
> index 8926dab8e2e60..01ad12fdf3058 100644
> --- a/Documentation/networking/rxrpc.rst
> +++ b/Documentation/networking/rxrpc.rst

[ ... ]

> @@ -880,13 +879,16 @@ The kernel interface functions are as follows:
>       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.  len is the total amount of data to transmit.
> +     other than MSG_MORE.

[Severity: Low]
This is a pre-existing issue, and this patch did not introduce it. Is the
"any flags other than MSG_MORE" restriction still correct? The in-tree
caller afs_make_call() passes MSG_WAITALL:

fs/afs/rxrpc.c:afs_make_call() {
    ...
	msg.msg_flags		= MSG_WAITALL | (call->write_iter ? MSG_MORE : 0);
    ...
}

rxrpc_send_data() also acts on both MSG_DONTWAIT and MSG_WAITALL:

net/rxrpc/sendmsg.c:rxrpc_send_data() {
    ...
	timeo = sock_sndtimeo(sk, msg->msg_flags & MSG_DONTWAIT);
    ...
wait_for_space:
	ret = -EAGAIN;
	if (msg->msg_flags & MSG_DONTWAIT)
		goto maybe_error;
	...
	ret = rxrpc_wait_for_tx_window(rx, call, &timeo,
				       msg->msg_flags & MSG_WAITALL);
    ...
}

This sentence is already being edited here. Could it be changed to list
the flags that are actually honoured? The same wording is still present
at the end of the series.

>  
>       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
>       called with a spinlock held to prevent the last DATA packet from being
>       transmitted until the function returns.
>  
> +     It returns 0 if all the data is queued and a negative error code on
> +     failure.

[Severity: Low]
Is "queued" the right word here? Suppose MSG_MORE is set and the data
does not exactly fill a txbuf. In that case rxrpc_send_data() leaves the
partly filled txbuf in call->tx_pending and does not call
rxrpc_queue_packet():

net/rxrpc/sendmsg.c:rxrpc_send_data() {
    ...
		/* add the packet to the send queue if it's now full */
		if (!txb->space ||
		    (msg_data_left(msg) == 0 && !more)) {
			...
			rxrpc_queue_packet(rx, call, txb, notify_end_tx);
			call->tx_pending = NULL;
		}
    ...
}

At that point the iterator is empty, so the new loop in
rxrpc_kernel_send_data() returns 0:

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

Two examples of this path are rxperf_process_call() sending ZERO_PAGE
chunks, and afs_make_call() sending the request header before
write_iter.

So a return of 0 seems to mean the data was taken from the iterator and
buffered, not queued in the rxrpc_queue_packet() sense. A later patch in
the series, "rxrpc: Fix sendmsg length", uses the word "buffered" for
this same state in this document.

A negative return can also now follow a partial transfer. An earlier pass
of the loop may consume part of the iterator before a later pass fails.
Should the documentation say so as well?

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

[PATCH net v11 05/17] afs: Fix afs to abort the rxrpc call on send error

[David Howells]

Fix afs_send_empty_reply() and afs_send_simple_reply() to always try to
abort the rxrpc call rather than just aborting on -ENOMEM and otherwise
abandoning it.  If the call is already complete due to network failure or a
received abort, this will do nothing.

Also make afs_make_call() always abort on send error; again, it does
nothing if the rxrpc call is already dead.

Fixes: 08e0e7c82eea ("[AF_RXRPC]: Make the in-kernel AFS filesystem use AF_RXRPC.")
Link: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260824091645.415423-1-dhowells%40redhat.com
Signed-off-by: David Howells <dhowells@redhat.com>
cc: Marc Dionne <marc.dionne@auristor.com>
cc: Eric Dumazet <edumazet@google.com>
cc: "David S. Miller" <davem@davemloft.net>
cc: Jakub Kicinski <kuba@kernel.org>
cc: Paolo Abeni <pabeni@redhat.com>
cc: Simon Horman <horms@kernel.org>
cc: linux-afs@lists.infradead.org
cc: stable@vger.kernel.org
---
 fs/afs/rxrpc.c               | 36 +++++++++++-------------------------
 include/trace/events/rxrpc.h |  2 +-
 2 files changed, 12 insertions(+), 26 deletions(-)

diff --git a/fs/afs/rxrpc.c b/fs/afs/rxrpc.c
index bcb8bb15d471..c38458157ecc 100644
--- a/fs/afs/rxrpc.c
+++ b/fs/afs/rxrpc.c
@@ -441,10 +441,8 @@ void afs_make_call(struct afs_call *call, gfp_t gfp)
 	return;
 
 error_do_abort:
-	if (ret != -ECONNABORTED)
-		rxrpc_kernel_abort_call(call->net->socket, rxcall,
-					RX_USER_ABORT, ret,
-					afs_abort_send_data_error);
+	rxrpc_kernel_abort_call(call->net->socket, rxcall,
+				RX_USER_ABORT, ret, afs_abort_send_data_error);
 	if (call->async) {
 		afs_see_call(call, afs_call_trace_async_abort);
 		return;
@@ -857,6 +855,7 @@ void afs_send_empty_reply(struct afs_call *call)
 {
 	struct afs_net *net = call->net;
 	struct msghdr msg;
+	int ret;
 
 	_enter("");
 
@@ -869,22 +868,12 @@ void afs_send_empty_reply(struct afs_call *call)
 	msg.msg_controllen	= 0;
 	msg.msg_flags		= 0;
 
-	switch (rxrpc_kernel_send_data(net->socket, call->rxcall, &msg,
-				       afs_notify_end_reply_tx)) {
-	case 0:
-		_leave(" [replied]");
-		return;
-
-	case -ENOMEM:
-		_debug("oom");
+	ret = rxrpc_kernel_send_data(net->socket, call->rxcall, &msg,
+				     afs_notify_end_reply_tx);
+	if (ret < 0)
 		rxrpc_kernel_abort_call(net->socket, call->rxcall,
-					RXGEN_SS_MARSHAL, -ENOMEM,
-					afs_abort_oom);
-		fallthrough;
-	default:
-		_leave(" [error]");
-		return;
-	}
+					RXGEN_SS_MARSHAL, ret,
+					afs_abort_send_error);
 }
 
 /*
@@ -913,12 +902,9 @@ void afs_send_simple_reply(struct afs_call *call, const void *buf, size_t len)
 	ret = rxrpc_kernel_send_data(net->socket, call->rxcall, &msg,
 				     afs_notify_end_reply_tx);
 	if (ret < 0) {
-		if (ret == -ENOMEM) {
-			_debug("oom");
-			rxrpc_kernel_abort_call(net->socket, call->rxcall,
-						RXGEN_SS_MARSHAL, -ENOMEM,
-						afs_abort_oom);
-		}
+		rxrpc_kernel_abort_call(net->socket, call->rxcall,
+					RXGEN_SS_MARSHAL, ret,
+					afs_abort_send_error);
 		_leave(" [error]");
 	}
 }
diff --git a/include/trace/events/rxrpc.h b/include/trace/events/rxrpc.h
index 704a10de6670..554dfb777b93 100644
--- a/include/trace/events/rxrpc.h
+++ b/include/trace/events/rxrpc.h
@@ -20,10 +20,10 @@
 	/* AFS errors */						\
 	EM(afs_abort_general_error,		"afs-error")		\
 	EM(afs_abort_interrupted,		"afs-intr")		\
-	EM(afs_abort_oom,			"afs-oom")		\
 	EM(afs_abort_op_not_supported,		"afs-op-notsupp")	\
 	EM(afs_abort_probeuuid_negative,	"afs-probeuuid-neg")	\
 	EM(afs_abort_send_data_error,		"afs-send-data")	\
+	EM(afs_abort_send_error,		"afs-send-error")	\
 	EM(afs_abort_unmarshal_error,		"afs-unmarshal")	\
 	EM(afs_abort_unsupported_sec_class,	"afs-unsup-sec-class")	\
 	/* rxperf errors */						\

Re: [PATCH net v11 05/17] afs: Fix afs to abort the rxrpc call on send error

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 0 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] When sending a reply fails on an incoming cache-manager (CB)
  service call, the afs_call is never completed.

--

afs: Fix afs to abort the rxrpc call on send error

This patch makes afs_send_empty_reply() and afs_send_simple_reply() abort
the rxrpc call on any send error, not only on -ENOMEM. It also makes
afs_make_call() abort on any send error. The afs_abort_oom trace reason is
replaced with afs_abort_send_error.

Link: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260824091645.415423-1-dhowells%40redhat.com

> diff --git a/fs/afs/rxrpc.c b/fs/afs/rxrpc.c
> index bcb8bb15d471d..c38458157eccf 100644
> --- a/fs/afs/rxrpc.c
> +++ b/fs/afs/rxrpc.c

[ ... ]

> @@ -869,22 +868,12 @@ void afs_send_empty_reply(struct afs_call *call)
>  	msg.msg_controllen	= 0;
>  	msg.msg_flags		= 0;
>  
> -	switch (rxrpc_kernel_send_data(net->socket, call->rxcall, &msg,
> -				       afs_notify_end_reply_tx)) {
> -	case 0:
> -		_leave(" [replied]");
> -		return;
> -
> -	case -ENOMEM:
> -		_debug("oom");
> +	ret = rxrpc_kernel_send_data(net->socket, call->rxcall, &msg,
> +				     afs_notify_end_reply_tx);
> +	if (ret < 0)
>  		rxrpc_kernel_abort_call(net->socket, call->rxcall,
> -					RXGEN_SS_MARSHAL, -ENOMEM,
> -					afs_abort_oom);
> -		fallthrough;
> -	default:
> -		_leave(" [error]");
> -		return;
> -	}
> +					RXGEN_SS_MARSHAL, ret,
> +					afs_abort_send_error);
>  }

[Severity: High]
Is the afs_call still abandoned here when the send fails? The rxrpc call
is aborted, but nothing calls afs_set_call_complete(), so the afs_call
stays in AFS_CALL_SV_REPLYING. afs_send_simple_reply() has the same
pattern.

The commit message says the change stops "otherwise abandoning it". That
is true for the rxrpc call, but it does not seem to hold for the afs_call.

The only way out of SV_REPLYING is afs_notify_end_reply_tx():

    afs_set_call_state(call, AFS_CALL_SV_REPLYING, AFS_CALL_SV_AWAIT_ACK);

That callback only runs once the final DATA packet has been queued, and
that does not happen on a send error.

A later notification (from this abort or from a received abort) goes
through afs_wake_up_async_call() -> afs_process_async_call() ->
afs_deliver_to_call(). The loop there does not handle SV_REPLYING:

    while (state = READ_ONCE(call->state),
           state == AFS_CALL_CL_AWAIT_REPLY ||
           state == AFS_CALL_SV_AWAIT_OP_ID ||
           state == AFS_CALL_SV_AWAIT_REQUEST ||
           state == AFS_CALL_SV_AWAIT_ACK
           ) {

None of the CB call types has a ->done hook. So the drop_ref reference
set by afs_charge_preallocation() is never released, because only
afs_set_call_complete() drops it.

If that's right, the afs_call leaks along with its rxrpc_call, peer and
server references and its buffers. net->nr_outstanding_calls would then
never reach zero, and afs_close_socket() would block forever here during
netns teardown or module unload:

    wait_var_event(&net->nr_outstanding_calls,
                   !atomic_read(&net->nr_outstanding_calls));

A remote peer seems able to trigger this. It can send CB.Probe or
CB.CallBack and abort right after the last request packet. Then
SRXAFSCB_Probe() -> afs_send_empty_reply() -> rxrpc_kernel_send_data()
returns -ESHUTDOWN, and rxrpc_kernel_abort_call() does nothing.
-ENOMEM from txbuf or txqueue allocation leads to the same state.

afs_abort_service_call() in fs/afs/cmservice.c already pairs the two
steps:

    rxrpc_kernel_abort_call(call->net->socket, call->rxcall,
                            abort_code, error, why);
    afs_set_call_complete(call, error, 0);

Should the send-error paths in afs_send_empty_reply() and
afs_send_simple_reply() also call afs_set_call_complete(call, ret, 0)
after the abort? No later patch in the series changes these two
functions.

[ ... ]

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

[PATCH net v11 08/17] rxrpc: Fix error handling in rxrpc_send_data()

[David Howells]

Fix the error handling in rxrpc_send_data() so that it doesn't return an
error if it has successfully queued the last packet of a call, but the call
has seen to have completed after it did that.  Rather, leave it to
recvmsg() to report the completion (which it will do anyway).

The problem with trying to report the error twice is that the caller may
try to clean up the dead call twice.

Further, if we haven't queued the final packet yet, return -ESHUTDOWN if
the call is now marked complete (e.g. it got aborted by the peer) as
there's no point sendmsg() continuing to try to add data to a call if it is
defunct.  The application should abort the call and then call recvmsg() to
pick up the reason.

Fixes: 4ba68c519255 ("rxrpc: Return an error to sendmsg if call failed")
Signed-off-by: David Howells <dhowells@redhat.com>
cc: Marc Dionne <marc.dionne@auristor.com>
cc: Jeffrey Altman <jaltman@auristor.com>
cc: Eric Dumazet <edumazet@google.com>
cc: "David S. Miller" <davem@davemloft.net>
cc: Jakub Kicinski <kuba@kernel.org>
cc: Paolo Abeni <pabeni@redhat.com>
cc: Simon Horman <horms@kernel.org>
cc: linux-afs@lists.infradead.org
cc: stable@vger.kernel.org
---
 fs/afs/rxrpc.c      |  11 +++--
 net/rxrpc/rxperf.c  |  29 +++++++----
 net/rxrpc/sendmsg.c | 114 ++++++++++++++++++++++++++++++--------------
 3 files changed, 103 insertions(+), 51 deletions(-)

diff --git a/fs/afs/rxrpc.c b/fs/afs/rxrpc.c
index c38458157ecc..0b9027549580 100644
--- a/fs/afs/rxrpc.c
+++ b/fs/afs/rxrpc.c
@@ -448,13 +448,14 @@ void afs_make_call(struct afs_call *call, gfp_t gfp)
 		return;
 	}
 
-	if (ret == -ECONNABORTED) {
+	if (ret == -ESHUTDOWN) {
 		len = 0;
 		iov_iter_kvec(&msg.msg_iter, ITER_DEST, NULL, 0, 0);
-		rxrpc_kernel_recv_data(call->net->socket, rxcall,
-				       &msg.msg_iter, &len, false,
-				       &call->abort_code, &call->service_id);
-		call->responded = true;
+		ret = rxrpc_kernel_recv_data(call->net->socket, rxcall,
+					     &msg.msg_iter, &len, false,
+					     &call->abort_code, &call->service_id);
+		if (ret == -ECONNABORTED)
+			call->responded = true;
 	}
 	call->error = ret;
 	trace_afs_call_done(call);
diff --git a/net/rxrpc/rxperf.c b/net/rxrpc/rxperf.c
index 823eedc5d16f..5042e8bfca55 100644
--- a/net/rxrpc/rxperf.c
+++ b/net/rxrpc/rxperf.c
@@ -74,7 +74,7 @@ static struct workqueue_struct *rxperf_workqueue;
 static void rxperf_deliver_to_call(struct work_struct *work);
 static int rxperf_deliver_param_block(struct rxperf_call *call);
 static int rxperf_deliver_request(struct rxperf_call *call);
-static int rxperf_process_call(struct rxperf_call *call);
+static void rxperf_process_call(struct rxperf_call *call);
 static void rxperf_charge_preallocation(struct work_struct *work);
 
 static DECLARE_WORK(rxperf_charge_preallocation_work,
@@ -293,18 +293,28 @@ static void rxperf_deliver_to_call(struct work_struct *work)
 	       state == RXPERF_CALL_SV_AWAIT_ACK
 	       ) {
 		if (state == RXPERF_CALL_SV_AWAIT_ACK) {
-			if (!rxrpc_kernel_check_life(rxperf_socket, call->rxcall))
+			size_t len = 0;
+			iov_iter_kvec(&call->iter, ITER_DEST, NULL, 0, 0);
+			ret = rxrpc_kernel_recv_data(rxperf_socket,
+						     call->rxcall, &call->iter,
+						     &len, false, &remote_abort,
+						     &call->service_id);
+
+			if (ret == -EINPROGRESS || ret == -EAGAIN)
+				return;
+			if (ret < 0 || ret == 1) {
+				if (ret == 1)
+					ret = 0;
 				goto call_complete;
+			}
 			return;
 		}
 
 		ret = call->deliver(call);
-		if (ret == 0)
-			ret = rxperf_process_call(call);
-
 		switch (ret) {
 		case 0:
-			continue;
+			rxperf_process_call(call);
+			return;
 		case -EINPROGRESS:
 		case -EAGAIN:
 			return;
@@ -496,7 +506,7 @@ static int rxperf_deliver_request(struct rxperf_call *call)
 /*
  * Process a call for which we've received the request.
  */
-static int rxperf_process_call(struct rxperf_call *call)
+static void rxperf_process_call(struct rxperf_call *call)
 {
 	struct msghdr msg = {};
 	struct bio_vec bv;
@@ -527,11 +537,11 @@ static int rxperf_process_call(struct rxperf_call *call)
 	ret = rxrpc_kernel_send_data(rxperf_socket, call->rxcall, &msg,
 				     rxperf_notify_end_reply_tx);
 	if (ret == 0)
-		return 0;
+		return;
+
 send_error:
 	rxrpc_kernel_abort_call(rxperf_socket, call->rxcall, RXGEN_SS_MARSHAL,
 				ret, rxperf_abort_send_error);
-	return ret;
 }
 
 /*
@@ -684,4 +694,3 @@ static void __exit rxperf_exit(void)
 	rcu_barrier();
 }
 module_exit(rxperf_exit);
-
diff --git a/net/rxrpc/sendmsg.c b/net/rxrpc/sendmsg.c
index 312be27ca75b..80c6afc64787 100644
--- a/net/rxrpc/sendmsg.c
+++ b/net/rxrpc/sendmsg.c
@@ -324,19 +324,10 @@ static int rxrpc_send_data(struct rxrpc_sock *rx,
 	__releases(&call->user_mutex)
 {
 	struct sock *sk = &rx->sk;
-	enum rxrpc_call_state state;
 	long timeo;
 	bool more = msg->msg_flags & MSG_MORE;
 	int ret, copied = 0;
 
-	if (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);
-		ret = -EPROTO;
-		goto out_unlock;
-	}
-
 	timeo = sock_sndtimeo(sk, msg->msg_flags & MSG_DONTWAIT);
 
 	ret = rxrpc_wait_to_be_connected(call, &timeo);
@@ -355,21 +346,31 @@ static int rxrpc_send_data(struct rxrpc_sock *rx,
 reload:
 	ret = -EPIPE;
 	if (sk->sk_shutdown & SEND_SHUTDOWN)
-		goto maybe_error;
-	state = rxrpc_call_state(call);
-	ret = -ESHUTDOWN;
-	if (state >= RXRPC_CALL_COMPLETE)
-		goto maybe_error;
-	ret = -EPROTO;
-	if (state != RXRPC_CALL_CLIENT_PRE_SEND &&
-	    state != RXRPC_CALL_CLIENT_SEND_REQUEST &&
-	    state != RXRPC_CALL_SERVER_ACK_REQUEST &&
-	    state != RXRPC_CALL_SERVER_SEND_REPLY) {
-		/* Request phase complete for this client call */
+		goto out_unlock;
+
+	switch (rxrpc_call_state(call)) {
+	case RXRPC_CALL_CLIENT_PRE_SEND:
+	case RXRPC_CALL_CLIENT_SEND_REQUEST:
+	case RXRPC_CALL_SERVER_ACK_REQUEST:
+	case RXRPC_CALL_SERVER_SEND_REPLY:
+		break;
+	case RXRPC_CALL_COMPLETE:
+		ret = -ESHUTDOWN;
+		goto out_unlock;
+	default:
+		ret = -EPROTO;
 		trace_rxrpc_abort(call->debug_id, rxrpc_sendmsg_late_send,
 				  call->cid, call->call_id, call->rx_consumed,
 				  0, -EPROTO);
-		goto maybe_error;
+		goto out_unlock;
+	}
+
+	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);
+		ret = -EPROTO;
+		goto out_unlock;
 	}
 
 	ret = -EMSGSIZE;
@@ -435,8 +436,9 @@ static int rxrpc_send_data(struct rxrpc_sock *rx,
 
 		/* check for the far side aborting the call or a network error
 		 * occurring */
+		ret = -ESHUTDOWN;
 		if (rxrpc_call_is_complete(call))
-			goto call_terminated;
+			goto out_unlock;
 
 		/* add the packet to the send queue if it's now full */
 		if (!txb->space ||
@@ -449,31 +451,71 @@ static int rxrpc_send_data(struct rxrpc_sock *rx,
 				goto out_unlock;
 			rxrpc_queue_packet(rx, call, txb, notify_end_tx);
 			call->tx_pending = NULL;
+
+			/* At this point, if that was the last packet, it may
+			 * have been transmitted and the reply (client call) or
+			 * final ACK (service call) may have been received,
+			 * completing the call.
+			 */
 		}
 	} while (len > 0 && msg_data_left(msg) > 0);
 
-success:
+	/* Don't check for call completeness here, but leave that to recvmsg or
+	 * a further call to sendmsg().
+	 */
 	ret = copied;
-	if (rxrpc_call_is_complete(call) &&
-	    call->error < 0)
-		ret = call->error;
 out_unlock:
 	mutex_unlock(&call->user_mutex);
+out:
+
+	/* The return value is a bit complicated as we want to avoid returning
+	 * an error if we have queued the final packet.  In descending order of
+	 * preference:
+	 *
+	 * (1) If the send side of the socket is shut down, -EPIPE.
+	 *
+	 * (2) If the call has terminated early, likely due to an external
+	 *     event such as being remotely aborted: -ESHUTDOWN.
+	 *
+	 * (3) If the call is in the wrong state to transmit: -EPROTO.
+	 *
+	 * (4) If another sendmsg() has already queued the last packet: -EPROTO.
+	 *
+	 * (5) 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
+	 *     (which will be greater than zero).
+	 *
+	 * (7) 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.
+	 */
 	_leave(" = %d", ret);
 	return ret;
 
-call_terminated:
-	ret = call->error;
-	goto out_unlock;
-
 maybe_error:
-	if (copied)
-		goto success;
+	if (copied) {
+		if (test_bit(RXRPC_CALL_TX_NO_MORE, &call->flags)) {
+			/* If we've get here, we must have slept waiting for space and .
+			 */
+			ret = copied;
+			goto out_unlock;
+		}
+		if (rxrpc_call_is_complete(call)) {
+			ret = -ESHUTDOWN;
+			goto out_unlock;
+		}
+		ret = copied;
+	}
 	goto out_unlock;
 
 efault:
 	ret = -EFAULT;
-	goto out_unlock;
+	goto maybe_error;
 
 wait_for_space:
 	ret = -EAGAIN;
@@ -496,7 +538,9 @@ static int rxrpc_send_data(struct rxrpc_sock *rx,
 	goto reload;
 out_nolock:
 	_leave(" = %d [intr]", ret);
-	return copied ?: ret;
+	if (copied)
+		ret = copied;
+	goto out;
 }
 
 /*
@@ -818,8 +862,6 @@ int rxrpc_kernel_send_data(struct socket *sock, struct rxrpc_call *call,
 
 		ret = rxrpc_send_data(rxrpc_sk(sock->sk), call, msg,
 				      msg_data_left(msg), notify_end_tx);
-		if (ret == -ESHUTDOWN)
-			ret = call->error;
 		if (ret < 0)
 			break;
 		if (msg_data_left(msg) == 0) {

Re: [PATCH net v11 08/17] rxrpc: Fix error handling in rxrpc_send_data()

[netdev-bot+sashiko]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The fix leaves the new -ESHUTDOWN contract incomplete, and this
  breaks AFS abort handling.
- [Medium] At this commit, the new SV_AWAIT_ACK handling in
  rxperf_deliver_to_call() treats a return of 1 from…
- [Medium] rxperf_process_call() now returns void, and
  rxperf_deliver_to_call() returns right after calling it (`case 0:…
- [Low] In rxrpc_send_data(), the new `test_bit(RXRPC_CALL_TX_NO_MORE)`
  branch under maybe_error is dead code, and its comment stops mid-…

--

rxrpc: Fix error handling in rxrpc_send_data()

This commit changes rxrpc_send_data() so that it doesn't return an error
after it has queued the last packet of a call. It also makes it return
-ESHUTDOWN if the call completes before the final packet is queued. The
-ESHUTDOWN to call->error translation is removed from
rxrpc_kernel_send_data(), and afs_make_call() and rxperf are changed to
match.

> diff --git a/fs/afs/rxrpc.c b/fs/afs/rxrpc.c
> index c38458157eccf..0b9027549580e 100644
> --- a/fs/afs/rxrpc.c
> +++ b/fs/afs/rxrpc.c
> @@ -448,13 +448,14 @@ void afs_make_call(struct afs_call *call, gfp_t gfp)
>  		return;
>  	}
>  
> -	if (ret == -ECONNABORTED) {
> +	if (ret == -ESHUTDOWN) {
>  		len = 0;
>  		iov_iter_kvec(&msg.msg_iter, ITER_DEST, NULL, 0, 0);
> -		rxrpc_kernel_recv_data(call->net->socket, rxcall,
> -				       &msg.msg_iter, &len, false,
> -				       &call->abort_code, &call->service_id);
> -		call->responded = true;
> +		ret = rxrpc_kernel_recv_data(call->net->socket, rxcall,
> +					     &msg.msg_iter, &len, false,
> +					     &call->abort_code, &call->service_id);
> +		if (ret == -ECONNABORTED)
> +			call->responded = true;
>  	}

[Severity: High]
Can rxrpc_kernel_send_data() still return -ECONNABORTED for a call that
the peer has aborted? The translation from -ESHUTDOWN to call->error is
gone. However, some paths in rxrpc_send_data() still return the raw
call->error and never reach the new RXRPC_CALL_COMPLETE case at reload.

rxrpc_wait_to_be_connected() still does:

no_wait:
	if (ret == 0 && rxrpc_call_is_complete(call))
		ret = call->error;

rxrpc_send_data() then goes straight to out_unlock with that value.

The rxrpc_wait_for_tx_window_*() helpers also still do:

		if (rxrpc_call_is_complete(call))
			return call->error;

That value goes to out_nolock. out_nolock returns copied if it is
nonzero, and the raw error otherwise.

Here is one example. The client is blocked in
rxrpc_wait_for_tx_window_nonintr() during a StoreData, and the fileserver
aborts the call with VBUSY or VOFFLINE:

- out_nolock returns copied (> 0).
- rxrpc_kernel_send_data() loops.
- The next rxrpc_send_data() gets -ECONNABORTED from
  rxrpc_wait_to_be_connected().

In that case the check above fails and rxrpc_kernel_recv_data() is not
called. So call->abort_code stays 0 and call->responded stays false.

afs_select_fileserver() then sees -ECONNABORTED with an abort code of 0
and takes the default case. Doesn't that lose the VBUSY/VOFFLINE retry,
the VMOVED handling and the EDQUOT/ENOSPC translation?

Before this patch, those paths all returned -ECONNABORTED, which the old
check here handled.

For userspace AF_RXRPC, an early termination can now show up three ways:
-ESHUTDOWN, the raw call->error, or a positive count from out_nolock.
That doesn't seem to match the new return-value comment in
rxrpc_send_data() or the commit message:

  "return -ESHUTDOWN if the call is now marked complete (e.g. it got
  aborted by the peer)"

Should these helpers return -ESHUTDOWN for a completed call instead, or
should rxrpc_kernel_send_data() keep the translation?

[ ... ]

> diff --git a/net/rxrpc/rxperf.c b/net/rxrpc/rxperf.c
> index 823eedc5d16f9..5042e8bfca550 100644
> --- a/net/rxrpc/rxperf.c
> +++ b/net/rxrpc/rxperf.c

[ ... ]

> @@ -293,18 +293,28 @@ static void rxperf_deliver_to_call(struct work_struct *work)
>  	       state == RXPERF_CALL_SV_AWAIT_ACK
>  	       ) {
>  		if (state == RXPERF_CALL_SV_AWAIT_ACK) {
> -			if (!rxrpc_kernel_check_life(rxperf_socket, call->rxcall))
> +			size_t len = 0;
> +			iov_iter_kvec(&call->iter, ITER_DEST, NULL, 0, 0);
> +			ret = rxrpc_kernel_recv_data(rxperf_socket,
> +						     call->rxcall, &call->iter,
> +						     &len, false, &remote_abort,
> +						     &call->service_id);
> +
> +			if (ret == -EINPROGRESS || ret == -EAGAIN)
> +				return;
> +			if (ret < 0 || ret == 1) {
> +				if (ret == 1)
> +					ret = 0;
>  				goto call_complete;
> +			}

[Severity: Medium]
Does a return of 1 here really mean the call has completed? At this
commit, rxrpc_kernel_recv_data() returns 1 from read_phase_complete once
the request data has been fully read, which can be before the final ACK
arrives:

read_phase_complete:
	ret = 1;

Suppose a requeued work item runs in RXPERF_CALL_SV_AWAIT_ACK while the
reply is still in flight. Wouldn't call_complete then call
rxrpc_kernel_shutdown_call(), rxrpc_kernel_put_call() and kfree(call) on
a call that is still live?

Also, rxrpc_kernel_recv_data() doesn't seem to return -EINPROGRESS, so
that test never matches.

A later commit in the series, "rxrpc: Fix return in rxrpc_recvmsg_data()
for service calls", fixes this. It makes rxrpc_kernel_recv_data() return
2 for a service call that hasn't completed, and changes this test to
check for 2. Could that change be folded in or moved earlier, so that
this commit doesn't have the problem on its own?

>  			return;
>  		}
>  
>  		ret = call->deliver(call);
> -		if (ret == 0)
> -			ret = rxperf_process_call(call);
> -
>  		switch (ret) {
>  		case 0:
> -			continue;
> +			rxperf_process_call(call);
> +			return;

[Severity: Medium]
Can this leak the rxperf_call and its rxrpc_call reference if sending the
reply fails? Before this patch, a send error went to the default case and
then to call_complete. Now teardown relies on a later notify_rx
requeueing the work.

rxperf_close_socket() does:

	kernel_listen(rxperf_socket, 0);
	kernel_sock_shutdown(rxperf_socket, SHUT_RDWR);
	flush_workqueue(rxperf_workqueue);
	sock_release(rxperf_socket);

Suppose a work item is inside rxperf_process_call(), sending a
multi-page reply, when the shutdown happens. The next chunk hits
SEND_SHUTDOWN at reload in rxrpc_send_data() and gets -EPIPE.
rxperf_process_call() then aborts the call and returns.

The completion then goes through __rxrpc_notify_socket(), which skips
notify_rx once the socket is closed:

	if (rx && sk->sk_state < RXRPC_CLOSE) {
		if (call->notify_rx) {

So the work is never requeued, and neither kfree(call) nor
rxrpc_kernel_put_call() runs. Wouldn't rxrpc_destroy_all_calls() then
block in wait_var_event() on rxnet->nr_calls when rxrpc is unloaded?

[ ... ]

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

[ ... ]

> @@ -449,31 +451,71 @@ static int rxrpc_send_data(struct rxrpc_sock *rx,

[ ... ]

>  maybe_error:
> -	if (copied)
> -		goto success;
> +	if (copied) {
> +		if (test_bit(RXRPC_CALL_TX_NO_MORE, &call->flags)) {
> +			/* If we've get here, we must have slept waiting for space and .
> +			 */
> +			ret = copied;
> +			goto out_unlock;
> +		}

[Severity: Low]
This isn't a bug, but the comment stops mid-sentence ("waiting for space
and ."), and "we've get" should probably be "we've got".

Can this branch be reached at all? Only rxrpc_queue_packet() sets
RXRPC_CALL_TX_NO_MORE, and only for the last packet. That happens only
when len == 0 && !more, which ends the loop at "ret = copied". After a
sleep, reload rejects TX_NO_MORE with -EPROTO before anything can jump
to maybe_error.

[ ... ]

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

Re: [PATCH net v11 08/17] rxrpc: Fix error handling in rxrpc_send_data()

[Jakub Kicinski]

On Sun, 27 Sep 2026 14:59:52 +0000 netdev-bot+sashiko@kernel.org wrote:
> - [High] The fix leaves the new -ESHUTDOWN contract incomplete, and this
>   breaks AFS abort handling.

This one looks like the main blocker for the series, right?

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

[David Howells]

In rxrpc_send_data(), if ->secure_packet() returns an error, the code
currently just jumps to out: and returns the error to the app on the
assumption that any error returned by this is automatically fatal for the
call, and may even have corrupted the transmission queue - but leaving it
to userspace to deal with.  Nothing stops the application from retrying the
sendmsg(), which will try to encrypt the buffer again, and might succeed
with a corrupt buffer.

Fix rxrpc_send_data() in the following ways:

 (1) If -ENOMEM is returned, assume we never got as far as the encryption
     and that the operation is retryable.  In which case, jump to
     maybe_error_rewind and, if we've copied all remaining data into the
     last packet, remove some of the bytes from it that we just added so
     that we don't tell the caller that we've completed the transmission
     phase.  The iterator is also correspondingly rewound.

 (2) If any other error occurs, set the TX_ERROR flag on the call and
     return that error directly; on all subsequent attempts to add data to
     the call, return -EIO.  The app must then abort the call to get rid of
     it (this allows the app to choose the abort code to use).

Fixes: 17926a79320a ("[AF_RXRPC]: Provide secure RxRPC sockets for use by userspace and kernel both")
Closes: https://sashiko.dev/#/patchset/20260702144919.172295-1-dhowells%40redhat.com
Signed-off-by: David Howells <dhowells@redhat.com>
cc: Marc Dionne <marc.dionne@auristor.com>
cc: Eric Dumazet <edumazet@google.com>
cc: "David S. Miller" <davem@davemloft.net>
cc: Jakub Kicinski <kuba@kernel.org>
cc: Paolo Abeni <pabeni@redhat.com>
cc: Simon Horman <horms@kernel.org>
cc: linux-afs@lists.infradead.org
cc: stable@vger.kernel.org
---
 Documentation/networking/rxrpc.rst |  6 ++-
 include/trace/events/rxrpc.h       |  1 +
 net/rxrpc/ar-internal.h            |  1 +
 net/rxrpc/sendmsg.c                | 68 +++++++++++++++++++++++++-----
 4 files changed, 63 insertions(+), 13 deletions(-)

diff --git a/Documentation/networking/rxrpc.rst b/Documentation/networking/rxrpc.rst
index 01ad12fdf305..9b7eeae84148 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
diff --git a/include/trace/events/rxrpc.h b/include/trace/events/rxrpc.h
index 56dc9b614071..a5c92592d8f9 100644
--- a/include/trace/events/rxrpc.h
+++ b/include/trace/events/rxrpc.h
@@ -148,6 +148,7 @@
 	EM(rxrpc_eproto_wrong_security,		"wrong-sec")		\
 	EM(rxrpc_recvmsg_excess_data,		"recvmsg-excess")	\
 	EM(rxrpc_recvmsg_short_data,		"recvmsg-short")	\
+	EM(rxrpc_sendmsg_tx_error,		"tx-error")		\
 	E_(rxrpc_sendmsg_late_send,		"sendmsg-late")
 
 #define rxrpc_call_poke_traces \
diff --git a/net/rxrpc/ar-internal.h b/net/rxrpc/ar-internal.h
index 865f05fe37ab..a6f830c1621f 100644
--- a/net/rxrpc/ar-internal.h
+++ b/net/rxrpc/ar-internal.h
@@ -642,6 +642,7 @@ enum rxrpc_call_flag {
 	RXRPC_CALL_TX_LAST,		/* Last packet in Tx buffer (at rxtx_top) */
 	RXRPC_CALL_TX_ALL_ACKED,	/* Last packet has been hard-acked */
 	RXRPC_CALL_TX_NO_MORE,		/* No more data to transmit (MSG_MORE deasserted) */
+	RXRPC_CALL_TX_ERROR,		/* Terminal error; call needs abort */
 	RXRPC_CALL_SEND_PING,		/* A ping will need to be sent */
 	RXRPC_CALL_RETRANS_TIMEOUT,	/* Retransmission due to timeout occurred */
 	RXRPC_CALL_BEGAN_RX_TIMER,	/* We began the expect_rx_by timer */
diff --git a/net/rxrpc/sendmsg.c b/net/rxrpc/sendmsg.c
index 80c6afc64787..e3b6347888bf 100644
--- a/net/rxrpc/sendmsg.c
+++ b/net/rxrpc/sendmsg.c
@@ -324,6 +324,7 @@ static int rxrpc_send_data(struct rxrpc_sock *rx,
 	__releases(&call->user_mutex)
 {
 	struct sock *sk = &rx->sk;
+	unsigned int rewind_by = 0;
 	long timeo;
 	bool more = msg->msg_flags & MSG_MORE;
 	int ret, copied = 0;
@@ -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;
+	}
 
 	ret = -EMSGSIZE;
 	if (call->tx_total_len != -1) {
@@ -425,6 +433,7 @@ static int rxrpc_send_data(struct rxrpc_sock *rx,
 						 copy, &msg->msg_iter))
 				goto efault;
 			_debug("added");
+			rewind_by = copy;
 			txb->space -= copy;
 			txb->len += copy;
 			txb->offset += copy;
@@ -443,14 +452,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)) {
-			if (len == 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) {
+				/* 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_unlock;
+			}
+
+			if (len == 0 && !more)
+				txb->flags |= RXRPC_LAST_PACKET;
 			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
@@ -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.
 	 */
 	_leave(" = %d", ret);
 	return ret;
 
+maybe_error_rewind:
+	/* If we got a retryable error after copying all the supplied data into
+	 * the last packet, we need to rewind as much as we can so the caller
+	 * knows they need to retry the sendmsg.
+	 */
+	if (rewind_by && !more && !len) {
+		struct rxrpc_txbuf *txb = call->tx_pending;
+
+		txb->space  += rewind_by;
+		txb->len    -= rewind_by;
+		txb->offset -= rewind_by;
+		copied      -= rewind_by;
+		if (call->tx_total_len != -1)
+			call->tx_total_len += rewind_by;
+		iov_iter_revert(&msg->msg_iter, rewind_by);
+	}
 maybe_error:
 	if (copied) {
 		if (test_bit(RXRPC_CALL_TX_NO_MORE, &call->flags)) {

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

[netdev-bot+sashiko]

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

[PATCH net v11 10/17] rxrpc: Fix return in rxrpc_recvmsg_data() for service calls

[David Howells]

When rxrpc_recvmsg_data() gets called on a service call that has received
all of the request, RXRPC_CALL_RECVMSG_READ_ALL has been set, and this
causes rxrpc_recvmsg_data() to jump straight out, indicating the end of the
call (ie. rxrpc_kernel_recv_data() returns 1) without waiting for the call
to be processed or the reply to be transmitted.

Link: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260914151340.3227501-1-dhowells%40redhat.com
Signed-off-by: David Howells <dhowells@redhat.com>
cc: Marc Dionne <marc.dionne@auristor.com>
cc: Jeffrey Altman <jaltman@auristor.com>
cc: Eric Dumazet <edumazet@google.com>
cc: "David S. Miller" <davem@davemloft.net>
cc: Jakub Kicinski <kuba@kernel.org>
cc: Paolo Abeni <pabeni@redhat.com>
cc: Simon Horman <horms@kernel.org>
cc: linux-afs@lists.infradead.org
cc: stable@vger.kernel.org
---
 fs/afs/rxrpc.c      |  4 ++--
 net/rxrpc/recvmsg.c | 13 ++++++++++---
 net/rxrpc/rxperf.c  |  4 ++--
 3 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/fs/afs/rxrpc.c b/fs/afs/rxrpc.c
index 0b9027549580..8e7a83a95ac9 100644
--- a/fs/afs/rxrpc.c
+++ b/fs/afs/rxrpc.c
@@ -542,7 +542,7 @@ void afs_deliver_to_call(struct afs_call *call)
 						     &call->service_id);
 			trace_afs_receive_data(call, &call->def_iter, false, ret);
 
-			if (ret == -EINPROGRESS || ret == -EAGAIN)
+			if (ret == -EAGAIN || ret == 2)
 				return;
 			if (ret < 0 || ret == 1) {
 				if (ret == 1)
@@ -932,7 +932,7 @@ int afs_extract_data(struct afs_call *call, bool want_more)
 		return ret;
 
 	state = READ_ONCE(call->state);
-	if (ret == 1) {
+	if (ret == 1 || ret == 2) {
 		switch (state) {
 		case AFS_CALL_CL_AWAIT_REPLY:
 			afs_set_call_state(call, state, AFS_CALL_CL_PROC_REPLY);
diff --git a/net/rxrpc/recvmsg.c b/net/rxrpc/recvmsg.c
index efcba4b2e74f..716f7f47d7e4 100644
--- a/net/rxrpc/recvmsg.c
+++ b/net/rxrpc/recvmsg.c
@@ -637,9 +637,11 @@ int rxrpc_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
  * Note that we may return %-EAGAIN to drain empty packets at the end
  * of the data, even if we've already copied over the requested data.
  *
- * Return: %0 if got what was asked for and there's more available, %1
- * if we got what was asked for and we're at the end of the data and
- * %-EAGAIN if we need more data.
+ * Return: %0 if got what was asked for and there's more available, %1 if we
+ * got what was asked for and we're at the end of the call, %2 if a service
+ * call received all of the request but is still in progress and %-EAGAIN if we
+ * need more data.  A variety of other errors can be returned if the call
+ * completed with failure.
  */
 int rxrpc_kernel_recv_data(struct socket *sock, struct rxrpc_call *call,
 			   struct iov_iter *iter, size_t *_len,
@@ -678,6 +680,11 @@ int rxrpc_kernel_recv_data(struct socket *sock, struct rxrpc_call *call,
 
 read_phase_complete:
 	ret = 1;
+	if (rxrpc_is_service_call(call)) {
+		if (rxrpc_call_is_complete(call))
+			goto call_failed;
+		ret = 2;
+	}
 out:
 	if (_service)
 		*_service = call->dest_srx.srx_service;
diff --git a/net/rxrpc/rxperf.c b/net/rxrpc/rxperf.c
index 5042e8bfca55..6ccfd40b5388 100644
--- a/net/rxrpc/rxperf.c
+++ b/net/rxrpc/rxperf.c
@@ -300,7 +300,7 @@ static void rxperf_deliver_to_call(struct work_struct *work)
 						     &len, false, &remote_abort,
 						     &call->service_id);
 
-			if (ret == -EINPROGRESS || ret == -EAGAIN)
+			if (ret == -EAGAIN || ret == 2)
 				return;
 			if (ret < 0 || ret == 1) {
 				if (ret == 1)
@@ -379,7 +379,7 @@ static int rxperf_extract_data(struct rxperf_call *call, bool want_more)
 	if (ret == 0 || ret == -EAGAIN)
 		return ret;
 
-	if (ret == 1) {
+	if (ret == 1 || ret == 2) {
 		switch (call->state) {
 		case RXPERF_CALL_SV_AWAIT_REQUEST:
 			rxperf_set_call_state(call, RXPERF_CALL_SV_REPLYING);

Re: [PATCH net v11 10/17] rxrpc: Fix return in rxrpc_recvmsg_data() for service calls

[David Howells]

Fixes: d001648ec7cf ("rxrpc: Don't expose skbs to in-kernel users [ver #2]")

Re: [PATCH net v11 10/17] rxrpc: Fix return in rxrpc_recvmsg_data() for service calls

[netdev-bot+sashiko]

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 commit is Cc'd to stable@vger.kernel.org but has no Fixes:
  tag.
- [Low] The patch adds a new normal return value, 2, to the exported
  kernel API rxrpc_kernel_recv_data(), but…

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but rxperf can
  leak rxperf_call objects, and the rxrpc_call references they hold,…

--

rxrpc: Fix return in rxrpc_recvmsg_data() for service calls

This patch makes rxrpc_kernel_recv_data() return a new value, 2, when a
service call has received all of its request but has not finished yet. It
no longer returns 1 in that case, which made the call look complete. The
AFS and rxperf callers are updated to handle the new return value.

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

> cc: linux-afs@lists.infradead.org
> cc: stable@vger.kernel.org

[Severity: Low]
Should this have a Fixes: tag? It is Cc'd to stable, but nothing says
which trees need it.

The behaviour being fixed looks like it came from:

Fixes: 93368b6bd58a ("rxrpc: Move call state changes from recvmsg to I/O thread")

That commit dropped the state switch in rxrpc_kernel_recv_data(). The
switch had a default case that returned -EINPROGRESS for service calls
that were still processing, replying or waiting for the final ACK. In its
place, it added the RXRPC_CALL_RECVMSG_READ_ALL early return in
rxrpc_recvmsg_data(), which returns 1:

    if (test_bit(RXRPC_CALL_RECVMSG_READ_ALL, &call->flags)) {
        seq = call->ackr_window - 1;
        ret = 1;
        goto done;
    }

Since then, the ret == -EINPROGRESS checks in afs_deliver_to_call() and
rxperf_deliver_to_call() have been dead code.

Trees older than v6.2 still use the -EINPROGRESS contract. Without the
tag, stable maintainers could backport this to trees that don't need it.

> diff --git a/net/rxrpc/recvmsg.c b/net/rxrpc/recvmsg.c
> index efcba4b2e74f0..716f7f47d7e46 100644
> --- a/net/rxrpc/recvmsg.c
> +++ b/net/rxrpc/recvmsg.c
> @@ -637,9 +637,11 @@ int rxrpc_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
>   * Note that we may return %-EAGAIN to drain empty packets at the end
>   * of the data, even if we've already copied over the requested data.
>   *
> - * Return: %0 if got what was asked for and there's more available, %1
> - * if we got what was asked for and we're at the end of the data and
> - * %-EAGAIN if we need more data.
> + * Return: %0 if got what was asked for and there's more available, %1 if we
> + * got what was asked for and we're at the end of the call, %2 if a service
> + * call received all of the request but is still in progress and %-EAGAIN if we
> + * need more data.  A variety of other errors can be returned if the call
> + * completed with failure.
>   */
>  int rxrpc_kernel_recv_data(struct socket *sock, struct rxrpc_call *call,
>  			   struct iov_iter *iter, size_t *_len,
> @@ -678,6 +680,11 @@ int rxrpc_kernel_recv_data(struct socket *sock, struct rxrpc_call *call,
>  
>  read_phase_complete:
>  	ret = 1;
> +	if (rxrpc_is_service_call(call)) {
> +		if (rxrpc_call_is_complete(call))
> +			goto call_failed;
> +		ret = 2;
> +	}

[Severity: Low]
Should Documentation/networking/rxrpc.rst be updated as well? Its section
on rxrpc_kernel_recv_data() still lists only three normal returns:

      There are three normal returns: 0 if the buffer was filled and want_more
      was true; 1 if the buffer was filled, the last DATA packet has been
      emptied and want_more was false; and -EAGAIN if the function needs to be
      called again.

With this change, a service call in that situation gets 2, or goes to
call_failed if the call has already completed. It no longer gets 1.

The kernel-doc above now lists %2, so the tree describes two different
return contracts for this exported function. A kernel service written from
rxrpc.rst would not know to handle 2.

>  out:
>  	if (_service)
>  		*_service = call->dest_srx.srx_service;
> diff --git a/net/rxrpc/rxperf.c b/net/rxrpc/rxperf.c
> index 5042e8bfca550..6ccfd40b5388c 100644
> --- a/net/rxrpc/rxperf.c
> +++ b/net/rxrpc/rxperf.c
> @@ -300,7 +300,7 @@ static void rxperf_deliver_to_call(struct work_struct *work)
>  						     &len, false, &remote_abort,
>  						     &call->service_id);
>  
> -			if (ret == -EINPROGRESS || ret == -EAGAIN)
> +			if (ret == -EAGAIN || ret == 2)
>  				return;

[Severity: Medium]
This is a pre-existing issue that this patch did not introduce, but can
rxperf leak rxperf_call objects, and the rxrpc_call refs they hold, when
the module is unloaded?

The only place an rxperf_call is freed is the call_complete path in
rxperf_deliver_to_call(). That path runs only when a notify_rx callback
queues the work item.

The teardown in rxperf_close_socket() looks like this:

    kernel_listen(rxperf_socket, 0);
    kernel_sock_shutdown(rxperf_socket, SHUT_RDWR);
    flush_workqueue(rxperf_workqueue);
    sock_release(rxperf_socket);

kernel_sock_shutdown() goes into rxrpc_shutdown(), which sets sk_state to
RXRPC_CLOSE. After that, sock_release()->rxrpc_release_calls_on_socket()
aborts any remaining calls with rxrpc_propose_abort(). The completion
notification is then dropped by this check in rxrpc_notify_socket():

    if (rx && sk->sk_state < RXRPC_CLOSE) {
        if (call->notify_rx) {

rxperf does not keep a list of outstanding calls. So a call still in
flight at unload, such as a partly received request or a call in
SV_AWAIT_ACK, never gets its rxperf_call freed or its rxcall ref dropped.

This patch returns early on ret == 2 in SV_AWAIT_ACK. Such calls now
reliably stay alive until the final ACK or completion arrives, so the
window for this is somewhat longer. The teardown gap itself is older than
this patch.

[ ... ]

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

[PATCH net v11 13/17] rxrpc: Fix RxGK key parser to check enctype is supported

[David Howells]

Fix the parser of RxGK keys supplied by userspace to check that the
specified encryption type is supported and check the key length.  Further,
since the checking function isn't necessarily available in CONFIG_RXGK=n,
make the RxGK key wrangling bits conditional.

Also only account the a token to the key's quota if that token is used.

Fixes: 0ca100ff4df6 ("rxrpc: Add YFS RxGK (GSSAPI) security class")
Link: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260824091645.415423-1-dhowells%40redhat.com
Signed-off-by: David Howells <dhowells@redhat.com>
cc: Marc Dionne <marc.dionne@auristor.com>
cc: Eric Dumazet <edumazet@google.com>
cc: "David S. Miller" <davem@davemloft.net>
cc: Jakub Kicinski <kuba@kernel.org>
cc: Paolo Abeni <pabeni@redhat.com>
cc: Simon Horman <horms@kernel.org>
cc: linux-afs@lists.infradead.org
cc: stable@vger.kernel.org
---
 net/rxrpc/key.c | 41 +++++++++++++++++++++++++++++++----------
 1 file changed, 31 insertions(+), 10 deletions(-)

diff --git a/net/rxrpc/key.c b/net/rxrpc/key.c
index cbd26da44951..904da3fe7e47 100644
--- a/net/rxrpc/key.c
+++ b/net/rxrpc/key.c
@@ -71,14 +71,11 @@ static int rxrpc_preparse_xdr_rxkad(struct key_preparsed_payload *prep,
 	if (toklen < 8 * 4 + tktlen)
 		return -EKEYREJECTED;
 
-	plen = sizeof(*token) + sizeof(*token->kad) + tktlen;
-	prep->quotalen += datalen + plen;
-
-	plen -= sizeof(*token);
 	token = kzalloc_obj(*token);
 	if (!token)
 		return -ENOMEM;
 
+	plen = sizeof(*token->kad) + tktlen;
 	token->kad = kzalloc(plen, GFP_KERNEL);
 	if (!token->kad) {
 		kfree(token);
@@ -112,6 +109,8 @@ static int rxrpc_preparse_xdr_rxkad(struct key_preparsed_payload *prep,
 		       token->kad->ticket[4], token->kad->ticket[5],
 		       token->kad->ticket[6], token->kad->ticket[7]);
 
+	prep->quotalen += sizeof(*token) + datalen + plen;
+
 	/* count the number of tokens attached */
 	prep->payload.data[1] = (void *)((unsigned long)prep->payload.data[1] + 1);
 
@@ -129,6 +128,7 @@ static int rxrpc_preparse_xdr_rxkad(struct key_preparsed_payload *prep,
 	return 0;
 }
 
+#ifdef CONFIG_RXGK
 static u64 xdr_dec64(const __be32 *xdr)
 {
 	return (u64)ntohl(xdr[0]) << 32 | (u64)ntohl(xdr[1]);
@@ -166,12 +166,13 @@ static int rxrpc_preparse_xdr_yfs_rxgk(struct key_preparsed_payload *prep,
 				       size_t datalen,
 				       const __be32 *xdr, unsigned int toklen)
 {
+	const struct krb5_enctype *enc;
 	struct rxrpc_key_token *token, **pptoken;
 	time64_t expiry;
-	size_t plen;
 	const __be32 *ticket, *key;
 	s64 tmp;
 	size_t raw_keylen, raw_tktlen, keylen, tktlen;
+	int ret = -EKEYREJECTED;
 
 	_enter(",{%x,%x,%x,%x},%x",
 	       ntohl(xdr[0]), ntohl(xdr[1]), ntohl(xdr[2]), ntohl(xdr[3]),
@@ -202,10 +203,6 @@ static int rxrpc_preparse_xdr_yfs_rxgk(struct key_preparsed_payload *prep,
 		goto reject;
 	}
 
-	plen = sizeof(*token) + sizeof(*token->rxgk) + tktlen + keylen;
-	prep->quotalen += datalen + plen;
-
-	plen -= sizeof(*token);
 	token = kzalloc_obj(*token);
 	if (!token)
 		goto nomem;
@@ -229,6 +226,17 @@ static int rxrpc_preparse_xdr_yfs_rxgk(struct key_preparsed_payload *prep,
 	token->rxgk->key.data	= token->rxgk->_key;
 	token->rxgk->ticket.len = raw_tktlen;
 
+	/* Check the enctype is supported. */
+	enc = crypto_krb5_find_enctype(token->rxgk->enctype);
+	if (!enc) {
+		ret = -ENOPKG;
+		goto reject_token;
+	}
+	if (raw_keylen != enc->key_len) {
+		ret = -EKEYREJECTED;
+		goto reject_token;
+	}
+
 	if (token->rxgk->endtime != 0) {
 		expiry = rxrpc_s64_to_time64(token->rxgk->endtime);
 		if (expiry < 0)
@@ -257,6 +265,8 @@ static int rxrpc_preparse_xdr_yfs_rxgk(struct key_preparsed_payload *prep,
 	_debug("TICK: %*phN",
 	       min_t(u32, token->rxgk->ticket.len, 32), token->rxgk->ticket.data);
 
+	prep->quotalen += sizeof(*token) + datalen + tktlen + keylen;
+
 	/* count the number of tokens attached */
 	prep->payload.data[1] = (void *)((unsigned long)prep->payload.data[1] + 1);
 
@@ -280,12 +290,13 @@ static int rxrpc_preparse_xdr_yfs_rxgk(struct key_preparsed_payload *prep,
 	kfree(token->rxgk);
 	kfree(token);
 reject:
-	return -EKEYREJECTED;
+	return ret;
 expired:
 	kfree(token->rxgk);
 	kfree(token);
 	return -EKEYEXPIRED;
 }
+#endif /* CONFIG_RXGK */
 
 /*
  * attempt to parse the data as the XDR format
@@ -386,9 +397,11 @@ static int rxrpc_preparse_xdr(struct key_preparsed_payload *prep)
 		case RXRPC_SECURITY_RXKAD:
 			ret2 = rxrpc_preparse_xdr_rxkad(prep, datalen, token, toklen);
 			break;
+#ifdef CONFIG_RXGK
 		case RXRPC_SECURITY_YFS_RXGK:
 			ret2 = rxrpc_preparse_xdr_yfs_rxgk(prep, datalen, token, toklen);
 			break;
+#endif
 		default:
 			ret2 = -EPROTONOSUPPORT;
 			break;
@@ -556,10 +569,12 @@ static void rxrpc_free_token_list(struct rxrpc_key_token *token)
 		case RXRPC_SECURITY_RXKAD:
 			kfree(token->kad);
 			break;
+#ifdef CONFIG_RXGK
 		case RXRPC_SECURITY_YFS_RXGK:
 			kfree(token->rxgk->ticket.data);
 			kfree(token->rxgk);
 			break;
+#endif
 		default:
 			pr_err("Unknown token type %x on rxrpc key\n",
 			       token->security_index);
@@ -603,9 +618,11 @@ static void rxrpc_describe(const struct key *key, struct seq_file *m)
 		case RXRPC_SECURITY_RXKAD:
 			seq_puts(m, "ka");
 			break;
+#ifdef CONFIG_RXGK
 		case RXRPC_SECURITY_YFS_RXGK:
 			seq_puts(m, "ygk");
 			break;
+#endif
 		default: /* we have a ticket we can't encode */
 			seq_printf(m, "%u", token->security_index);
 			break;
@@ -770,12 +787,14 @@ static long rxrpc_read(const struct key *key,
 				toksize += RND(token->kad->ticket_len);
 			break;
 
+#ifdef CONFIG_RXGK
 		case RXRPC_SECURITY_YFS_RXGK:
 			toksize += 6 * 8 + 2 * 4;
 			if (!token->no_leak_key)
 				toksize += RND(token->rxgk->key.len);
 			toksize += RND(token->rxgk->ticket.len);
 			break;
+#endif
 
 		default: /* we have a ticket we can't encode */
 			pr_err("Unsupported key token type (%u)\n",
@@ -856,6 +875,7 @@ static long rxrpc_read(const struct key *key,
 				ENCODE_DATA(token->kad->ticket_len, token->kad->ticket);
 			break;
 
+#ifdef CONFIG_RXGK
 		case RXRPC_SECURITY_YFS_RXGK:
 			ENCODE64(token->rxgk->begintime);
 			ENCODE64(token->rxgk->endtime);
@@ -869,6 +889,7 @@ static long rxrpc_read(const struct key *key,
 				ENCODE_DATA(token->rxgk->key.len, token->rxgk->key.data);
 			ENCODE_DATA(token->rxgk->ticket.len, token->rxgk->ticket.data);
 			break;
+#endif
 
 		default:
 			pr_err("Unsupported key token type (%u)\n",

Re: [PATCH net v11 13/17] rxrpc: Fix RxGK key parser to check enctype is supported

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 2 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The patch rewrote the RxGK key quota charge in
  rxrpc_preparse_xdr_yfs_rxgk() (net/rxrpc/key.c) and dropped the size…
- [Medium] The new key-length check in rxrpc_preparse_xdr_yfs_rxgk()
  (net/rxrpc/key.c:235-238, `if (raw_keylen != enc->key_len) { ret =…

--

rxrpc: Fix RxGK key parser to check enctype is supported

The RxGK key parser now checks that the token's enctype is supported and
that the key length matches that enctype. The RxGK key handling is now
built only when CONFIG_RXGK is set. Quota is now charged in both the rxkad
and RxGK parsers only after a token is accepted.

Link: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260824091645.415423-1-dhowells%40redhat.com

> diff --git a/net/rxrpc/key.c b/net/rxrpc/key.c
> index cbd26da449510..904da3fe7e478 100644
> --- a/net/rxrpc/key.c
> +++ b/net/rxrpc/key.c

[ ... ]

> @@ -202,10 +203,6 @@ static int rxrpc_preparse_xdr_yfs_rxgk(struct key_preparsed_payload *prep,
>  		goto reject;
>  	}
>  
> -	plen = sizeof(*token) + sizeof(*token->rxgk) + tktlen + keylen;
> -	prep->quotalen += datalen + plen;
> -
> -	plen -= sizeof(*token);
>  	token = kzalloc_obj(*token);
>  	if (!token)
>  		goto nomem;
> @@ -229,6 +226,17 @@ static int rxrpc_preparse_xdr_yfs_rxgk(struct key_preparsed_payload *prep,
>  	token->rxgk->key.data	= token->rxgk->_key;
>  	token->rxgk->ticket.len = raw_tktlen;
>  
> +	/* Check the enctype is supported. */
> +	enc = crypto_krb5_find_enctype(token->rxgk->enctype);
> +	if (!enc) {
> +		ret = -ENOPKG;
> +		goto reject_token;
> +	}
> +	if (raw_keylen != enc->key_len) {
> +		ret = -EKEYREJECTED;
> +		goto reject_token;
> +	}

[Severity: Medium]
Does the server side handle this new -EKEYREJECTED return? The server
reaches this parser through this chain:

rxgk_verify_response()
  -> rxgk_extract_token()
    -> rxgk_yfs_decode_ticket()
      -> key_instantiate_and_link()
        -> rxrpc_preparse()
          -> rxrpc_preparse_xdr()
            -> rxrpc_preparse_xdr_yfs_rxgk()

rxgk_yfs_decode_ticket() compares klen with the ticket length but not with
the enctype. It returns the error from key_instantiate_and_link() unchanged.

The cant_get_token switch in rxgk_extract_token() has no case for
-EKEYREJECTED:

net/rxrpc/rxgk_app.c:rxgk_extract_token() {
    ...
cant_get_token:
	switch (ret) {
	case -ENOMEM:
		goto temporary_error;
	case -EINVAL:
		return rxrpc_abort_conn(conn, skb, RXGK_NOTAUTH, -EKEYREJECTED,
					rxgk_abort_resp_tok_internal_error);
	case -ENOPKG:
		return rxrpc_abort_conn(conn, skb, KRB5_PROG_KEYTYPE_NOSUPP,
					-EKEYREJECTED, rxgk_abort_resp_tok_nopkg);
	}

temporary_error:
	...
	return ret;
}

So the error is handled like a temporary error. rxgk_verify_response()
then does goto out, the RESPONSE is dropped, and the connection stays in
RXRPC_CONN_SERVICE_CHALLENGING.

Before this patch, the same ticket got through key instantiation and failed
later, in rxgk_generate_transport_key() -> rxgk_set_up_ciphers() ->
crypto_krb5_calc_PRFplus().

For the RFC3961 enctypes, a key length mismatch made rfc3961_calc_DK()
return -EINVAL, and the connection was aborted with RXGK_NOTAUTH. For the
other enctypes, the authenticator failed to decrypt and the connection was
aborted with RXGK_SEALEDINCON.

The ticket is decrypted with the server key, so only a buggy or
misconfigured token issuer can produce one like this.

Should the cant_get_token switch in rxgk_extract_token() get an
-EKEYREJECTED case? Then the client would get an abort instead of being
re-challenged until it times out.

> +
>  	if (token->rxgk->endtime != 0) {
>  		expiry = rxrpc_s64_to_time64(token->rxgk->endtime);
>  		if (expiry < 0)

[ ... ]

> @@ -257,6 +265,8 @@ static int rxrpc_preparse_xdr_yfs_rxgk(struct key_preparsed_payload *prep,
>  	_debug("TICK: %*phN",
>  	       min_t(u32, token->rxgk->ticket.len, 32), token->rxgk->ticket.data);
>  
> +	prep->quotalen += sizeof(*token) + datalen + tktlen + keylen;

[Severity: Medium]
Was sizeof(*token->rxgk) meant to be dropped from the quota charge in
rxrpc_preparse_xdr_yfs_rxgk()? The removed code included it:

	plen = sizeof(*token) + sizeof(*token->rxgk) + tktlen + keylen;

token->rxgk is still allocated separately, and that allocation includes the
fixed struct rxgk_key header:

	token->rxgk = kzalloc_flex(struct rxgk_key, _key, raw_keylen);

The rxkad hunk in this same patch still includes sizeof(*token->kad) in
plen, so the two parsers now charge differently.

The datalen term doesn't make up the difference. rxrpc_preparse_xdr()
requires the remaining datalen to be 0 before it calls the token parsers,
so datalen is always 0 here.

So each accepted RxGK token uses about 72 bytes more (on 64-bit) than
generic_key_instantiate() reserves against the user's key quota.

Should this be something like:

	prep->quotalen += sizeof(*token) + sizeof(*token->rxgk) + datalen +
			  tktlen + keylen;

> +
>  	/* count the number of tokens attached */
>  	prep->payload.data[1] = (void *)((unsigned long)prep->payload.data[1] + 1);

[ ... ]

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

[PATCH net v11 14/17] afs: Fix creation of RxGK CM channel token to have right size

[David Howells]

Fix afs_create_yfs_cm_token() so that it calculates the token size
correctly, remembering to add in the 4 bytes of the level.

Fixes: d98c317fd9aa ("afs: Use rxgk RESPONSE to pass token for callback channel")
Link: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260824091645.415423-1-dhowells%40redhat.com
Signed-off-by: David Howells <dhowells@redhat.com>
cc: Marc Dionne <marc.dionne@auristor.com>
cc: Eric Dumazet <edumazet@google.com>
cc: "David S. Miller" <davem@davemloft.net>
cc: Jakub Kicinski <kuba@kernel.org>
cc: Paolo Abeni <pabeni@redhat.com>
cc: Simon Horman <horms@kernel.org>
cc: linux-afs@lists.infradead.org
cc: stable@vger.kernel.org
---
 fs/afs/cm_security.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/afs/cm_security.c b/fs/afs/cm_security.c
index 103168c70dd4..5eeeef761cf3 100644
--- a/fs/afs/cm_security.c
+++ b/fs/afs/cm_security.c
@@ -235,7 +235,7 @@ static int afs_create_yfs_cm_token(struct sk_buff *challenge,
 	 *	struct RXGK_AuthName	identities<>;
 	 * };
 	 */
-	toksize = keysize + 8 + 4 + 4 + 8 + xdr_len_object(authsize);
+	toksize = keysize + 4 + 8 + 4 + 4 + 8 + xdr_len_object(authsize);
 
 	offset = 0;
 	encsize = crypto_krb5_how_much_buffer(token_krb5, KRB5_ENCRYPT_MODE, toksize, &offset);

[PATCH net v11 17/17] rxrpc: fix use-after-free in rxrpc_poke_conn()

[David Howells]

From: Seungwon Bae <qotmddnjs@ajou.ac.kr>

rxrpc_poke_conn() takes a reference on the connection with no liveness
check, unlike its sibling rxrpc_queue_conn() which gates on
atomic_read(&conn->active) >= 0.  The per-connection timer is armed with
no reference held for it, and rxrpc_put_connection() cancels it with a
non-synchronous timer_delete() only after the refcount reaches 0.
refcount_t saturates rather than resurrecting, so the connection can be
kfree()d while still linked in local->conn_attend_q (nothing in teardown
unlinks attend_link).  The rxrpc I/O thread then performs a UAF write
(list_del_init) plus UAF reads and indirect calls through conn->security.

Reproduced on a KASAN + PREEMPT kernel: 56 "refcount_t: addition on 0"
saturations at load, escalating to

  BUG: KASAN: slab-use-after-free in rxrpc_io_thread   Write of size 8

AF_RXRPC socket creation (rxrpc_create) has no capability check, so this
is reachable by an unprivileged user.

Guard rxrpc_poke_conn() with the same liveness/refcount check the sibling
rxrpc_queue_conn() uses before taking the poke reference, so a connection
past its last-active point is not poked/requeued after teardown began.

Verified before/after on KASAN+PREEMPT at equal timer volume: 56
saturations + 15 KASAN reports unpatched vs 0 and 0 patched.

Fixes: f2cce89a074e ("rxrpc: Implement a mechanism to send an event notification to a connection")
Signed-off-by: Seungwon Bae <qotmddnjs@ajou.ac.kr>
Signed-off-by: David Howells <dhowells@redhat.com>
cc: Marc Dionne <marc.dionne@auristor.com>
cc: Eric Dumazet <edumazet@google.com>
cc: "David S. Miller" <davem@davemloft.net>
cc: Jakub Kicinski <kuba@kernel.org>
cc: Paolo Abeni <pabeni@redhat.com>
cc: Simon Horman <horms@kernel.org>
cc: linux-afs@lists.infradead.org
cc: stable@vger.kernel.org
---
 net/rxrpc/conn_object.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/rxrpc/conn_object.c b/net/rxrpc/conn_object.c
index 0ece717db0f8..1be50e0c9cee 100644
--- a/net/rxrpc/conn_object.c
+++ b/net/rxrpc/conn_object.c
@@ -34,7 +34,10 @@ void rxrpc_poke_conn(struct rxrpc_connection *conn, enum rxrpc_conn_trace why)
 	spin_lock_irq(&local->lock);
 	busy = !list_empty(&conn->attend_link);
 	if (!busy) {
-		rxrpc_get_connection(conn, why);
+		if (!rxrpc_get_connection_maybe(conn, why)) {
+			spin_unlock_irq(&local->lock);
+			return;
+		}
 		list_add_tail(&conn->attend_link, &local->conn_attend_q);
 	}
 	spin_unlock_irq(&local->lock);

Re: [PATCH net v11 17/17] rxrpc: fix use-after-free in rxrpc_poke_conn()

[netdev-bot+sashiko]

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 commit message misdescribes the fix.

Pre-existing issues:
- [High] Nothing drains local->conn_attend_q after the I/O thread leaves
  its main loop, so a poke that lands in that window leaks the…

--

rxrpc: fix use-after-free in rxrpc_poke_conn()

This patch makes rxrpc_poke_conn() take its poke reference with
rxrpc_get_connection_maybe() in place of rxrpc_get_connection(). If the
refcount is already zero, the connection is no longer added to
local->conn_attend_q. This closes a use-after-free where the I/O thread
touched a freed connection that was still queued.

> Guard rxrpc_poke_conn() with the same liveness/refcount check the sibling
> rxrpc_queue_conn() uses before taking the poke reference, so a connection
> past its last-active point is not poked/requeued after teardown began.

[Severity: Low]
Is this paragraph accurate? rxrpc_queue_conn() checks only the active count
and takes no reference:

net/rxrpc/conn_object.c:rxrpc_queue_conn() {
	if (atomic_read(&conn->active) >= 0 &&
	    rxrpc_queue_work(&conn->processor))
		rxrpc_see_connection(conn, why);
}

The patch never reads conn->active. rxrpc_get_connection_maybe() checks
only the refcount:

	if (__refcount_inc_not_zero(&conn->ref, &r))

That matches what rxrpc_poke_call() does with rxrpc_try_get_call(), not
what rxrpc_queue_conn() does.

Could the message say that the poke reference is now taken with
inc_not_zero, and drop the claim that this copies the conn->active check
in rxrpc_queue_conn()?

> diff --git a/net/rxrpc/conn_object.c b/net/rxrpc/conn_object.c
> index 0ece717db0f85..1be50e0c9cee5 100644
> --- a/net/rxrpc/conn_object.c
> +++ b/net/rxrpc/conn_object.c
> @@ -34,7 +34,10 @@ void rxrpc_poke_conn(struct rxrpc_connection *conn, enum rxrpc_conn_trace why)
>  	spin_lock_irq(&local->lock);
>  	busy = !list_empty(&conn->attend_link);
>  	if (!busy) {
> -		rxrpc_get_connection(conn, why);
> +		if (!rxrpc_get_connection_maybe(conn, why)) {
> +			spin_unlock_irq(&local->lock);
> +			return;
> +		}
>  		list_add_tail(&conn->attend_link, &local->conn_attend_q);

[Severity: High]
This is a pre-existing issue and was not introduced by this patch, but what
happens if a poke arrives after the I/O thread has left its main loop?

rxrpc_io_thread() breaks out of the loop once kthread_should_stop() is set
and conn_attend_q is empty. It then calls rxrpc_destroy_local():

net/rxrpc/io_thread.c:rxrpc_io_thread() {
    ...
		if (should_stop)
			break;
    ...
	rxrpc_destroy_local(local);
    ...
}

rxrpc_destroy_local() sets local->dead and cleans up the client conns, the
socket and rx_queue. It never touches local->conn_attend_q.

A client connection can still have conn->timer armed at that point.
rxrpc_disconnect_client_call() arms the timer for jiffies+2, then puts the
connection on the idle list with a reference held:

net/rxrpc/conn_client.c:rxrpc_disconnect_client_call() {
    ...
		set_bit(RXRPC_CONN_FINAL_ACK_0 + channel, &conn->flags);
		rxrpc_reduce_conn_timer(conn, final_ack_at);
    ...
}

rxrpc_clean_up_local_conns() cancels only local->client_conn_reap_timer
synchronously. It doesn't cancel each connection's own timer before it
drops the unbundle and idle references.

If conn->timer fires in that window, the refcount is still above zero:

rxrpc_connection_timer()
  rxrpc_poke_conn()
    rxrpc_get_connection_maybe()   <- succeeds
    list_add_tail(&conn->attend_link, &local->conn_attend_q)

The only thing that consumes conn_attend_q is the loop that has already
exited. Does this leak the connection and its poke reference, along with
its references on local, peer and bundle and its key?

If so, rxnet->nr_conns never reaches zero. On netns exit,
rxrpc_destroy_all_connections() would then wait forever here:

	wait_var_event(&rxnet->nr_conns, !atomic_read(&rxnet->nr_conns));

AF_RXRPC sockets can be created without privileges. An unprivileged user
could reach this window by closing the socket right after a client call
succeeds.

The old unconditional rxrpc_get_connection() had the same behaviour. The
new inc_not_zero guard doesn't help here because the refcount is nonzero.

Would rxrpc_poke_conn() need a local->dead check under local->lock to close
this? Alternatively, rxrpc_destroy_local() could drain conn_attend_q and
drop the poke references.

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