[PATCH net v9 01/14] afs: Fix lack of loop around sendmsg() to rxrpc

[PATCH net v9 01/14] afs: Fix lack of loop around sendmsg() to rxrpc

[David Howells]

Fix the afs callers of sendmsg() to send data through an rxrpc socket to
call again if a short send occurs.

Note that 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: 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 | 38 ++++++++++++++++++++++++--------------
 1 file changed, 24 insertions(+), 14 deletions(-)

diff --git a/fs/afs/rxrpc.c b/fs/afs/rxrpc.c
index d82916657a3d..a80b043d36be 100644
--- a/fs/afs/rxrpc.c
+++ b/fs/afs/rxrpc.c
@@ -412,26 +412,32 @@ 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,
-				     afs_notify_end_request_tx);
-	if (ret < 0)
-		goto error_do_abort;
+	do {
+		ret = rxrpc_kernel_send_data(call->net->socket, rxcall, &msg,
+					     msg_data_left(&msg),
+					     afs_notify_end_request_tx);
+		if (ret < 0)
+			goto error_do_abort;
+	} while (msg_data_left(&msg) > 0);
 
 	if (call->write_iter) {
 		msg.msg_iter = *call->write_iter;
 		msg.msg_flags &= ~MSG_MORE;
 		trace_afs_send_data(call, &msg);
 
-		ret = rxrpc_kernel_send_data(call->net->socket,
-					     call->rxcall, &msg,
-					     iov_iter_count(&msg.msg_iter),
-					     afs_notify_end_request_tx);
+		do {
+			ret = rxrpc_kernel_send_data(call->net->socket,
+						     call->rxcall, &msg,
+						     msg_data_left(&msg),
+						     afs_notify_end_request_tx);
+			if (ret < 0) {
+				trace_afs_sent_data(call, &msg, ret);
+				goto error_do_abort;
+			}
+		} while (msg_data_left(&msg) > 0);
 		*call->write_iter = msg.msg_iter;
 
-		trace_afs_sent_data(call, &msg, ret);
-		if (ret < 0)
-			goto error_do_abort;
+		trace_afs_sent_data(call, &msg, 0);
 	}
 
 	/* Note that at this point, we may have received the reply or an abort
@@ -912,8 +918,12 @@ 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);
+	do {
+		n = rxrpc_kernel_send_data(net->socket, call->rxcall,
+					   &msg, msg_data_left(&msg),
+					   afs_notify_end_reply_tx);
+	} while (n >= 0 && msg_data_left(&msg) > 0);
+
 	if (n >= 0) {
 		/* Success */
 		_leave(" [replied]");

[PATCH net v9 02/14] 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               | 35 +++++++++++------------------------
 include/trace/events/rxrpc.h |  2 +-
 2 files changed, 12 insertions(+), 25 deletions(-)

diff --git a/fs/afs/rxrpc.c b/fs/afs/rxrpc.c
index a80b043d36be..04756d8744e2 100644
--- a/fs/afs/rxrpc.c
+++ b/fs/afs/rxrpc.c
@@ -449,10 +449,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;
@@ -865,6 +863,7 @@ void afs_send_empty_reply(struct afs_call *call)
 {
 	struct afs_net *net = call->net;
 	struct msghdr msg;
+	int ret;
 
 	_enter("");
 
@@ -877,22 +876,13 @@ 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,
-				       afs_notify_end_reply_tx)) {
-	case 0:
-		_leave(" [replied]");
+	ret = rxrpc_kernel_send_data(net->socket, call->rxcall, &msg, 0,
+				     afs_notify_end_reply_tx);
+	if (ret >= 0)
 		return;
 
-	case -ENOMEM:
-		_debug("oom");
-		rxrpc_kernel_abort_call(net->socket, call->rxcall,
-					RXGEN_SS_MARSHAL, -ENOMEM,
-					afs_abort_oom);
-		fallthrough;
-	default:
-		_leave(" [error]");
-		return;
-	}
+	rxrpc_kernel_abort_call(net->socket, call->rxcall,
+				RXGEN_SS_MARSHAL, ret, afs_abort_send_error);
 }
 
 /*
@@ -930,12 +920,9 @@ void afs_send_simple_reply(struct afs_call *call, const void *buf, size_t len)
 		return;
 	}
 
-	if (n == -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, n, 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 */						\

[PATCH net v9 04/14] rxrpc: Fix sendmsg to not return an error if last packet queued

[David Howells]

Fix AF_RXRPC sendmsg() 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.

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      |  2 +-
 net/rxrpc/sendmsg.c | 22 ++++++++++++++++------
 2 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/fs/afs/rxrpc.c b/fs/afs/rxrpc.c
index 04756d8744e2..1f5b6aa68943 100644
--- a/fs/afs/rxrpc.c
+++ b/fs/afs/rxrpc.c
@@ -878,7 +878,7 @@ void afs_send_empty_reply(struct afs_call *call)
 
 	ret = rxrpc_kernel_send_data(net->socket, call->rxcall, &msg, 0,
 				     afs_notify_end_reply_tx);
-	if (ret >= 0)
+	if (ret >= 0) /* Shouldn't buffer more than 0 bytes. */
 		return;
 
 	rxrpc_kernel_abort_call(net->socket, call->rxcall,
diff --git a/net/rxrpc/sendmsg.c b/net/rxrpc/sendmsg.c
index ed2c9a51005a..1d66e9808162 100644
--- a/net/rxrpc/sendmsg.c
+++ b/net/rxrpc/sendmsg.c
@@ -453,9 +453,6 @@ static int rxrpc_send_data(struct rxrpc_sock *rx,
 
 success:
 	ret = copied;
-	if (rxrpc_call_is_complete(call) &&
-	    call->error < 0)
-		ret = call->error;
 out:
 	call->tx_pending = txb;
 	_leave(" = %d", ret);
@@ -467,8 +464,14 @@ static int rxrpc_send_data(struct rxrpc_sock *rx,
 	return call->error;
 
 maybe_error:
-	if (copied)
+	if (copied) {
+		if (rxrpc_call_is_complete(call) &&
+		    call->error < 0) {
+			ret = call->error;
+			goto out;
+		}
 		goto success;
+	}
 	goto out;
 
 efault:
@@ -800,9 +803,16 @@ int rxrpc_do_sendmsg(struct rxrpc_sock *rx, struct msghdr *msg, size_t len)
  * Allow a kernel service to send data on a call.  The call must be in an state
  * appropriate to sending data.  No control data should be supplied in @msg,
  * nor should an address be supplied.  MSG_MORE should be flagged if there's
- * more data to come, otherwise this data will end the transmission phase.
+ * more data to come, otherwise this data will end the transmission phase if
+ * all the data is buffered.
+ *
+ * Note that this function may return a short send, in which case it should be
+ * called again for the remainder of the data or to pick up an error that
+ * caused the short send.
  *
- * Return: %0 if successful and a negative error code otherwise.
+ * Return: The number of bytes buffered (could be %0 if @len is 0 or
+ * msg_iter holds 0 bytes) 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,

[PATCH net v9 05/14] rxrpc: Fix sendmsg length

[David Howells]

rxrpc_send_data() is given two data lengths (len and msg->msg_iter.count)
and is inconsistent about how it uses them.  Fix this by using len in
preference to msg->msg_iter.count.  Also limit the amount copied to either
len or msg->msg_iter.count, whichever is smaller.

Note that, currently, all the callers have len and msg->msg_iter.count the
same and so the problem won't occur.

Fixes: 382d7974de31 ("RxRPC: Use iov_iter_count() in rxrpc_send_data() instead of the len argument")
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
---
 net/rxrpc/sendmsg.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/net/rxrpc/sendmsg.c b/net/rxrpc/sendmsg.c
index 1d66e9808162..565799548102 100644
--- a/net/rxrpc/sendmsg.c
+++ b/net/rxrpc/sendmsg.c
@@ -379,9 +379,9 @@ static int rxrpc_send_data(struct rxrpc_sock *rx,
 
 	ret = -EMSGSIZE;
 	if (call->tx_total_len != -1) {
-		if (len - copied > call->tx_total_len)
+		if (len > call->tx_total_len)
 			goto maybe_error;
-		if (!more && len - copied != call->tx_total_len)
+		if (!more && len != call->tx_total_len)
 			goto maybe_error;
 	}
 
@@ -405,7 +405,7 @@ static int rxrpc_send_data(struct rxrpc_sock *rx,
 			 * the security header is going to be in the padded
 			 * region (enc blocksize), but the trailer is not.
 			 */
-			remain = more ? INT_MAX : msg_data_left(msg);
+			remain = more ? INT_MAX : len;
 			txb = call->conn->security->alloc_txbuf(call, remain, sk->sk_allocation);
 			if (!txb) {
 				ret = -ENOMEM;
@@ -416,8 +416,8 @@ static int rxrpc_send_data(struct rxrpc_sock *rx,
 		_debug("append");
 
 		/* append next segment of data to the current buffer */
-		if (msg_data_left(msg) > 0) {
-			size_t copy = umin(txb->space, msg_data_left(msg));
+		if (len > 0) {
+			size_t copy = min3(txb->space, len, msg_data_left(msg));
 
 			_debug("add %zu", copy);
 			if (!copy_from_iter_full(txb->data + txb->offset,
@@ -428,6 +428,7 @@ static int rxrpc_send_data(struct rxrpc_sock *rx,
 			txb->len += copy;
 			txb->offset += copy;
 			copied += copy;
+			len -= copy;
 			if (call->tx_total_len != -1)
 				call->tx_total_len -= copy;
 		}
@@ -439,8 +440,8 @@ static int rxrpc_send_data(struct rxrpc_sock *rx,
 
 		/* add the packet to the send queue if it's now full */
 		if (!txb->space ||
-		    (msg_data_left(msg) == 0 && !more)) {
-			if (msg_data_left(msg) == 0 && !more)
+		    (len == 0 && !more)) {
+			if (len == 0 && !more)
 				txb->flags |= RXRPC_LAST_PACKET;
 
 			ret = call->security->secure_packet(call, txb);
@@ -449,7 +450,7 @@ static int rxrpc_send_data(struct rxrpc_sock *rx,
 			rxrpc_queue_packet(rx, call, txb, notify_end_tx);
 			txb = NULL;
 		}
-	} while (msg_data_left(msg) > 0);
+	} while (len > 0 && msg_data_left(msg) > 0);
 
 success:
 	ret = copied;

[PATCH net v9 06/14] 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 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).

 (3) The TX_NO_MORE test is moved so that both it and the TX_ERROR test are
     repeated after a wait-for-space is performed.

afs_make_call() and afs_send_simple_reply() are also modified to repeat
calls to rxrpc_kernel_send_data() if less than a full transfer was made.

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 | 11 +++++-
 include/trace/events/rxrpc.h       |  1 +
 net/rxrpc/ar-internal.h            |  1 +
 net/rxrpc/sendmsg.c                | 60 ++++++++++++++++++++++++------
 4 files changed, 60 insertions(+), 13 deletions(-)

diff --git a/Documentation/networking/rxrpc.rst b/Documentation/networking/rxrpc.rst
index 8926dab8e2e6..7df6aff7644c 100644
--- a/Documentation/networking/rxrpc.rst
+++ b/Documentation/networking/rxrpc.rst
@@ -879,14 +879,21 @@ 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.  len is the total amount of data to transmit.
+     msg must not specify a destination address, control data or any flags
+     other than MSG_MORE.  len is the amount of data to add to the
+     transmission.  The last-packet flag will only be set on the outgoing
+     packet if MSG_MORE is not set and len amount of bytes are 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
      called with a spinlock held to prevent the last DATA packet from being
      transmitted until the function returns.
 
+     The function returns the amount of data buffered or an error.  It will
+     return zero only if len is 0 or if msg->msg_iter is empty.  It may also
+     make a short write, buffering less than the amount of data provided or the
+     len specified, in which case it should be called again.
+
  (#) Receive data from a call::
 
 	int rxrpc_kernel_recv_data(struct socket *sock,
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 565799548102..4ce3ae0ba2e8 100644
--- a/net/rxrpc/sendmsg.c
+++ b/net/rxrpc/sendmsg.c
@@ -330,13 +330,6 @@ static int rxrpc_send_data(struct rxrpc_sock *rx,
 	bool more = msg->msg_flags & MSG_MORE;
 	int ret, copied = 0;
 
-	if (test_bit(RXRPC_CALL_TX_NO_MORE, &call->flags)) {
-		trace_rxrpc_abort(call->debug_id, rxrpc_sendmsg_late_send,
-				  call->cid, call->call_id, call->rx_consumed,
-				  0, -EPROTO);
-		return -EPROTO;
-	}
-
 	timeo = sock_sndtimeo(sk, msg->msg_flags & MSG_DONTWAIT);
 
 	ret = rxrpc_wait_to_be_connected(call, &timeo);
@@ -353,6 +346,21 @@ static int rxrpc_send_data(struct rxrpc_sock *rx,
 	sk_clear_bit(SOCKWQ_ASYNC_NOSPACE, sk);
 
 reload:
+	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 maybe_error;
+	}
+	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 maybe_error;
+	}
+
 	txb = call->tx_pending;
 	call->tx_pending = NULL;
 	if (txb)
@@ -441,12 +449,26 @@ 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;
+			}
+
+			if (len == 0 && !more)
+				txb->flags |= RXRPC_LAST_PACKET;
 			rxrpc_queue_packet(rx, call, txb, notify_end_tx);
 			txb = NULL;
 		}
@@ -464,6 +486,22 @@ static int rxrpc_send_data(struct rxrpc_sock *rx,
 	_leave(" = %d", call->error);
 	return call->error;
 
+maybe_error_rewind:
+	/* If we got a retryable error after copying all the supplied data into
+	 * the last packet, we need to rewind as much as we can so the caller
+	 * knows they need to retry the sendmsg.
+	 */
+	if (copied && !more && !len) {
+		unsigned int rewind_by = umin(copied, txb->len);
+
+		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 (rxrpc_call_is_complete(call) &&

[PATCH net v9 07/14] 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

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 | 48 ++++++++++++++++++++++-----------------------
 1 file changed, 24 insertions(+), 24 deletions(-)

diff --git a/net/rxrpc/sendmsg.c b/net/rxrpc/sendmsg.c
index 4ce3ae0ba2e8..8bb327dc2833 100644
--- a/net/rxrpc/sendmsg.c
+++ b/net/rxrpc/sendmsg.c
@@ -320,8 +320,8 @@ 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;
@@ -334,12 +334,12 @@ static int rxrpc_send_data(struct rxrpc_sock *rx,
 
 	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 */
@@ -464,7 +464,7 @@ static int rxrpc_send_data(struct rxrpc_sock *rx,
 				if (ret == -ENOMEM)
 					goto maybe_error_rewind;
 				set_bit(RXRPC_CALL_TX_ERROR, &call->flags);
-				goto out;
+				goto out_txb;
 			}
 
 			if (len == 0 && !more)
@@ -476,15 +476,18 @@ static int rxrpc_send_data(struct rxrpc_sock *rx,
 
 success:
 	ret = copied;
-out:
+out_txb:
 	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;
+	call->tx_pending = NULL;
+	ret = call->error;
+	goto out_unlock;
 
 maybe_error_rewind:
 	/* If we got a retryable error after copying all the supplied data into
@@ -507,36 +510,38 @@ static int rxrpc_send_data(struct rxrpc_sock *rx,
 		if (rxrpc_call_is_complete(call) &&
 		    call->error < 0) {
 			ret = call->error;
-			goto out;
+			goto out_txb;
 		}
 		goto success;
 	}
-	goto out;
+	goto out_txb;
 
 efault:
 	ret = -EFAULT;
-	goto out;
+	goto out_txb;
 
 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;
 }
 
 /*
@@ -702,7 +707,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 = {
@@ -811,16 +815,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);
@@ -857,7 +860,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);
@@ -868,12 +870,10 @@ 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 v9 10/14] 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.

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 | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/net/rxrpc/key.c b/net/rxrpc/key.c
index a0aa78d89289..30d6db052c21 100644
--- a/net/rxrpc/key.c
+++ b/net/rxrpc/key.c
@@ -172,6 +172,7 @@ static int rxrpc_preparse_xdr_yfs_rxgk(struct key_preparsed_payload *prep,
 	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]),
@@ -229,6 +230,11 @@ 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;
 
+	if (!crypto_krb5_find_enctype(token->rxgk->enctype)) {
+		ret = -ENOPKG;
+		goto reject_token;
+	}
+
 	if (token->rxgk->endtime != 0) {
 		expiry = rxrpc_s64_to_time64(token->rxgk->endtime);
 		if (expiry < 0)
@@ -280,7 +286,7 @@ 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);

[PATCH net v9 11/14] 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 v9 13/14] 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);

[PATCH net v9 14/14] rxrpc: Take write lock when publishing the initial RxGK key

[David Howells]

From: Chengfeng Ye <nicoyip.dev@gmail.com>

rxgk_rekey() updates the transport-key ring under security_use_lock,
and rxgk_get_key() takes a reference under the corresponding read
lock.  The initial publication in rxgk_init_connection_security()
writes conn->rxgk.enctype and conn->rxgk.keys[] without the write
lock.

On a client connection, a second sendmsg can observe
RXRPC_CONN_CLIENT through a lockless load of conn->state, skip
rxrpc_init_client_conn_security(), and call rxgk_get_key() without
ever acquiring security_lock.  Because the initializer never took
the write lock, the reader's read lock provides neither exclusion
nor a matching acquire-release pair.

Concurrent RxGK key consumers were observed in a two-sender
workload.  KCSAN reported:

  BUG: KCSAN: data-race in rxgk_get_key / rxgk_secure_packet

  write to 0xffff8aef4023c318 of 8 bytes by task 1968 on cpu 0:
   rxgk_secure_packet+0x46c/0x820
   rxrpc_send_data+0x562/0x1a20
   rxrpc_do_sendmsg+0x976/0xa80
   rxrpc_sendmsg+0x20f/0x2a0

  read to 0xffff8aef4023c318 of 8 bytes by task 1969 on cpu 1:
   rxgk_get_key+0x209/0x5e0
   rxgk_alloc_txbuf+0xa4/0x2a0
   rxrpc_send_data+0x8e2/0x1a20
   rxrpc_do_sendmsg+0x976/0xa80
   rxrpc_sendmsg+0x20f/0x2a0

  value changed: 0x7fffffffffffffff -> 0x7fffffffffffffee

That report is on the key context's byte counter rather than the
initial publication, but it shows that lookup and secured transmit
already overlap on the same connection.

Take security_use_lock for writing while publishing the initial
enctype and transport key, matching the locking used when rekeying.

Fixes: 9d1d2b59341f ("rxrpc: rxgk: Implement the yfs-rxgk security class (GSSAPI)")
Signed-off-by: Chengfeng Ye <nicoyip.dev@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
---
 net/rxrpc/rxgk.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/rxrpc/rxgk.c b/net/rxrpc/rxgk.c
index 77a67ace1d24..b49221f57f67 100644
--- a/net/rxrpc/rxgk.c
+++ b/net/rxrpc/rxgk.c
@@ -251,8 +251,10 @@ static int rxgk_init_connection_security(struct rxrpc_connection *conn,
 					 GFP_NOFS);
 	if (IS_ERR(gk))
 		return PTR_ERR(gk);
+	write_lock(&conn->security_use_lock);
 	conn->rxgk.enctype = gk->krb5->etype;
 	conn->rxgk.keys[gk->key_number & 3] = gk;
+	write_unlock(&conn->security_use_lock);
 
 	switch (conn->security_level) {
 	case RXRPC_SECURITY_PLAIN:

Re: [PATCH net v9 01/14] afs: Fix lack of loop around sendmsg() to rxrpc

[David Laight]

On Mon,  7 Sep 2026 12:37:28 +0100
David Howells <dhowells@redhat.com> wrote:

> Fix the afs callers of sendmsg() to send data through an rxrpc socket to
> call again if a short send occurs.
> 
> Note that 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: 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 | 38 ++++++++++++++++++++++++--------------
>  1 file changed, 24 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/afs/rxrpc.c b/fs/afs/rxrpc.c
> index d82916657a3d..a80b043d36be 100644
> --- a/fs/afs/rxrpc.c
> +++ b/fs/afs/rxrpc.c
> @@ -412,26 +412,32 @@ 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,
> -				     afs_notify_end_request_tx);
> -	if (ret < 0)
> -		goto error_do_abort;
> +	do {
> +		ret = rxrpc_kernel_send_data(call->net->socket, rxcall, &msg,
> +					     msg_data_left(&msg),
> +					     afs_notify_end_request_tx);
> +		if (ret < 0)
> +			goto error_do_abort;
> +	} while (msg_data_left(&msg) > 0);

Is there any reason you didn't change rxrpc_kernel_send_data() instead?

David

>  
>  	if (call->write_iter) {
>  		msg.msg_iter = *call->write_iter;
>  		msg.msg_flags &= ~MSG_MORE;
>  		trace_afs_send_data(call, &msg);
>  
> -		ret = rxrpc_kernel_send_data(call->net->socket,
> -					     call->rxcall, &msg,
> -					     iov_iter_count(&msg.msg_iter),
> -					     afs_notify_end_request_tx);
> +		do {
> +			ret = rxrpc_kernel_send_data(call->net->socket,
> +						     call->rxcall, &msg,
> +						     msg_data_left(&msg),
> +						     afs_notify_end_request_tx);
> +			if (ret < 0) {
> +				trace_afs_sent_data(call, &msg, ret);
> +				goto error_do_abort;
> +			}
> +		} while (msg_data_left(&msg) > 0);
>  		*call->write_iter = msg.msg_iter;
>  
> -		trace_afs_sent_data(call, &msg, ret);
> -		if (ret < 0)
> -			goto error_do_abort;
> +		trace_afs_sent_data(call, &msg, 0);
>  	}
>  
>  	/* Note that at this point, we may have received the reply or an abort
> @@ -912,8 +918,12 @@ 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);
> +	do {
> +		n = rxrpc_kernel_send_data(net->socket, call->rxcall,
> +					   &msg, msg_data_left(&msg),
> +					   afs_notify_end_reply_tx);
> +	} while (n >= 0 && msg_data_left(&msg) > 0);
> +
>  	if (n >= 0) {
>  		/* Success */
>  		_leave(" [replied]");
> 
> 

Re: [PATCH net v9 02/14] 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: 0 · Medium: 0 · Low: 1

- [Low] Whitespace debris introduced by the patch:
  afs_send_simple_reply() in fs/afs/rxrpc.c now contains two consecutive…

--

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

This makes afs_send_empty_reply() and afs_send_simple_reply() always try to
abort the rxrpc call on a send error instead of only aborting on -ENOMEM,
and makes afs_make_call() abort unconditionally on send error.  The
afs_abort_oom trace enum is replaced by 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 a80b043d36be4..04756d8744e2f 100644
> --- a/fs/afs/rxrpc.c
> +++ b/fs/afs/rxrpc.c

[ ... ]

> @@ -930,12 +920,9 @@ void afs_send_simple_reply(struct afs_call *call, const void *buf, size_t len)
>  		return;
>  	}
>  
> -	if (n == -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, n, afs_abort_send_error);
>  	_leave(" [error]");
>  }

[Severity: Low]
This isn't a bug, but the removal of the -ENOMEM block seems to have left an
extra blank line behind in afs_send_simple_reply().  After the patch the
function reads:

	if (n >= 0) {
		/* Success */
		_leave(" [replied]");
		return;
	}

	rxrpc_kernel_abort_call(net->socket, call->rxcall,
				RXGEN_SS_MARSHAL, n, afs_abort_send_error);

That is two consecutive blank lines, which checkpatch --strict flags with
"please don't use multiple blank lines".  Since this patch carries
cc: stable@vger.kernel.org, would it be worth dropping the added blank line
so the backports don't inherit it?  It is still present at the end of the
series.

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

Re: [PATCH net v9 04/14] rxrpc: Fix sendmsg to not return an error if last packet queued

[netdev-bot+sashiko]

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

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

- [Medium] rxrpc_send_data() (net/rxrpc/sendmsg.c): the patch deletes the
  call-completion/error check from the *unconditional* `success:` path…

Pre-existing issues:
- [Low] Divergence between Documentation/networking/rxrpc.rst and the
  kernel-doc for rxrpc_kernel_send_data().
- [Low] At this patch, rxrpc_send_data()'s `out:` label writes
  `call->tx_pending = txb` (net/rxrpc/sendmsg.c:456-457) and is…

--

rxrpc: Fix sendmsg to not return an error if last packet queued

This moves the "call completed with an error" check in rxrpc_send_data()
out of the success path and into the partial-send path, so that a sendmsg()
that queued the last packet of a call reports the byte count and leaves the
completion for recvmsg() to report.  The kernel-doc for
rxrpc_kernel_send_data() is updated to document a byte-count return and the
possibility of short sends.

[ ... fs/afs/rxrpc.c hunk snipped, unrelated to the comments below ... ]

> diff --git a/net/rxrpc/sendmsg.c b/net/rxrpc/sendmsg.c
> index ed2c9a51005ad..1d66e98081624 100644
> --- a/net/rxrpc/sendmsg.c
> +++ b/net/rxrpc/sendmsg.c
> @@ -453,9 +453,6 @@ static int rxrpc_send_data(struct rxrpc_sock *rx,
>  
>  success:
>  	ret = copied;
> -	if (rxrpc_call_is_complete(call) &&
> -	    call->error < 0)
> -		ret = call->error;
>  out:
>  	call->tx_pending = txb;
>  	_leave(" = %d", ret);

[Severity: Medium]

The commit message describes the change as covering the case where sendmsg
"has successfully queued the last packet of a call", but the check is being
removed from the unconditional success: label in rxrpc_send_data(), which is
reached on every normal exit from the copy loop.  Is the broader scope
intended?

Two cases reach success: without any last packet having been queued:

 - a MSG_MORE send whose data does not fill a txbuf, where the bytes are
   only copied into call->tx_pending and nothing is queued at all;

 - a MSG_MORE send that queues a non-final packet.

For those sends the only completion test left is the one inside the loop:

	/* check for the far side aborting the call or a network error
	 * occurring */
	if (rxrpc_call_is_complete(call))
		goto call_terminated;

If the rxrpc I/O thread completes the call with an abort or network error
after that test and before the loop exits, sendmsg() now returns a positive
byte count for data that will never be transmitted.

The specific error is then not recoverable from the next sendmsg() either,
since rxrpc_do_sendmsg() substitutes a generic code before reaching
rxrpc_send_data():

	if (rxrpc_call_is_complete(call)) {
		/* it's too late for this call */
		ret = -ESHUTDOWN;
		goto out_put_unlock;
	}

so a remote abort delivered as -ECONNABORTED is reported as -ESHUTDOWN, and
userspace has to go to recvmsg() to obtain the abort code.

Since this patch is tagged for stable, would it be worth narrowing the
change to the "last packet queued" case, or describing the MSG_MORE
behaviour change in the commit message?

> @@ -467,8 +464,14 @@ static int rxrpc_send_data(struct rxrpc_sock *rx,
>  	return call->error;
>  
>  maybe_error:
> -	if (copied)
> +	if (copied) {
> +		if (rxrpc_call_is_complete(call) &&
> +		    call->error < 0) {
> +			ret = call->error;
> +			goto out;
> +		}
>  		goto success;
> +	}
>  	goto out;
>  
>  efault:

Reinstating the check here does not cover the cases above: once the last
packet has been queued the loop exits straight to success:, so maybe_error
is only reachable for partial or failed sends.

[Severity: Low]

This is a pre-existing issue and not introduced by this patch, but the
rewritten maybe_error block is the entry point for paths that jump to out:,
where the mutex-protected call->tx_pending is stored:

out:
	call->tx_pending = txb;

Is that store always made with call->user_mutex held?  wait_for_space
drops the mutex and then jumps back to maybe_error on failure:

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;
	if (call->interruptibility == RXRPC_INTERRUPTIBLE) {
		if (mutex_lock_interruptible(&call->user_mutex) < 0) {
			ret = sock_intr_errno(timeo);
			goto maybe_error;
		}

With two concurrent sendmsg() calls on the same user_call_ID, can the
unlocked store overwrite a partially filled txbuf that the thread actually
holding the mutex has just stored, leaking that rxrpc_txbuf and dropping
the data already copied into it?

For the record, the later commit "rxrpc: Fix update of call->tx_pending
without holding lock" in the same series removes this by moving the store
to an out_txb: label immediately followed by out_unlock: with the
mutex_unlock(), dropping the _dropped_lock parameter and returning the
lock-dropped failure paths via out_nolock: without touching
call->tx_pending.

> @@ -800,9 +803,16 @@ int rxrpc_do_sendmsg(struct rxrpc_sock *rx, struct msghdr *msg, size_t len)
>   * Allow a kernel service to send data on a call.  The call must be in an state
>   * appropriate to sending data.  No control data should be supplied in @msg,
>   * nor should an address be supplied.  MSG_MORE should be flagged if there's
> - * more data to come, otherwise this data will end the transmission phase.
> + * more data to come, otherwise this data will end the transmission phase if
> + * all the data is buffered.
> + *
> + * Note that this function may return a short send, in which case it should be
> + * called again for the remainder of the data or to pick up an error that
> + * caused the short send.
>   *
> - * Return: %0 if successful and a negative error code otherwise.
> + * Return: The number of bytes buffered (could be %0 if @len is 0 or
> + * msg_iter holds 0 bytes) if successful and a negative error code
> + * otherwise.
>   */

[Severity: Low]

This is a pre-existing documentation issue rather than something introduced
here, but Documentation/networking/rxrpc.rst describes a different contract
for rxrpc_kernel_send_data() than this kernel-doc: the rst still says len is
"the total amount of data to transmit" and mentions neither short sends nor
a byte-count return.

That paragraph is rewritten later in the series by "rxrpc: Fix packet
encryption error handling", which changes it to "len is the amount of data
to add to the transmission" and adds the short-write wording, so only one
sentence in the rst is left out of step:

     msg must not specify a destination address, control data or any flags
     other than MSG_MORE.

afs_make_call() passes MSG_WAITALL | MSG_MORE to rxrpc_kernel_send_data(),
which predates this series.  Should the flag restriction in the rst be
updated as well?

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

Re: [PATCH net v9 05/14] rxrpc: Fix sendmsg length

[netdev-bot+sashiko]

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

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

- [Low] "rxrpc: Fix sendmsg length" carries a `Fixes:` tag and `cc:
  stable@vger.kernel.org` while its own commit message states…

--

rxrpc: Fix sendmsg length

This makes rxrpc_send_data() consistently use its len argument rather than
msg->msg_iter.count, decrementing len as data is copied, and limits each
copy to the smaller of len and msg_data_left(msg).

> diff --git a/net/rxrpc/sendmsg.c b/net/rxrpc/sendmsg.c
> index 1d66e98081624..565799548102c 100644
> --- a/net/rxrpc/sendmsg.c
> +++ b/net/rxrpc/sendmsg.c
> @@ -379,9 +379,9 @@ static int rxrpc_send_data(struct rxrpc_sock *rx,
>  
>  	ret = -EMSGSIZE;
>  	if (call->tx_total_len != -1) {
> -		if (len - copied > call->tx_total_len)
> +		if (len > call->tx_total_len)
>  			goto maybe_error;
> -		if (!more && len - copied != call->tx_total_len)
> +		if (!more && len != call->tx_total_len)
>  			goto maybe_error;
>  	}

[Severity: Low]
This isn't a bug in the code, but a question about the tagging.  The
commit carries:

    Fixes: 382d7974de31 ("RxRPC: Use iov_iter_count() in rxrpc_send_data() instead of the len argument")
    cc: stable@vger.kernel.org

while the message itself says:

    Note that, currently, all the callers have len and msg->msg_iter.count the
    same and so the problem won't occur.

Walking the callers agrees with that statement.  rxrpc_do_sendmsg()
forwards the sendmsg() len that the syscall also used to build msg_iter.
In fs/afs/rxrpc.c, afs_make_call() and afs_send_simple_reply() pass
msg_data_left(&msg):

	ret = rxrpc_kernel_send_data(call->net->socket, rxcall, &msg,
				     msg_data_left(&msg),
				     afs_notify_end_request_tx);

and afs_send_empty_reply() passes 0 for a zero-length iterator.  In
net/rxrpc/rxperf.c, rxperf_process_call() passes the same local len used
to build the iterator:

		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);

So no in-tree caller can diverge.  Given there is no symptom, reproducer
or affected caller named, are the Fixes: and cc: stable tags intended
here?

What stable trees would pick up instead is a rewrite of the sendmsg data
loop: len becomes mutable and is decremented in the loop, the copy bound
becomes min3(), the loop termination test gains a second clause, and the
-EMSGSIZE re-check that is re-executed via the reload label now tests the
decremented len instead of len - copied.

>  
> @@ -405,7 +405,7 @@ static int rxrpc_send_data(struct rxrpc_sock *rx,
>  			 * the security header is going to be in the padded
>  			 * region (enc blocksize), but the trailer is not.
>  			 */
> -			remain = more ? INT_MAX : msg_data_left(msg);
> +			remain = more ? INT_MAX : len;
>  			txb = call->conn->security->alloc_txbuf(call, remain, sk->sk_allocation);
>  			if (!txb) {
>  				ret = -ENOMEM;
> @@ -416,8 +416,8 @@ static int rxrpc_send_data(struct rxrpc_sock *rx,
>  		_debug("append");
>  
>  		/* append next segment of data to the current buffer */
> -		if (msg_data_left(msg) > 0) {
> -			size_t copy = umin(txb->space, msg_data_left(msg));
> +		if (len > 0) {
> +			size_t copy = min3(txb->space, len, msg_data_left(msg));
>  
>  			_debug("add %zu", copy);
>  			if (!copy_from_iter_full(txb->data + txb->offset,
> @@ -428,6 +428,7 @@ static int rxrpc_send_data(struct rxrpc_sock *rx,
>  			txb->len += copy;
>  			txb->offset += copy;
>  			copied += copy;
> +			len -= copy;
>  			if (call->tx_total_len != -1)
>  				call->tx_total_len -= copy;
>  		}
> @@ -439,8 +440,8 @@ static int rxrpc_send_data(struct rxrpc_sock *rx,
>  
>  		/* add the packet to the send queue if it's now full */
>  		if (!txb->space ||
> -		    (msg_data_left(msg) == 0 && !more)) {
> -			if (msg_data_left(msg) == 0 && !more)
> +		    (len == 0 && !more)) {
> +			if (len == 0 && !more)
>  				txb->flags |= RXRPC_LAST_PACKET;
>  
>  			ret = call->security->secure_packet(call, txb);
> @@ -449,7 +450,7 @@ static int rxrpc_send_data(struct rxrpc_sock *rx,
>  			rxrpc_queue_packet(rx, call, txb, notify_end_tx);
>  			txb = NULL;
>  		}
> -	} while (msg_data_left(msg) > 0);
> +	} while (len > 0 && msg_data_left(msg) > 0);

Making len mutable also appears to be a prerequisite for the later patch
in the series, "rxrpc: Fix packet encryption error handling", whose rewind
path tests:

	if (copied && !more && !len) {

which only behaves as intended once len is decremented here.  Could that
dependency be mentioned in the commit message?

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

Re: [PATCH net v9 06/14] rxrpc: Fix packet encryption error handling

[netdev-bot+sashiko]

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

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

- [Critical] rxrpc_send_data() (net/rxrpc/sendmsg.c) publishes an
  uninitialised pointer into call->tx_pending.
- [High] Concurrent sendmsg calls cause maybe_error_rewind to rewind
  another thread's data and revert the msg_iter too far, causing data…
- [Medium] The commit message of "rxrpc: Fix packet encryption error
  handling" states "afs_make_call() and afs_send_simple_reply() are also…
- [Medium] Stale local `txb` after the wait-for-space lock drop: on the
  wait_for_space path rxrpc_send_data() drops call->user_mutex…

--

rxrpc: Fix packet encryption error handling

This changes rxrpc_send_data() so that an -ENOMEM from ->secure_packet() is
treated as retryable (rewinding the last partial packet and the iterator),
while any other encryption error sets a new RXRPC_CALL_TX_ERROR flag and
makes subsequent data adds return -EIO.  The RXRPC_CALL_TX_NO_MORE test is
also moved down to the reload: label so it is rechecked after a
wait-for-space.

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

A few questions below.

> rxrpc: Fix packet encryption error handling
>
> [ ... ]
>
> afs_make_call() and afs_send_simple_reply() are also modified to repeat
> calls to rxrpc_kernel_send_data() if less than a full transfer was made.

[Severity: Medium]
Is this paragraph accurate for this commit?  The diffstat only covers
Documentation/networking/rxrpc.rst, include/trace/events/rxrpc.h,
net/rxrpc/ar-internal.h and net/rxrpc/sendmsg.c, with no fs/afs change.

The retry loops in afs_make_call() and afs_send_simple_reply() come from
the earlier series patch bd42a7623ae5 ("afs: Fix lack of loop around
sendmsg() to rxrpc"), and the equivalent rxperf loop from 658684df7eb2
("rxrpc: Fix lack of loop around reply send in rxperf server").

That distinction matters here because this patch carries Fixes:
17926a79320a and cc: stable, and it newly makes rxrpc_send_data() return a
short positive count where it previously returned an error (the
maybe_error_rewind path below).  A caller that treats any non-negative
return as completion, such as an unmodified afs_make_call(), would silently
truncate the request and never queue the last packet:

fs/afs/rxrpc.c:afs_make_call() {
    ...
	do {
		ret = rxrpc_kernel_send_data(call->net->socket, rxcall, &msg,
					     msg_data_left(&msg),
					     afs_notify_end_request_tx);
		if (ret < 0)
			goto error_do_abort;
	} while (msg_data_left(&msg) > 0);
    ...
}

Could the message either drop the afs claim or name bd42a7623ae5 (and
658684df7eb2) as backport prerequisites?

> diff --git a/Documentation/networking/rxrpc.rst b/Documentation/networking/rxrpc.rst
> index 8926dab8e2e60..7df6aff7644c0 100644
> --- a/Documentation/networking/rxrpc.rst
> +++ b/Documentation/networking/rxrpc.rst
> @@ -879,14 +879,21 @@ The kernel interface functions are as follows:
>       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.
>  
> +     The function returns the amount of data buffered or an error.  It will
> +     return zero only if len is 0 or if msg->msg_iter is empty.  It may also
> +     make a short write, buffering less than the amount of data provided or the
> +     len specified, in which case it should be called again.
> +

[ ... ]

> diff --git a/net/rxrpc/sendmsg.c b/net/rxrpc/sendmsg.c
> index 565799548102c..4ce3ae0ba2e88 100644
> --- a/net/rxrpc/sendmsg.c
> +++ b/net/rxrpc/sendmsg.c
> @@ -330,13 +330,6 @@ static int rxrpc_send_data(struct rxrpc_sock *rx,
>  	bool more = msg->msg_flags & MSG_MORE;
>  	int ret, copied = 0;
>  
> -	if (test_bit(RXRPC_CALL_TX_NO_MORE, &call->flags)) {
> -		trace_rxrpc_abort(call->debug_id, rxrpc_sendmsg_late_send,
> -				  call->cid, call->call_id, call->rx_consumed,
> -				  0, -EPROTO);
> -		return -EPROTO;
> -	}
> -
>  	timeo = sock_sndtimeo(sk, msg->msg_flags & MSG_DONTWAIT);
>  
>  	ret = rxrpc_wait_to_be_connected(call, &timeo);
> @@ -353,6 +346,21 @@ static int rxrpc_send_data(struct rxrpc_sock *rx,
>  	sk_clear_bit(SOCKWQ_ASYNC_NOSPACE, sk);
>  
>  reload:
> +	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 maybe_error;
> +	}
> +	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 maybe_error;
> +	}
> +
>  	txb = call->tx_pending;
>  	call->tx_pending = NULL;
>  	if (txb)

[Severity: Critical]
Can these two new gotos store an uninitialised pointer into
call->tx_pending?

The old TX_NO_MORE test did "return -EPROTO" before touching any call
state.  Both new tests instead jump to maybe_error, and they run before the
txb snapshot, while txb is still uninitialised:

net/rxrpc/sendmsg.c:rxrpc_send_data() {
	struct rxrpc_txbuf *txb;		/* no initialiser */
	...
reload:
	if (unlikely(test_bit(RXRPC_CALL_TX_NO_MORE, &call->flags))) {
		...
		goto maybe_error;
	}
	...
	txb = call->tx_pending;			/* never reached */
}

With copied == 0, which is always the case on the first pass, maybe_error
falls straight through to out:

maybe_error:
	if (copied) {
		...
	}
	goto out;

success:
	ret = copied;
out:
	call->tx_pending = txb;

Is the resulting call->tx_pending value indeterminate?  A trigger looks
straightforward from userspace: sendmsg() the whole request without
MSG_MORE, so rxrpc_queue_packet() sets RXRPC_CALL_TX_NO_MORE for the last
packet, then sendmsg() again on the same RXRPC_USER_CALL_ID.
rxrpc_do_sendmsg() still dispatches to rxrpc_send_data() because the call
is in RXRPC_CALL_CLIENT_AWAIT_REPLY, and the state switch there only
rejects UNINITIALISED/PREALLOC/AWAIT_CONN/SERVER_RECV_REQUEST.

The stored value is later consumed at teardown:

net/rxrpc/call_object.c:rxrpc_destroy_call() {
	...
	rxrpc_put_txbuf(call->tx_pending, rxrpc_txbuf_put_cleaned);
	...
}

which dereferences txb->debug_id/call_debug_id/seq, does
__refcount_dec_and_test(&txb->ref) and can then free the buffer.  A further
sendmsg() on the same call would also pick it up via rxrpc_see_txbuf() and
copy user data through "txb->data + txb->offset".

Note that -Wno-maybe-uninitialized is added by scripts/Makefile.warn, so
gcc builds will not flag this.  Would initialising txb = NULL, returning
directly from the two tests, or placing them after the tx_pending fetch be
preferable?

> @@ -441,12 +449,26 @@ 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;
> +			}
> +

[Severity: Medium]
Can this terminal path lose the txbuf it parks in call->tx_pending when
another sender is sleeping in wait-for-space?

The wait_for_space path drops call->user_mutex, sleeps, retakes the mutex
and jumps back to reload:

net/rxrpc/sendmsg.c:rxrpc_send_data() {
    ...
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);
	...
	goto reload;
}

Since the new flag tests at reload: run before "txb = call->tx_pending",
the local txb is still the stale value from before the sleep, and on this
path it is NULL because wait_for_space is only reachable when !txb.  So:

  thread A: sendmsg(), no Tx space, drops user_mutex, sleeps
  thread B: sendmsg() on the same user_call_ID, copies data,
            ->secure_packet() fails with a non-ENOMEM error, does
            set_bit(RXRPC_CALL_TX_ERROR) and "goto out", leaving its
            txbuf in call->tx_pending
  thread A: retakes user_mutex, hits the new TX_ERROR test, reaches
            out: and does call->tx_pending = txb, i.e. NULL

Does that detach B's txbuf permanently?  Nothing else appears to hold a
reference, and rxrpc_destroy_call() only frees what is left in
call->tx_pending, so the txbuf plus its page fragment would never be
released, along with the data already buffered in it.

Would taking the tx_pending snapshot before the flag tests, or returning
directly from them, avoid this?

> +			if (len == 0 && !more)
> +				txb->flags |= RXRPC_LAST_PACKET;
>  			rxrpc_queue_packet(rx, call, txb, notify_end_tx);
>  			txb = NULL;
>  		}
> @@ -464,6 +486,22 @@ static int rxrpc_send_data(struct rxrpc_sock *rx,
>  	_leave(" = %d", call->error);
>  	return call->error;
>  
> +maybe_error_rewind:
> +	/* If we got a retryable error after copying all the supplied data into
> +	 * the last packet, we need to rewind as much as we can so the caller
> +	 * knows they need to retry the sendmsg.
> +	 */
> +	if (copied && !more && !len) {
> +		unsigned int rewind_by = umin(copied, txb->len);
> +
> +		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 (rxrpc_call_is_complete(call) &&

[Severity: High]
Is rewind_by computed from the right quantity here?

copied is cumulative for the whole call to rxrpc_send_data(): it
accumulates across every iteration of the do-while loop, so it can
cover packets that were already handed to rxrpc_queue_packet(), and it
survives the wait_for_space path, which drops call->user_mutex and
sleeps in rxrpc_wait_for_tx_window() before jumping back to reload:.
txb->len, on the other hand, only describes the one packet in front of
us, and it may contain bytes that this call never copied - a previous
(or concurrent) sendmsg() on the same user_call_ID can have left a
partially filled buffer in call->tx_pending, which we pick up again at
the txb = call->tx_pending snapshot.

So the two arguments to umin() are not measuring the same thing, and
the rewind can be too large in both directions:

  thread A: sendmsg(len=6192), fills and queues txb1 (4096 bytes),
            copied=4096, no Tx space left, drops user_mutex and sleeps
  thread B: sendmsg(len=2000, MSG_MORE) on the same call, copies 2000
            bytes into a fresh txb2 and leaves it in call->tx_pending
  thread A: wakes, reloads txb2, copies its remaining 2096 bytes so
            txb2->len == 4096, len == 0, more == false
  thread A: ->secure_packet() returns -ENOMEM, so
            rewind_by = umin(6192, 4096) == 4096

txb->len then goes to 0, which silently discards the 2000 bytes B had
buffered and told userspace were accepted, and
iov_iter_revert(&msg->msg_iter, 4096) rewinds A past the 2096 bytes it
actually put into txb2 and into the region it already queued in txb1,
so the retry re-sends data that is already on the wire.  Duplication
and loss in the same step.  call->tx_total_len is adjusted by the same
wrong figure.

Even without a second thread, if copied spans more than one txb the
umin() clamp to txb->len can still exceed what this iteration put into
the current buffer whenever the buffer was inherited from an earlier
sendmsg().

Shouldn't the rewind use the number of bytes this pass copied into
this specific txb - e.g. a separate counter updated alongside
txb->offset in the copy_from_iter_full() block - rather than the
cumulative copied?  And should the rewind path be reachable at all
when the buffer may have been shared with another sender, or does that
want the tx_pending ownership question sorted out first?

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

Re: [PATCH net v9 07/14] rxrpc: Fix update of call->tx_pending without holding lock

[netdev-bot+sashiko]

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

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

- [Critical] rxrpc_send_data() (net/rxrpc/sendmsg.c) can store an
  *indeterminate* pointer into call->tx_pending.

--

rxrpc: Fix update of call->tx_pending without holding lock

This moves the release of call->user_mutex down into rxrpc_send_data() and
drops the _dropped_lock indicator parameter, so the callers no longer have
to work out whether the lock is still held.  The interrupted-reacquire path
in wait_for_space now returns directly instead of falling through the
common exit path, and the exit labels are renamed to out_txb/out_unlock.

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

> diff --git a/net/rxrpc/sendmsg.c b/net/rxrpc/sendmsg.c
> index 4ce3ae0ba2e88..8bb327dc2833e 100644
> --- a/net/rxrpc/sendmsg.c
> +++ b/net/rxrpc/sendmsg.c

[ ... ]

> @@ -464,7 +464,7 @@ static int rxrpc_send_data(struct rxrpc_sock *rx,
>  				if (ret == -ENOMEM)
>  					goto maybe_error_rewind;
>  				set_bit(RXRPC_CALL_TX_ERROR, &call->flags);
> -				goto out;
> +				goto out_txb;
>  			}
>  
>  			if (len == 0 && !more)
> @@ -476,15 +476,18 @@ static int rxrpc_send_data(struct rxrpc_sock *rx,
>  
>  success:
>  	ret = copied;
> -out:
> +out_txb:
>  	call->tx_pending = txb;
> +out_unlock:
> +	mutex_unlock(&call->user_mutex);
>  	_leave(" = %d", ret);
>  	return ret;

[Severity: Critical]

Can out_txb publish an uninitialised txb into call->tx_pending?

txb is declared without an initialiser in rxrpc_send_data():

	struct rxrpc_txbuf *txb;

but the two flag tests at the top of the reload block run before txb is
ever assigned, and both jump out of the function:

reload:
	if (unlikely(test_bit(RXRPC_CALL_TX_NO_MORE, &call->flags))) {
		...
		ret = -EPROTO;
		goto maybe_error;
	}
	if (unlikely(test_bit(RXRPC_CALL_TX_ERROR, &call->flags))) {
		...
		ret = -EIO;
		goto maybe_error;
	}

	txb = call->tx_pending;
	call->tx_pending = NULL;

With copied == 0, which is the case on first entry, maybe_error falls
straight through to "goto out_txb", so "call->tx_pending = txb;" runs on a
never-assigned variable.

A two-sendmsg() sequence on one AF_RXRPC call looks like it reaches this:
the first sendmsg() sends the whole request without MSG_MORE, so
rxrpc_queue_packet() sets RXRPC_CALL_TX_NO_MORE for the last packet; a
second sendmsg() before the call completes passes the
rxrpc_call_is_complete() gate in rxrpc_do_sendmsg(), enters
rxrpc_send_data(), trips the TX_NO_MORE test and returns -EPROTO via
out_txb.

Does call->tx_pending then hold a stale stack value that rxrpc_destroy_call()
hands to rxrpc_put_txbuf()?

net/rxrpc/call_object.c:rxrpc_destroy_call() {
	...
	rxrpc_put_txbuf(call->tx_pending, rxrpc_txbuf_put_cleaned);
	...
}

net/rxrpc/txbuf.c:rxrpc_put_txbuf() {
	if (txb) {
		debug_id = txb->debug_id;
		call_debug_id = txb->call_debug_id;
		seq = txb->seq;
		dead = __refcount_dec_and_test(&txb->ref, &r);
		...
	}
}

That dereferences, decrements and possibly kfree()s whatever was stored.
CONFIG_INIT_STACK_ALL_ZERO would reduce this to a NULL store, but
CONFIG_INIT_STACK_NONE builds would not.

There also looks to be a txbuf reference dropped on the TX_ERROR path even
when the pointer happens to be NULL: the fatal secure_packet() error above
sets RXRPC_CALL_TX_ERROR and stores the affected txbuf into
call->tx_pending via out_txb, and the next sendmsg() overwrites that
pointer from the TX_ERROR branch before "txb = call->tx_pending;" runs, so
the txbuf and its data buffer are never freed.

The two flag tests were placed above the "txb = call->tx_pending;"
assignment by the earlier "rxrpc: Fix packet encryption error handling"
change in this series, and this patch's rename to out_txb keeps the same
path.  Would having those two paths use "goto out_unlock", or initialising
txb to NULL at declaration, be preferable?

> @@ -507,36 +510,38 @@ static int rxrpc_send_data(struct rxrpc_sock *rx,
>  		if (rxrpc_call_is_complete(call) &&
>  		    call->error < 0) {
>  			ret = call->error;
> -			goto out;
> +			goto out_txb;
>  		}
>  		goto success;
>  	}
> -	goto out;
> +	goto out_txb;

[ ... ]

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

Re: [PATCH net v9 10/14] 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: 1 · Medium: 0 · Low: 1 (1)

- [High] Link/modpost failure in valid configurations: the newly added
  call to `crypto_krb5_find_enctype()` in…
- [Low] Key quota over-charge newly reachable: in
  `rxrpc_preparse_xdr_yfs_rxgk()` (net/rxrpc/key.c:206) the full token…

Pre-existing issues:
- [Low] `rxrpc_preparse_xdr_yfs_rxgk()` now resolves the krb5 enctype
  handle but discards it, so the user-supplied session-key length…

--

rxrpc: Fix RxGK key parser to check enctype is supported

The RxGK token parser in net/rxrpc/key.c now looks up the userspace-supplied
encryption type and rejects the token with -ENOPKG when the enctype is not
known.  The return value of rxrpc_preparse_xdr_yfs_rxgk() is changed from a
hardcoded -EKEYREJECTED to a new local variable so the new error code can be
propagated.

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 a0aa78d892897..30d6db052c216 100644
> --- a/net/rxrpc/key.c
> +++ b/net/rxrpc/key.c
> @@ -172,6 +172,7 @@ static int rxrpc_preparse_xdr_yfs_rxgk(struct key_preparsed_payload *prep,
>  	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]),
> @@ -229,6 +230,11 @@ 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;
>  
> +	if (!crypto_krb5_find_enctype(token->rxgk->enctype)) {
> +		ret = -ENOPKG;
> +		goto reject_token;
> +	}
> +

[Severity: High]
Does this build with CONFIG_AF_RXRPC=y (or m) and CONFIG_RXGK=n?

key.o is built unconditionally, while crypto_krb5_find_enctype() only exists
when CRYPTO_KRB5 is built:

net/rxrpc/Makefile:
	rxrpc-y := \
		...
		key.o \
		...
	rxrpc-$(CONFIG_RXGK) += \
		rxgk.o \
		rxgk_app.o \
		rxgk_kdf.o

crypto/krb5/Makefile:
	obj-$(CONFIG_CRYPTO_KRB5) += krb5.o

net/rxrpc/Kconfig has AF_RXRPC selecting only CRYPTO, CRYPTO_LIB_DES if
RXKAD, KEYS and NET_UDP_TUNNEL; the "select CRYPTO_KRB5" lives in the
optional RXGK bool, which defaults to n.  So AF_RXRPC=y, RXGK=n,
CRYPTO_KRB5=n looks like a legal configuration, and the declaration in
include/crypto/krb5.h (reached via include/keys/rxrpc-type.h) has no
!CONFIG_CRYPTO_KRB5 stub, so the failure would only show up at link or
modpost time as an undefined crypto_krb5_find_enctype.  The same would apply
to AF_RXRPC=y with CRYPTO_KRB5=m.

The other rxrpc caller guards this exact call:

net/rxrpc/rxperf.c:
	#ifdef CONFIG_RXGK
	/*
	 * Add a yfs-rxgk key to the security keyring.
	 */
	static int rxperf_add_yfs_rxgk_key(struct key *keyring, u32 enctype)
	{
		const struct krb5_enctype *krb5 = crypto_krb5_find_enctype(enctype);

Should the new call site be wrapped in #ifdef CONFIG_RXGK too, or should
AF_RXRPC select CRYPTO_KRB5?  Neither net/rxrpc/Kconfig nor
net/rxrpc/Makefile is touched anywhere later in the series, and the patch
carries a cc: stable tag.

[Severity: Low]
Can this over-charge the caller's key quota?

Earlier in rxrpc_preparse_xdr_yfs_rxgk() the whole token cost is accounted
before any validation:

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

The new path frees the token and returns -ENOPKG without undoing that
addition, and rxrpc_preparse_xdr() treats -ENOPKG as a per-token error that
does not abort the key:

		case -ENOPKG:
			if (ret != 0)
				ret = -ENOPKG;
			break;

So a payload holding one parsable token (say rxkad) plus an RxGK token with
an unsupported enctype gets instantiated while prep->quotalen still includes
the discarded RxGK token.  Before this change every reject_token path
returned -EKEYREJECTED, which hit the default: case and failed the whole
instantiation, so the inflated value was never committed.  Would it be
simpler to do the enctype lookup before the prep->quotalen update, or to
subtract it again on this path?

[Severity: Low]
This is a pre-existing issue and not introduced by this patch, but now that
the enctype descriptor is looked up here, should raw_keylen also be checked
against krb5->key_len?  The returned pointer is discarded, so the
user-supplied key length (bounded only by AFSTOKEN_GK_KEY_MAX, and allowed
to be 0) is still unvalidated.

The server-key parser pairs the two checks:

net/rxrpc/rxgk.c:rxgk_preparse_server_key() {
	krb5 = crypto_krb5_find_enctype(enctype);
	if (!krb5)
		return -ENOPKG;
	...
}

At use time rxgk_derive_transport_key() passes &rxgk->key to
crypto_krb5_calc_PRFplus(), and the rfc8009 profile just does:

crypto/krb5/rfc8009_aes2.c:rfc8009_calc_KDF_HMAC_SHA2() {
	ret = crypto_shash_setkey(shash, key->data, key->len);
	...
}

HMAC accepts any length, so a transport key gets derived from a truncated or
empty session key, while the rfc3961/rfc3962 profiles fail later:

crypto/krb5/rfc3961_simplified.c:rfc3961_calc_DK() {
	if (inkey->len != keylength || result->len != keylength)
		return -EINVAL;
	...
}

All consumers honour key.len, so there is no out-of-bounds access, just
acceptance of malformed key material and inconsistent late failures.

>  	if (token->rxgk->endtime != 0) {
>  		expiry = rxrpc_s64_to_time64(token->rxgk->endtime);
>  		if (expiry < 0)
> @@ -280,7 +286,7 @@ 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);

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

Re: [PATCH net v9 11/14] afs: Fix creation of RxGK CM channel token to have right size

[netdev-bot+sashiko]

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

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

- [Low] Changelog of the stable-tagged fix does not state the user-
  visible symptom of the off-by-4 `toksize` in afs_create_yfs_cm_token()…

Pre-existing issues:
- [Low] afs_create_yfs_cm_token() keeps a hand-maintained field-by-field
  size expression (`toksize`, fs/afs/cm_security.c:238) whose only guard…

--

afs: Fix creation of RxGK CM channel token to have right size

This corrects the token size arithmetic in afs_create_yfs_cm_token() by
adding the missing 4 bytes for the RXGK_Level field.  The size feeds the
encrypted token container and the appdata buffer published as
server->cm_rxgk_appdata.

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

> diff --git a/fs/afs/cm_security.c b/fs/afs/cm_security.c
> index 103168c70dd4d..5eeeef761cf31 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);

[Severity: Low]
Could the changelog say what the observable symptom of the old arithmetic
was?  The field count matches what I see written after the K0 blob in
afs_create_yfs_cm_token():

	level(4) + starttime(8) + lifetime(4) + bytelife(4) +
	expirationtime(8) + identities.count(4) + kind(4) +
	data.len(4) + uuid(16) + display.len(4) = 60

and the pre-patch expression accounted for 56, so the arithmetic fix looks
right.

What is not stated is where the missing 4 bytes went.  toksize is passed as
data_len to crypto_krb5_encrypt(), and the rfc3961 encrypt profile places
the checksum at conf_len + data_len:

crypto/krb5/rfc3961_simplified.c:krb5_aead_encrypt() {
	...
	base_len	= krb5->conf_len + data_len;
	...
	cksum_offset	= secure_len;
	if (WARN_ON(cksum_offset + krb5->cksum_len > sg_len))
		return -EFAULT;
	...
}

So with the short toksize, the trailing word written here:

	*xdr++ = htonl(0);			/* token.identities[0].display.len */

sat outside the encrypted region and was then overwritten by the first
4 bytes of the HMAC, producing a truncated/malformed RXGK_Token in the
callback-channel appdata.  Since the token key enctype is fixed to
AES128-CTS-HMAC-SHA1-96 (cksum_len 12), the extra bytes stayed inside the
kzalloc'd buffer, so there was no out-of-bounds access.

Would it help stable backporters to state that this was a silent
protocol-encoding defect with no memory-safety impact and no kernel log
message?

[Severity: Low]
This isn't a bug introduced by this patch, but while the toksize term is
being corrected, is the existing size self-check in
afs_create_yfs_cm_token() able to detect this class of error at all?

Just before the check, the write cursor is thrown away and recomputed from
encsize:

	xdr = encbase + xdr_round_up(encsize);

	if ((unsigned long)xdr - (unsigned long)appdata != adatasize)
		pr_err("Appdata size incorrect %lx != %zx\n",
		       (unsigned long)xdr - (unsigned long)appdata, adatasize);

encsize comes from crypto_krb5_how_much_buffer(..., toksize, &offset) and
adatasize is derived from contsize, which is derived from encsize, so both
operands move together with any error in toksize.  That is presumably why
the check stayed balanced with the pre-patch 4-byte-short token and nothing
was logged.

The check is also non-fatal: execution falls through to

	sg_init_one(&sg, encbase, encsize);
	ret = crypto_krb5_encrypt(token_krb5, aead, &sg, 1, encsize, offset, toksize, false);

and then to publication into server->cm_rxgk_appdata.

Would comparing the post-write cursor against encbase + offset + toksize
and failing be preferable, in the style of the decoder counterpart?

net/rxrpc/rxgk_app.c:rxgk_yfs_decode_ticket() {
	...
	if (WARN_ON((unsigned long)q != (unsigned long)ticket)) {
		ret = -EIO;
		goto error;
	}
	...
}

As it stands, a future field addition or reordering in the token can
silently regenerate the same malformed-token result.  I could not find a
way to overflow the buffer today: keysize (4 + xdr_len_object(key_len))
exactly matches the three K0 writes, and every other emitted field is a
compile-time constant, so this is a hardening observation rather than a
live defect.

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

Re: [PATCH net v9 13/14] rxrpc: fix use-after-free in rxrpc_poke_conn()

[netdev-bot+sashiko]

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

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

- [Low] Commit message mischaracterizes the applied fix: it claims to
  guard rxrpc_poke_conn() "with the same liveness/refcount check the…

Pre-existing issues:
- [High] Residual poke-reference leak after the rxrpc I/O thread exits:
  rxrpc_poke_conn() (net/rxrpc/conn_object.c:34-44) still takes a poke…

--

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

This changes rxrpc_poke_conn() to take the poke reference with
rxrpc_get_connection_maybe() instead of rxrpc_get_connection(), returning
early if the refcount is already zero.  The intent is to stop a connection
being linked into local->conn_attend_q after it has started teardown.

No Link: tags in the commit header.

Two questions below, one about the changelog wording and one about a
pre-existing lifetime issue that this hunk touches.

> 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 description of the applied change accurate?  rxrpc_queue_conn()
gates on the active count and takes no reference at all:

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

and rxrpc_see_connection() only does refcount_read() plus a tracepoint.
The patch instead adds a refcount gate
(rxrpc_get_connection_maybe() is __refcount_inc_not_zero()), which is a
different liveness signal.

Also, does the code achieve "a connection past its last-active point is not
poked/requeued"?  rxrpc_service_connection_reaper() sets active to -1 while
the refcount is still non-zero:

net/rxrpc/conn_object.c:rxrpc_service_connection_reaper() {
	...
		active = 0;
		if (!atomic_try_cmpxchg(&conn->active, &active, -1))
			continue;
	...
		rxrpc_put_connection(conn, rxrpc_conn_put_service_reaped);
}

so such a connection still passes the new refcount test and is queued to
the I/O thread.  Would it be clearer to describe the change as a
refcount_inc_not_zero gate rather than the same check as
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);
>  	}
>  	spin_unlock_irq(&local->lock);

[Severity: High]
This isn't a bug introduced by this patch, but can a poke reference still be
taken after the I/O thread has exited, and if so who drops it?

The only consumer of conn_attend_q is rxrpc_io_thread():

net/rxrpc/io_thread.c:rxrpc_io_thread() {
	...
		while ((conn = list_first_entry_or_null(&conn_attend_q,
						       struct rxrpc_connection,
						       attend_link))) {
			spin_lock_irq(&local->lock);
			list_del_init(&conn->attend_link);
			spin_unlock_irq(&local->lock);
			rxrpc_input_conn_event(conn, NULL);
			rxrpc_put_connection(conn, rxrpc_conn_put_poke);
		}
	...
		if (should_stop)
			break;
	...
	rxrpc_destroy_local(local);
}

After that break, nothing drains conn_attend_q again.  rxrpc_destroy_local()
purges rx_queue and the client conns but never unlinks attend_link or drops
poke refs:

net/rxrpc/local_object.c:rxrpc_destroy_local() {
	local->dead = true;

	mutex_lock(&rxnet->local_mutex);
	hlist_del_init_rcu(&local->link);
	mutex_unlock(&rxnet->local_mutex);

	rxrpc_clean_up_local_conns(local);
	...
	rxrpc_purge_queue(&local->rx_queue);
	rxrpc_purge_client_connections(local);
}

The final-ACK timer armed in rxrpc_disconnect_client_call() has no reference
held for it:

net/rxrpc/conn_client.c:rxrpc_disconnect_client_call() {
	...
		unsigned long final_ack_at = jiffies + 2;
	...
		rxrpc_reduce_conn_timer(conn, final_ack_at);
}

If that timer expires between the I/O thread's last conn_attend_q re-check
and rxrpc_clean_up_local_conns()'s
rxrpc_put_connection(conn, rxrpc_conn_put_local_dead), the idle/bundle refs
are still held, so rxrpc_get_connection_maybe() succeeds and the connection
is queued with a poke ref that nothing will ever drop.

Does conn->ref then never reach zero, leaving conn->proc_link on
rxnet->conn_proc_list, rxnet->nr_conns undecremented and the rxrpc_local
reference held?  At netns exit that would hit

net/rxrpc/conn_object.c:rxrpc_destroy_all_connections() {
	ASSERT(list_empty(&rxnet->conn_proc_list));
	...
	wait_var_event(&rxnet->nr_conns, !atomic_read(&rxnet->nr_conns));
}

either the ASSERT or an indefinite wait on the cleanup_net worker.

Would closing this need a reference held for the armed conn->timer with a
synchronous cancel before the last put, or explicit unlinking of
conn_attend_q entries when the I/O thread exits?  The refcount-only gate
added here does not appear to cover it.

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

Re: [PATCH net v9 14/14] rxrpc: Take write lock when publishing the initial RxGK key

[netdev-bot+sashiko]

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

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

- [Medium] The new critical section does not cover conn->security_ix /
  conn->security_level.

Pre-existing issues:
- [High] Publish-before-validate leak of the transport key context.
- [High] Wrong array index / lost reference in the rxgk_rekey() non-crank
  slow path.
- [Medium] Unsynchronized read-modify-write of gk->bytes_remaining.

--

rxrpc: Take write lock when publishing the initial RxGK key

This adds write_lock(&conn->security_use_lock) around the initial
publication of conn->rxgk.enctype and conn->rxgk.keys[] in
rxgk_init_connection_security(), so that the initial publication uses the
same lock that rxgk_rekey() uses and that rxgk_get_key() reads under.

A few questions about the placement of the new critical section and about
some neighbouring code it touches.

> diff --git a/net/rxrpc/rxgk.c b/net/rxrpc/rxgk.c
> index 77a67ace1d248..b49221f57f677 100644
> --- a/net/rxrpc/rxgk.c
> +++ b/net/rxrpc/rxgk.c
> @@ -251,8 +251,10 @@ static int rxgk_init_connection_security(struct rxrpc_connection *conn,
>  					 GFP_NOFS);
>  	if (IS_ERR(gk))
>  		return PTR_ERR(gk);
> +	write_lock(&conn->security_use_lock);

[Severity: Medium]
Should conn->security_ix and conn->security_level be published inside this
same critical section?  They are still stored with plain writes earlier in
rxgk_init_connection_security():

	conn->security_ix = token->security_index;
	conn->security_level = token->rxgk->level;

and rxgk_alloc_txbuf() reads the level with no lock at all, and before it
calls rxgk_get_key():

	switch (call->conn->security_level) {
	default:
		alloc = umin(remain, RXRPC_JUMBO_DATALEN);
		return rxrpc_alloc_data_txbuf(call, alloc, 1, gfp);

This is the path the changelog describes.  The second sender does the plain
load of conn->state in rxrpc_send_data():

	if (call->conn->state == RXRPC_CONN_CLIENT_UNSECURED) {
		ret = rxrpc_init_client_conn_security(call->conn);

sees RXRPC_CONN_CLIENT, skips initialisation and goes straight into
rxgk_alloc_txbuf().  Since the pre-init value of conn->security_level comes
from rxrpc_alloc_client_connection():

	conn->security_level	= bundle->security_level;

which traces back to rx->min_sec_level and need not equal
token->rxgk->level, can that sender read the stale level on a weakly
ordered CPU, take the default arm above and allocate a txbuf with no
crypto_header/sec_header reservation, while rxgk_secure_packet() then runs
the AUTH/ENCRYPT path on it (rxgk_secure_packet_encrypted() writing the
rxgk_header over payload, crypto_krb5_encrypt() failing with -EFAULT and
the call aborting)?

The read of conn->security_level in rxgk_secure_packet() itself does happen
after rxgk_get_key()'s read_lock acquire, so that one now pairs with the new
write_unlock; the pre-lock read in rxgk_alloc_txbuf() looks like the
remaining gap.

>  	conn->rxgk.enctype = gk->krb5->etype;
>  	conn->rxgk.keys[gk->key_number & 3] = gk;
> +	write_unlock(&conn->security_use_lock);
>  
>  	switch (conn->security_level) {
>  	case RXRPC_SECURITY_PLAIN:

[Severity: High]
This isn't a bug introduced by this patch (it goes back to 9d1d2b59341f),
but the new critical section is placed before the security_level validation,
so does the reject path leak the rxgk_context?

The store into conn->rxgk.keys[] happens first, then the switch can fail:

	switch (conn->security_level) {
	case RXRPC_SECURITY_PLAIN:
	case RXRPC_SECURITY_AUTH:
	case RXRPC_SECURITY_ENCRYPT:
		break;
	default:
		ret = -EKEYREJECTED;
		goto error;
	}

The error path neither clears the slot nor calls rxgk_put(), and unlike
rxgk_rekey() there is no WARN_ON()/eviction of an occupant already in the
slot.

The reject arm looks user-reachable.  rxrpc_preparse_xdr_yfs_rxgk() accepts
a level of -1:

	token->rxgk->level	= tmp = xdr_dec64(xdr + 2 * 2);
	if (tmp < -1LL || tmp > RXRPC_SECURITY_ENCRYPT)
		goto reject_token;

and struct rxgk_key::level is s8, so -1 widens to 0xFFFFFFFF in the u32
conn->security_level and falls into the default arm.
rxgk_generate_transport_key() still succeeds for that level, so a full
context plus an AEAD transform is installed before the rejection.

Is that then repeatable?  rxrpc_init_client_conn_security() only promotes
the state on success:

		ret = conn->security->init_connection_security(conn, token);
		if (ret == 0) {
			spin_lock_irq(&conn->state_lock);
			if (conn->state == RXRPC_CONN_CLIENT_UNSECURED)
				conn->state = RXRPC_CONN_CLIENT;

and rxrpc_may_reuse_conn() lets the next call reuse it:

	if ((conn->state != RXRPC_CONN_CLIENT_UNSECURED &&
	     conn->state != RXRPC_CONN_CLIENT) ||

so every subsequent sendmsg() re-enters the initialiser and the store into
conn->rxgk.keys[] drops the array's only reference to the previous context.
rxgk_clear() only puts whatever pointers are still in the array at teardown.

The service side looks similar: rxrpc_do_process_connection() discards the
return value of rxrpc_process_event(), the connection stays
RXRPC_CONN_SERVICE_CHALLENGING, and a further RESPONSE re-enters the
initialiser.  rxgk_verify_response() also overwrites conn->key with no
key_put():

	conn->key = key;
	key = NULL;

Would it make sense to move the new write_lock section below the
security_level validation, and to rxgk_put() the evicted occupant the way
rxgk_rekey() handles its dead key?

[Severity: Medium]
This is a pre-existing issue and the changelog already says so ("That report
is on the key context's byte counter rather than the initial publication"),
but should the counter in the quoted KCSAN splat also be synchronised?

rxgk_get_key() reads it under the read lock to decide whether to rekey:

		if (time_after(jiffies, gk->expiry) ||
		    gk->bytes_remaining < 0) {
			set_bit(RXGK_TK_NEEDS_REKEY, &gk->flags);
			goto slow_path;
		}

while both rxgk_secure_packet_integrity() and
rxgk_secure_packet_encrypted() do the read-modify-write with no lock held,
after rxgk_get_key() has already dropped the read lock:

		gk->bytes_remaining -= ret;

With two senders on one connection, can decrements be lost (and the 64-bit
value torn on 32-bit), so that the byte lifetime set up in
rxgk_generate_transport_key() from key->bytelife is exceeded and the
bytes_remaining < 0 rekey trigger is delayed?  Making the counter atomic, or
updating it under the same lock the reader uses, would close that.

[Severity: High]
Not something this patch changes, but while looking at the matching locking
in rxgk_rekey(): does the non-crank generate_key path store the new context
at the wrong index and drop a reference?

When rxgk_rekey() is entered for key_number == current_key - 1 and that slot
is empty, it jumps to generate_key with crank still false:

	gk = conn->rxgk.keys[key_number & mask];
	if (!gk)
		goto generate_key;

The context is then derived for key_number, but installed into the current
slot:

	write_lock(&conn->security_use_lock);
	if (crank) {
		current_key++;
		conn->rxgk.key_number = current_key;
		dead = conn->rxgk.keys[(current_key - 2) & mask];
		conn->rxgk.keys[(current_key - 2) & mask] = NULL;
	}
	conn->rxgk.keys[current_key & mask] = gk;

The eviction into dead only runs when crank is true, so does the previous
current-key context lose its only reference here, leaking an rxgk_context
plus its AEAD/shash transforms?  And since the installed context's
gk->key_number differs from conn->rxgk.key_number, does rxgk_secure_packet()
then stamp the wrong number on transmits:

	txb->cksum = htons(gk->key_number);

The selector is peer-controlled.  rxgk_verify_packet() passes the wire value
in:

	u16 key_number = sp->hdr.cksum;
	...
	gk = rxgk_get_key(call->conn, &key_number);

On a connection that has never rekeyed (conn->rxgk.key_number == 0, only
keys[0] populated), a DATA packet with cksum == 0xFFFF matches:

		else if (*specific_key_number == (u16)(current_key - 1))
			key_number = current_key - 1;

giving key_number == UINT_MAX, so keys[UINT_MAX & 3] is keys[3], which is
NULL, leading to slow_path and rxgk_rekey() taking the branch above and
storing at index 0.  keys[3] stays NULL, so can a peer repeat this for every
packet?

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

Re: [PATCH net v9 01/14] afs: Fix lack of loop around sendmsg() to rxrpc

[David Howells]

David Laight <david.laight.linux@gmail.com> wrote:

> > +	do {
> > +		ret = rxrpc_kernel_send_data(call->net->socket, rxcall, &msg,
> > +					     msg_data_left(&msg),
> > +					     afs_notify_end_request_tx);
> > +		if (ret < 0)
> > +			goto error_do_abort;
> > +	} while (msg_data_left(&msg) > 0);
> 
> Is there any reason you didn't change rxrpc_kernel_send_data() instead?

My thought was to keep rxrpc_kernel_send_data() operating reasonably similarly
to sendmsg(), but I think you're right - that's a better way to do this since
I don't think there's a circumstance that I wouldn't then loop around it.

David