* [PATCH net v9 01/14] afs: Fix lack of loop around sendmsg() to rxrpc
2026-09-07 11:37 [PATCH net v9 00/14] rxrpc: Miscellaneous fixes David Howells
@ 2026-09-07 11:37 ` David Howells
2026-09-07 13:06 ` David Laight
2026-09-10 10:18 ` David Howells
2026-09-07 11:37 ` [PATCH net v9 02/14] afs: Fix afs to abort the rxrpc call on send error David Howells
` (12 subsequent siblings)
13 siblings, 2 replies; 29+ messages in thread
From: David Howells @ 2026-09-07 11:37 UTC (permalink / raw)
To: netdev
Cc: David Howells, Marc Dionne, Jakub Kicinski, David S. Miller,
Eric Dumazet, Paolo Abeni, Simon Horman, linux-afs, linux-kernel,
stable
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]");
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH net v9 01/14] afs: Fix lack of loop around sendmsg() to rxrpc
2026-09-07 11:37 ` [PATCH net v9 01/14] afs: Fix lack of loop around sendmsg() to rxrpc David Howells
@ 2026-09-07 13:06 ` David Laight
2026-09-10 10:18 ` David Howells
1 sibling, 0 replies; 29+ messages in thread
From: David Laight @ 2026-09-07 13:06 UTC (permalink / raw)
To: David Howells
Cc: netdev, Marc Dionne, Jakub Kicinski, David S. Miller,
Eric Dumazet, Paolo Abeni, Simon Horman, linux-afs, linux-kernel,
stable
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]");
>
>
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH net v9 01/14] afs: Fix lack of loop around sendmsg() to rxrpc
2026-09-07 11:37 ` [PATCH net v9 01/14] afs: Fix lack of loop around sendmsg() to rxrpc David Howells
2026-09-07 13:06 ` David Laight
@ 2026-09-10 10:18 ` David Howells
1 sibling, 0 replies; 29+ messages in thread
From: David Howells @ 2026-09-10 10:18 UTC (permalink / raw)
To: David Laight
Cc: dhowells, netdev, Marc Dionne, Jakub Kicinski, David S. Miller,
Eric Dumazet, Paolo Abeni, Simon Horman, linux-afs, linux-kernel,
stable
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH net v9 02/14] afs: Fix afs to abort the rxrpc call on send error
2026-09-07 11:37 [PATCH net v9 00/14] rxrpc: Miscellaneous fixes David Howells
2026-09-07 11:37 ` [PATCH net v9 01/14] afs: Fix lack of loop around sendmsg() to rxrpc David Howells
@ 2026-09-07 11:37 ` David Howells
2026-09-08 20:49 ` netdev-bot+sashiko
2026-09-07 11:37 ` [PATCH net v9 03/14] rxrpc: Fix lack of loop around reply send in rxperf server David Howells
` (11 subsequent siblings)
13 siblings, 1 reply; 29+ messages in thread
From: David Howells @ 2026-09-07 11:37 UTC (permalink / raw)
To: netdev
Cc: David Howells, Marc Dionne, Jakub Kicinski, David S. Miller,
Eric Dumazet, Paolo Abeni, Simon Horman, linux-afs, linux-kernel,
stable
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 */ \
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH net v9 02/14] afs: Fix afs to abort the rxrpc call on send error
2026-09-07 11:37 ` [PATCH net v9 02/14] afs: Fix afs to abort the rxrpc call on send error David Howells
@ 2026-09-08 20:49 ` netdev-bot+sashiko
0 siblings, 0 replies; 29+ messages in thread
From: netdev-bot+sashiko @ 2026-09-08 20:49 UTC (permalink / raw)
To: dhowells
Cc: netdev, marc.dionne, kuba, davem, edumazet, pabeni, horms,
linux-afs, linux-kernel, nicoyip.dev, stable
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH net v9 03/14] rxrpc: Fix lack of loop around reply send in rxperf server
2026-09-07 11:37 [PATCH net v9 00/14] rxrpc: Miscellaneous fixes David Howells
2026-09-07 11:37 ` [PATCH net v9 01/14] afs: Fix lack of loop around sendmsg() to rxrpc David Howells
2026-09-07 11:37 ` [PATCH net v9 02/14] afs: Fix afs to abort the rxrpc call on send error David Howells
@ 2026-09-07 11:37 ` David Howells
2026-09-08 20:49 ` netdev-bot+sashiko
2026-09-07 11:37 ` [PATCH net v9 04/14] rxrpc: Fix sendmsg to not return an error if last packet queued David Howells
` (10 subsequent siblings)
13 siblings, 1 reply; 29+ messages in thread
From: David Howells @ 2026-09-07 11:37 UTC (permalink / raw)
To: netdev
Cc: David Howells, Marc Dionne, Jakub Kicinski, David S. Miller,
Eric Dumazet, Paolo Abeni, Simon Horman, linux-afs, linux-kernel,
stable
Fix rxperf_process_call() to loop around when sending the magic cookie in
case of a short send. Also make it always try to abort the call on send
error.
Fixes: 75bfdbf2fca3 ("rxrpc: Implement an in-kernel rxperf server for testing purposes")
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@kernel.org
---
include/trace/events/rxrpc.h | 2 +-
net/rxrpc/rxperf.c | 15 +++++++++------
2 files changed, 10 insertions(+), 7 deletions(-)
diff --git a/include/trace/events/rxrpc.h b/include/trace/events/rxrpc.h
index 554dfb777b93..56dc9b614071 100644
--- a/include/trace/events/rxrpc.h
+++ b/include/trace/events/rxrpc.h
@@ -28,8 +28,8 @@
EM(afs_abort_unsupported_sec_class, "afs-unsup-sec-class") \
/* rxperf errors */ \
EM(rxperf_abort_general_error, "rxperf-error") \
- EM(rxperf_abort_oom, "rxperf-oom") \
EM(rxperf_abort_op_not_supported, "rxperf-op-notsupp") \
+ EM(rxperf_abort_send_error, "rxperf-send-error") \
EM(rxperf_abort_unmarshal_error, "rxperf-unmarshal") \
/* RxKAD security errors */ \
EM(rxkad_abort_1_short_check, "rxkad1-short-check") \
diff --git a/net/rxrpc/rxperf.c b/net/rxrpc/rxperf.c
index b8df6d22314d..ebd54222c64e 100644
--- a/net/rxrpc/rxperf.c
+++ b/net/rxrpc/rxperf.c
@@ -538,15 +538,18 @@ static int rxperf_process_call(struct rxperf_call *call)
iov[0].iov_len = len;
iov_iter_kvec(&msg.msg_iter, WRITE, iov, 1, len);
msg.msg_flags = 0;
- n = rxrpc_kernel_send_data(rxperf_socket, call->rxcall, &msg, len,
- rxperf_notify_end_reply_tx);
+ do {
+ n = rxrpc_kernel_send_data(rxperf_socket, call->rxcall,
+ &msg, msg_data_left(&msg),
+ rxperf_notify_end_reply_tx);
+ } while (n >= 0 && msg_data_left(&msg) > 0);
+
if (n >= 0)
return 0; /* Success */
- if (n == -ENOMEM)
- rxrpc_kernel_abort_call(rxperf_socket, call->rxcall,
- RXGEN_SS_MARSHAL, -ENOMEM,
- rxperf_abort_oom);
+ rxrpc_kernel_abort_call(rxperf_socket, call->rxcall,
+ RXGEN_SS_MARSHAL, n,
+ rxperf_abort_send_error);
return n;
}
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH net v9 03/14] rxrpc: Fix lack of loop around reply send in rxperf server
2026-09-07 11:37 ` [PATCH net v9 03/14] rxrpc: Fix lack of loop around reply send in rxperf server David Howells
@ 2026-09-08 20:49 ` netdev-bot+sashiko
0 siblings, 0 replies; 29+ messages in thread
From: netdev-bot+sashiko @ 2026-09-08 20:49 UTC (permalink / raw)
To: dhowells
Cc: netdev, marc.dionne, kuba, davem, edumazet, pabeni, horms,
linux-afs, linux-kernel, nicoyip.dev, stable
Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:
Critical: 0 · High: 0 · Medium: 0 · Low: 1
- [Low] Claimed infinite loop in the new `do { ... } while (n >= 0 &&
msg_data_left(&msg) > 0)` retry loop in rxperf_process_call()…
--
rxrpc: Fix lack of loop around reply send in rxperf server
This wraps the magic-cookie send in rxperf_process_call() in a loop so a
short send is retried, and changes the error path to always propose an
abort with the send error rather than only on -ENOMEM. The
rxperf_abort_oom trace enum is replaced by rxperf_abort_send_error.
Link: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260824091645.415423-1-dhowells%40redhat.com
> diff --git a/net/rxrpc/rxperf.c b/net/rxrpc/rxperf.c
> index b8df6d22314d6..ebd54222c64ef 100644
> --- a/net/rxrpc/rxperf.c
> +++ b/net/rxrpc/rxperf.c
> @@ -538,15 +538,18 @@ static int rxperf_process_call(struct rxperf_call *call)
> iov[0].iov_len = len;
> iov_iter_kvec(&msg.msg_iter, WRITE, iov, 1, len);
> msg.msg_flags = 0;
> - n = rxrpc_kernel_send_data(rxperf_socket, call->rxcall, &msg, len,
> - rxperf_notify_end_reply_tx);
> + do {
> + n = rxrpc_kernel_send_data(rxperf_socket, call->rxcall,
> + &msg, msg_data_left(&msg),
> + rxperf_notify_end_reply_tx);
> + } while (n >= 0 && msg_data_left(&msg) > 0);
[Severity: Low]
This isn't a bug as far as I could tell, but should the new loop also treat
a zero return as a failure the way the reply-payload loop just above it in
rxperf_process_call() does?
The earlier loop in the same function bails out explicitly:
n = rxrpc_kernel_send_data(rxperf_socket, call->rxcall, &msg,
len, rxperf_notify_end_reply_tx);
if (n < 0)
return n;
if (n == 0)
return -EIO;
whereas the new loop counts n == 0 as progress and re-enters
rxrpc_kernel_send_data() while msg_data_left(&msg) is still non-zero.
I tried to construct a path where rxrpc_kernel_send_data() returns exactly 0
with the four cookie bytes still pending and could not find one in this tree.
A positive return is the number of bytes copied out of msg->msg_iter, every
error path in rxrpc_send_data() returns a strictly negative value, and the
only routes to a zero return are:
net/rxrpc/sendmsg.c:rxrpc_kernel_send_data() {
...
if (ret == -ESHUTDOWN)
ret = call->error;
...
}
and the call_terminated path returning call->error, both of which need
call->error == 0, i.e. RXRPC_CALL_SUCCEEDED. For a service call that state
is only reached from RXRPC_CALL_SERVER_AWAIT_ACK in
rxrpc_end_tx_phase()->rxrpc_call_completed(), and AWAIT_ACK is only entered
by rxrpc_close_tx_phase() once a txb carrying RXRPC_LAST_PACKET has been
transmitted, which itself requires the caller's iterator to have been fully
consumed. So the two conditions look mutually exclusive and the loop
terminates today.
Given that, would adding the same zero-progress escape make the new loop
match its sibling and not depend on that implicit state-machine property?
do {
n = rxrpc_kernel_send_data(rxperf_socket, call->rxcall,
&msg, msg_data_left(&msg),
rxperf_notify_end_reply_tx);
if (n == 0) {
n = -EIO;
break;
}
} while (n > 0 && msg_data_left(&msg) > 0);
For reference, the later patch "rxrpc: Fix sendmsg to not return an error if
last packet queued" documents the return contract as the number of bytes
buffered, which can be 0 when the iterator holds 0 bytes; that is consistent
with the loop terminating here. rxperf.c is not touched again later in the
series, so the loop stays as introduced.
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260907113743.1453210-1-dhowells%40redhat.com
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH net v9 04/14] rxrpc: Fix sendmsg to not return an error if last packet queued
2026-09-07 11:37 [PATCH net v9 00/14] rxrpc: Miscellaneous fixes David Howells
` (2 preceding siblings ...)
2026-09-07 11:37 ` [PATCH net v9 03/14] rxrpc: Fix lack of loop around reply send in rxperf server David Howells
@ 2026-09-07 11:37 ` David Howells
2026-09-08 20:49 ` netdev-bot+sashiko
2026-09-07 11:37 ` [PATCH net v9 05/14] rxrpc: Fix sendmsg length David Howells
` (9 subsequent siblings)
13 siblings, 1 reply; 29+ messages in thread
From: David Howells @ 2026-09-07 11:37 UTC (permalink / raw)
To: netdev
Cc: David Howells, Marc Dionne, Jakub Kicinski, David S. Miller,
Eric Dumazet, Paolo Abeni, Simon Horman, linux-afs, linux-kernel,
Jeffrey Altman, stable
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,
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH net v9 04/14] rxrpc: Fix sendmsg to not return an error if last packet queued
2026-09-07 11:37 ` [PATCH net v9 04/14] rxrpc: Fix sendmsg to not return an error if last packet queued David Howells
@ 2026-09-08 20:49 ` netdev-bot+sashiko
0 siblings, 0 replies; 29+ messages in thread
From: netdev-bot+sashiko @ 2026-09-08 20:49 UTC (permalink / raw)
To: dhowells
Cc: netdev, marc.dionne, kuba, davem, edumazet, pabeni, horms,
linux-afs, linux-kernel, nicoyip.dev, stable
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH net v9 05/14] rxrpc: Fix sendmsg length
2026-09-07 11:37 [PATCH net v9 00/14] rxrpc: Miscellaneous fixes David Howells
` (3 preceding siblings ...)
2026-09-07 11:37 ` [PATCH net v9 04/14] rxrpc: Fix sendmsg to not return an error if last packet queued David Howells
@ 2026-09-07 11:37 ` David Howells
2026-09-08 20:49 ` netdev-bot+sashiko
2026-09-07 11:37 ` [PATCH net v9 06/14] rxrpc: Fix packet encryption error handling David Howells
` (8 subsequent siblings)
13 siblings, 1 reply; 29+ messages in thread
From: David Howells @ 2026-09-07 11:37 UTC (permalink / raw)
To: netdev
Cc: David Howells, Marc Dionne, Jakub Kicinski, David S. Miller,
Eric Dumazet, Paolo Abeni, Simon Horman, linux-afs, linux-kernel,
Jeffrey Altman, stable
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;
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH net v9 05/14] rxrpc: Fix sendmsg length
2026-09-07 11:37 ` [PATCH net v9 05/14] rxrpc: Fix sendmsg length David Howells
@ 2026-09-08 20:49 ` netdev-bot+sashiko
0 siblings, 0 replies; 29+ messages in thread
From: netdev-bot+sashiko @ 2026-09-08 20:49 UTC (permalink / raw)
To: dhowells
Cc: netdev, marc.dionne, kuba, davem, edumazet, pabeni, horms,
linux-afs, linux-kernel, nicoyip.dev, stable
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH net v9 06/14] rxrpc: Fix packet encryption error handling
2026-09-07 11:37 [PATCH net v9 00/14] rxrpc: Miscellaneous fixes David Howells
` (4 preceding siblings ...)
2026-09-07 11:37 ` [PATCH net v9 05/14] rxrpc: Fix sendmsg length David Howells
@ 2026-09-07 11:37 ` David Howells
2026-09-08 20:49 ` netdev-bot+sashiko
2026-09-07 11:37 ` [PATCH net v9 07/14] rxrpc: Fix update of call->tx_pending without holding lock David Howells
` (7 subsequent siblings)
13 siblings, 1 reply; 29+ messages in thread
From: David Howells @ 2026-09-07 11:37 UTC (permalink / raw)
To: netdev
Cc: David Howells, Marc Dionne, Jakub Kicinski, David S. Miller,
Eric Dumazet, Paolo Abeni, Simon Horman, linux-afs, linux-kernel,
stable
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) &&
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH net v9 06/14] rxrpc: Fix packet encryption error handling
2026-09-07 11:37 ` [PATCH net v9 06/14] rxrpc: Fix packet encryption error handling David Howells
@ 2026-09-08 20:49 ` netdev-bot+sashiko
0 siblings, 0 replies; 29+ messages in thread
From: netdev-bot+sashiko @ 2026-09-08 20:49 UTC (permalink / raw)
To: dhowells
Cc: netdev, marc.dionne, kuba, davem, edumazet, pabeni, horms,
linux-afs, linux-kernel, nicoyip.dev, stable
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH net v9 07/14] rxrpc: Fix update of call->tx_pending without holding lock
2026-09-07 11:37 [PATCH net v9 00/14] rxrpc: Miscellaneous fixes David Howells
` (5 preceding siblings ...)
2026-09-07 11:37 ` [PATCH net v9 06/14] rxrpc: Fix packet encryption error handling David Howells
@ 2026-09-07 11:37 ` David Howells
2026-09-08 20:49 ` netdev-bot+sashiko
2026-09-07 11:37 ` [PATCH net v9 08/14] rxrpc: Fix double IRQ enablement David Howells
` (6 subsequent siblings)
13 siblings, 1 reply; 29+ messages in thread
From: David Howells @ 2026-09-07 11:37 UTC (permalink / raw)
To: netdev
Cc: David Howells, Marc Dionne, Jakub Kicinski, David S. Miller,
Eric Dumazet, Paolo Abeni, Simon Horman, linux-afs, linux-kernel,
stable
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;
}
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH net v9 07/14] rxrpc: Fix update of call->tx_pending without holding lock
2026-09-07 11:37 ` [PATCH net v9 07/14] rxrpc: Fix update of call->tx_pending without holding lock David Howells
@ 2026-09-08 20:49 ` netdev-bot+sashiko
0 siblings, 0 replies; 29+ messages in thread
From: netdev-bot+sashiko @ 2026-09-08 20:49 UTC (permalink / raw)
To: dhowells
Cc: netdev, marc.dionne, kuba, davem, edumazet, pabeni, horms,
linux-afs, linux-kernel, nicoyip.dev, stable
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH net v9 08/14] rxrpc: Fix double IRQ enablement
2026-09-07 11:37 [PATCH net v9 00/14] rxrpc: Miscellaneous fixes David Howells
` (6 preceding siblings ...)
2026-09-07 11:37 ` [PATCH net v9 07/14] rxrpc: Fix update of call->tx_pending without holding lock David Howells
@ 2026-09-07 11:37 ` David Howells
2026-09-07 11:37 ` [PATCH net v9 09/14] rxrpc: Fix generation of notifications after call completion David Howells
` (5 subsequent siblings)
13 siblings, 0 replies; 29+ messages in thread
From: David Howells @ 2026-09-07 11:37 UTC (permalink / raw)
To: netdev
Cc: David Howells, Marc Dionne, Jakub Kicinski, David S. Miller,
Eric Dumazet, Paolo Abeni, Simon Horman, linux-afs, linux-kernel,
stable
rxrpc_notify_socket() explicitly disables and then re-enables IRQs, but one
of its call chains (rxrpc_input_queue_data() -> rxrpc_end_rx_phase() ->
rxrpc_call_completed() -> rxrpc_set_call_completion()) has IRQs disabled
around it.
Fix this by making rxrpc_notify_socket() use irqsave spinlocks.
Fixes: a2ea9a907260 ("rxrpc: Use irq-disabling spinlocks between app and I/O thread")
Link: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260812110129.979970-6-dhowells@redhat.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@kernel.org
---
net/rxrpc/recvmsg.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/net/rxrpc/recvmsg.c b/net/rxrpc/recvmsg.c
index efcba4b2e74f..56fa324d0962 100644
--- a/net/rxrpc/recvmsg.c
+++ b/net/rxrpc/recvmsg.c
@@ -24,6 +24,7 @@ void rxrpc_notify_socket(struct rxrpc_call *call)
{
struct rxrpc_sock *rx;
struct sock *sk;
+ unsigned long flags;
_enter("%d", call->debug_id);
@@ -38,16 +39,16 @@ void rxrpc_notify_socket(struct rxrpc_call *call)
sk = &rx->sk;
if (rx && sk->sk_state < RXRPC_CLOSE) {
if (call->notify_rx) {
- spin_lock_irq(&call->notify_lock);
+ spin_lock_irqsave(&call->notify_lock, flags);
call->notify_rx(sk, call, call->user_call_ID);
- spin_unlock_irq(&call->notify_lock);
+ spin_unlock_irqrestore(&call->notify_lock, flags);
} else {
- spin_lock_irq(&rx->recvmsg_lock);
+ spin_lock_irqsave(&rx->recvmsg_lock, flags);
if (list_empty(&call->recvmsg_link)) {
rxrpc_get_call(call, rxrpc_call_get_notify_socket);
list_add_tail(&call->recvmsg_link, &rx->recvmsg_q);
}
- spin_unlock_irq(&rx->recvmsg_lock);
+ spin_unlock_irqrestore(&rx->recvmsg_lock, flags);
if (!sock_flag(sk, SOCK_DEAD)) {
_debug("call %ps", sk->sk_data_ready);
^ permalink raw reply [flat|nested] 29+ messages in thread* [PATCH net v9 09/14] rxrpc: Fix generation of notifications after call completion
2026-09-07 11:37 [PATCH net v9 00/14] rxrpc: Miscellaneous fixes David Howells
` (7 preceding siblings ...)
2026-09-07 11:37 ` [PATCH net v9 08/14] rxrpc: Fix double IRQ enablement David Howells
@ 2026-09-07 11:37 ` David Howells
2026-09-08 20:49 ` netdev-bot+sashiko
2026-09-07 11:37 ` [PATCH net v9 10/14] rxrpc: Fix RxGK key parser to check enctype is supported David Howells
` (4 subsequent siblings)
13 siblings, 1 reply; 29+ messages in thread
From: David Howells @ 2026-09-07 11:37 UTC (permalink / raw)
To: netdev
Cc: David Howells, Marc Dionne, Jakub Kicinski, David S. Miller,
Eric Dumazet, Paolo Abeni, Simon Horman, linux-afs, linux-kernel,
stable
AF_RXRPC may generate a notification to the application after a call has
completed because it generates one notification when
rxrpc_input_split_jumbo() queues the final packet and completes the call
and then generates another when rxrpc_input_split_jumbo() does the
aggregated data receive notification at the end of the function.
This might cause the AFS filesystem to malfunction because it tries to
queue the afs_call for processing an extra time. Most of the time this
happens quickly enough that the second queue_work skips, but sometimes this
means that the call work may happen a second time with implications for
afs_call lifetime management.
Fix this by:
(1) Create a lighter version of rxrpc_notify_socket() that's just used to
requeue a call for rxrpc_recvmsg() without creating another
notification.
(2) Move rxrpc_notify_socket() to call_state.c and rename it to
__rxrpc_notify_socket().
(3) Create a wrapper called rxrpc_notify_socket() that skips the
notification if a call is completed.
(4) Make rxrpc_set_call_completion() call __rxrpc_notify_socket() to avoid
the skip-if-completed check.
Also remove the comment on rxrpc_notify_socket() that said it added the
call to a dummy queue to prevent further notification.
Fixes: 2d1faf7a0ca3 ("rxrpc: Simplify skbuff accounting in receive path")
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@kernel.org
---
include/trace/events/rxrpc.h | 1 +
net/rxrpc/ar-internal.h | 2 +-
net/rxrpc/call_state.c | 56 +++++++++++++++++++++++++++++++++++-
net/rxrpc/recvmsg.c | 43 +++++++++------------------
4 files changed, 71 insertions(+), 31 deletions(-)
diff --git a/include/trace/events/rxrpc.h b/include/trace/events/rxrpc.h
index a5c92592d8f9..52f8718cf725 100644
--- a/include/trace/events/rxrpc.h
+++ b/include/trace/events/rxrpc.h
@@ -343,6 +343,7 @@
EM(rxrpc_call_see_distribute_error, "SEE dist-err") \
EM(rxrpc_call_see_input, "SEE input ") \
EM(rxrpc_call_see_notify_released, "SEE nfy-rlsd") \
+ EM(rxrpc_call_see_notify_skipped, "SEE nfy-skip") \
EM(rxrpc_call_see_recvmsg, "SEE recvmsg ") \
EM(rxrpc_call_see_recvmsg_requeue, "SEE recv-rqu") \
EM(rxrpc_call_see_recvmsg_requeue_first, "SEE recv-rqF") \
diff --git a/net/rxrpc/ar-internal.h b/net/rxrpc/ar-internal.h
index a6f830c1621f..cb36a709f540 100644
--- a/net/rxrpc/ar-internal.h
+++ b/net/rxrpc/ar-internal.h
@@ -1110,6 +1110,7 @@ static inline bool rxrpc_is_client_call(const struct rxrpc_call *call)
/*
* call_state.c
*/
+void rxrpc_notify_socket(struct rxrpc_call *call);
bool rxrpc_set_call_completion(struct rxrpc_call *call,
enum rxrpc_call_completion compl,
u32 abort_code,
@@ -1442,7 +1443,6 @@ extern const struct seq_operations rxrpc_local_seq_ops;
/*
* recvmsg.c
*/
-void rxrpc_notify_socket(struct rxrpc_call *);
int rxrpc_recvmsg(struct socket *, struct msghdr *, size_t, int);
/*
diff --git a/net/rxrpc/call_state.c b/net/rxrpc/call_state.c
index 6afb54373ebb..364810c65276 100644
--- a/net/rxrpc/call_state.c
+++ b/net/rxrpc/call_state.c
@@ -7,6 +7,60 @@
#include "ar-internal.h"
+/*
+ * Post a call for attention by the socket or kernel service.
+ */
+static void __rxrpc_notify_socket(struct rxrpc_call *call)
+{
+ struct rxrpc_sock *rx;
+ struct sock *sk;
+ unsigned long flags;
+
+ if (test_bit(RXRPC_CALL_RELEASED, &call->flags)) {
+ rxrpc_see_call(call, rxrpc_call_see_notify_released);
+ return;
+ }
+
+ rcu_read_lock();
+
+ rx = rcu_dereference(call->socket);
+ sk = &rx->sk;
+ if (rx && sk->sk_state < RXRPC_CLOSE) {
+ if (call->notify_rx) {
+ spin_lock_irqsave(&call->notify_lock, flags);
+ call->notify_rx(sk, call, call->user_call_ID);
+ spin_unlock_irqrestore(&call->notify_lock, flags);
+ } else {
+ spin_lock_irqsave(&rx->recvmsg_lock, flags);
+ if (list_empty(&call->recvmsg_link)) {
+ rxrpc_get_call(call, rxrpc_call_get_notify_socket);
+ list_add_tail(&call->recvmsg_link, &rx->recvmsg_q);
+ }
+ spin_unlock_irqrestore(&rx->recvmsg_lock, flags);
+
+ if (!sock_flag(sk, SOCK_DEAD)) {
+ _debug("call %ps", sk->sk_data_ready);
+ sk->sk_data_ready(sk);
+ }
+ }
+ }
+
+ rcu_read_unlock();
+}
+
+/*
+ * Post a call for attention by the socket or kernel service.
+ */
+void rxrpc_notify_socket(struct rxrpc_call *call)
+{
+ if (rxrpc_call_is_complete(call)) {
+ rxrpc_see_call(call, rxrpc_call_see_notify_skipped);
+ return;
+ }
+
+ __rxrpc_notify_socket(call);
+}
+
/*
* Transition a call to the complete state.
*/
@@ -25,7 +79,7 @@ bool rxrpc_set_call_completion(struct rxrpc_call *call,
rxrpc_set_call_state(call, RXRPC_CALL_COMPLETE);
trace_rxrpc_call_complete(call);
wake_up(&call->waitq);
- rxrpc_notify_socket(call);
+ __rxrpc_notify_socket(call);
return true;
}
diff --git a/net/rxrpc/recvmsg.c b/net/rxrpc/recvmsg.c
index 56fa324d0962..28b2148b5693 100644
--- a/net/rxrpc/recvmsg.c
+++ b/net/rxrpc/recvmsg.c
@@ -17,14 +17,12 @@
#include "ar-internal.h"
/*
- * Post a call for attention by the socket or kernel service. Further
- * notifications are suppressed by putting recvmsg_link on a dummy queue.
+ * Requeue a call for recvmsg() to pick up.
*/
-void rxrpc_notify_socket(struct rxrpc_call *call)
+static void rxrpc_requeue_call(struct socket *sock, struct rxrpc_call *call)
{
- struct rxrpc_sock *rx;
- struct sock *sk;
- unsigned long flags;
+ struct rxrpc_sock *rx = rxrpc_sk(sock->sk);
+ struct sock *sk = &rx->sk;
_enter("%d", call->debug_id);
@@ -33,31 +31,18 @@ void rxrpc_notify_socket(struct rxrpc_call *call)
return;
}
- rcu_read_lock();
-
- rx = rcu_dereference(call->socket);
- sk = &rx->sk;
- if (rx && sk->sk_state < RXRPC_CLOSE) {
- if (call->notify_rx) {
- spin_lock_irqsave(&call->notify_lock, flags);
- call->notify_rx(sk, call, call->user_call_ID);
- spin_unlock_irqrestore(&call->notify_lock, flags);
- } else {
- spin_lock_irqsave(&rx->recvmsg_lock, flags);
- if (list_empty(&call->recvmsg_link)) {
- rxrpc_get_call(call, rxrpc_call_get_notify_socket);
- list_add_tail(&call->recvmsg_link, &rx->recvmsg_q);
- }
- spin_unlock_irqrestore(&rx->recvmsg_lock, flags);
+ spin_lock_irq(&rx->recvmsg_lock);
+ if (list_empty(&call->recvmsg_link)) {
+ rxrpc_get_call(call, rxrpc_call_get_notify_socket);
+ list_add_tail(&call->recvmsg_link, &rx->recvmsg_q);
+ }
+ spin_unlock_irq(&rx->recvmsg_lock);
- if (!sock_flag(sk, SOCK_DEAD)) {
- _debug("call %ps", sk->sk_data_ready);
- sk->sk_data_ready(sk);
- }
- }
+ if (!sock_flag(sk, SOCK_DEAD)) {
+ _debug("call %ps", sk->sk_data_ready);
+ sk->sk_data_ready(sk);
}
- rcu_read_unlock();
_leave("");
}
@@ -562,7 +547,7 @@ int rxrpc_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
if (!(flags & MSG_PEEK) &&
!skb_queue_empty(&call->recvmsg_queue))
- rxrpc_notify_socket(call);
+ rxrpc_requeue_call(sock, call);
goto not_yet_complete;
call_failed:
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH net v9 09/14] rxrpc: Fix generation of notifications after call completion
2026-09-07 11:37 ` [PATCH net v9 09/14] rxrpc: Fix generation of notifications after call completion David Howells
@ 2026-09-08 20:49 ` netdev-bot+sashiko
0 siblings, 0 replies; 29+ messages in thread
From: netdev-bot+sashiko @ 2026-09-08 20:49 UTC (permalink / raw)
To: dhowells
Cc: netdev, marc.dionne, kuba, davem, edumazet, pabeni, horms,
linux-afs, linux-kernel, nicoyip.dev, stable
Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:
Critical: 0 · High: 0 · Medium: 0 · Low: 2
- [Low] Undisclosed behavioural divergence introduced by the refactor:
the new helper rxrpc_requeue_call() (net/rxrpc/recvmsg.c:22-46), which…
- [Low] Comment vs code mismatch in net/rxrpc/call_state.c: the new
wrapper rxrpc_notify_socket() (line 54) carries a comment that is a…
--
rxrpc: Fix generation of notifications after call completion
The patch stops AF_RXRPC from generating a second notification after a call
has completed by splitting rxrpc_notify_socket() into a skip-if-completed
wrapper plus an unconditional __rxrpc_notify_socket() in call_state.c. The
recvmsg path instead gets a new lightweight rxrpc_requeue_call() helper that
only puts the call back on rx->recvmsg_q.
A couple of questions below about the split.
> diff --git a/net/rxrpc/call_state.c b/net/rxrpc/call_state.c
> index 6afb54373ebbf..364810c652766 100644
> --- a/net/rxrpc/call_state.c
> +++ b/net/rxrpc/call_state.c
> @@ -7,6 +7,60 @@
>
> #include "ar-internal.h"
>
> +/*
> + * Post a call for attention by the socket or kernel service.
> + */
> +static void __rxrpc_notify_socket(struct rxrpc_call *call)
> +{
> + struct rxrpc_sock *rx;
> + struct sock *sk;
> + unsigned long flags;
> +
> + if (test_bit(RXRPC_CALL_RELEASED, &call->flags)) {
> + rxrpc_see_call(call, rxrpc_call_see_notify_released);
> + return;
> + }
> +
> + rcu_read_lock();
> +
> + rx = rcu_dereference(call->socket);
> + sk = &rx->sk;
> + if (rx && sk->sk_state < RXRPC_CLOSE) {
> + if (call->notify_rx) {
> + spin_lock_irqsave(&call->notify_lock, flags);
> + call->notify_rx(sk, call, call->user_call_ID);
> + spin_unlock_irqrestore(&call->notify_lock, flags);
> + } else {
> + spin_lock_irqsave(&rx->recvmsg_lock, flags);
> + if (list_empty(&call->recvmsg_link)) {
> + rxrpc_get_call(call, rxrpc_call_get_notify_socket);
> + list_add_tail(&call->recvmsg_link, &rx->recvmsg_q);
> + }
> + spin_unlock_irqrestore(&rx->recvmsg_lock, flags);
> +
> + if (!sock_flag(sk, SOCK_DEAD)) {
> + _debug("call %ps", sk->sk_data_ready);
> + sk->sk_data_ready(sk);
> + }
> + }
> + }
> +
> + rcu_read_unlock();
> +}
> +
> +/*
> + * Post a call for attention by the socket or kernel service.
> + */
> +void rxrpc_notify_socket(struct rxrpc_call *call)
> +{
> + if (rxrpc_call_is_complete(call)) {
> + rxrpc_see_call(call, rxrpc_call_see_notify_skipped);
> + return;
> + }
> +
> + __rxrpc_notify_socket(call);
> +}
[Severity: Low]
This isn't a bug, but the comment on the new rxrpc_notify_socket() wrapper is
a verbatim copy of the one on __rxrpc_notify_socket(), and the wrapper does
not post anything when rxrpc_call_is_complete(call) is true, it just emits
the rxrpc_call_see_notify_skipped trace and returns.
Since the skip is the only difference between the two functions, and is the
reason rxrpc_set_call_completion() must use the double-underscore variant,
would it be worth mentioning it in the comment? As it stands, a caller that
needs a completed-but-not-yet-drained call re-posted cannot tell from the
comments which variant to pick.
> diff --git a/net/rxrpc/recvmsg.c b/net/rxrpc/recvmsg.c
> index 56fa324d0962e..28b2148b56936 100644
> --- a/net/rxrpc/recvmsg.c
> +++ b/net/rxrpc/recvmsg.c
> @@ -17,14 +17,12 @@
> #include "ar-internal.h"
>
> /*
> - * Post a call for attention by the socket or kernel service. Further
> - * notifications are suppressed by putting recvmsg_link on a dummy queue.
> + * Requeue a call for recvmsg() to pick up.
> */
> -void rxrpc_notify_socket(struct rxrpc_call *call)
> +static void rxrpc_requeue_call(struct socket *sock, struct rxrpc_call *call)
> {
> - struct rxrpc_sock *rx;
> - struct sock *sk;
> - unsigned long flags;
> + struct rxrpc_sock *rx = rxrpc_sk(sock->sk);
> + struct sock *sk = &rx->sk;
>
> _enter("%d", call->debug_id);
>
> @@ -33,31 +31,18 @@ void rxrpc_notify_socket(struct rxrpc_call *call)
> return;
> }
>
> - rcu_read_lock();
> -
> - rx = rcu_dereference(call->socket);
> - sk = &rx->sk;
> - if (rx && sk->sk_state < RXRPC_CLOSE) {
> - if (call->notify_rx) {
> - spin_lock_irqsave(&call->notify_lock, flags);
> - call->notify_rx(sk, call, call->user_call_ID);
> - spin_unlock_irqrestore(&call->notify_lock, flags);
> - } else {
> - spin_lock_irqsave(&rx->recvmsg_lock, flags);
> - if (list_empty(&call->recvmsg_link)) {
> - rxrpc_get_call(call, rxrpc_call_get_notify_socket);
> - list_add_tail(&call->recvmsg_link, &rx->recvmsg_q);
> - }
> - spin_unlock_irqrestore(&rx->recvmsg_lock, flags);
> + spin_lock_irq(&rx->recvmsg_lock);
> + if (list_empty(&call->recvmsg_link)) {
> + rxrpc_get_call(call, rxrpc_call_get_notify_socket);
> + list_add_tail(&call->recvmsg_link, &rx->recvmsg_q);
> + }
> + spin_unlock_irq(&rx->recvmsg_lock);
[Severity: Low]
Is dropping the "rx && sk->sk_state < RXRPC_CLOSE" test intentional here?
rxrpc_requeue_call() keeps only the RXRPC_CALL_RELEASED check, so it will add
the call to rx->recvmsg_q regardless of sk_state, while the other enqueue site
for the same list, __rxrpc_notify_socket() in call_state.c, still requires
sk_state < RXRPC_CLOSE.
The CLOSE transition looks like it was written as a fence for additions to
that list, since rxrpc_shutdown() and rxrpc_release_sock() in af_rxrpc.c set
it under the same lock:
net/rxrpc/af_rxrpc.c:rxrpc_shutdown() {
...
if (sk->sk_state < RXRPC_CLOSE) {
spin_lock_irq(&rx->recvmsg_lock);
sk->sk_state = RXRPC_CLOSE;
sk->sk_shutdown = SHUTDOWN_MASK;
spin_unlock_irq(&rx->recvmsg_lock);
...
}
The requeue in rxrpc_recvmsg() runs after release_sock(&rx->sk), so a
concurrent shutdown(SHUT_RDWR) on another thread of the same socket can flip
sk_state to RXRPC_CLOSE in that window and the call is requeued anyway, with a
fresh rxrpc_call_get_notify_socket ref.
The consequences look bounded: the extra ref is reclaimed when
rxrpc_release_calls_on_socket() drains rx->recvmsg_q, and handing out
already-queued data after shutdown was possible before this patch too, since
rxrpc_recvmsg() has no RXRPC_CLOSE rejection and the fence never removes
entries that are already queued.
Similarly, the call->notify_rx branch is gone from this path. That looks safe
today, since notify_rx is only set for kernel service calls and
__rxrpc_notify_socket() never puts those on rx->recvmsg_q, so every call
rxrpc_recvmsg() takes off that queue has notify_rx == NULL.
Would it be worth either restoring the sk_state test so both enqueue sites
agree, or noting in a comment why the requeue path does not need it?
> - if (!sock_flag(sk, SOCK_DEAD)) {
> - _debug("call %ps", sk->sk_data_ready);
> - sk->sk_data_ready(sk);
> - }
> - }
> + if (!sock_flag(sk, SOCK_DEAD)) {
> + _debug("call %ps", sk->sk_data_ready);
> + sk->sk_data_ready(sk);
> }
>
> - rcu_read_unlock();
> _leave("");
> }
[ ... ]
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260907113743.1453210-1-dhowells%40redhat.com
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH net v9 10/14] rxrpc: Fix RxGK key parser to check enctype is supported
2026-09-07 11:37 [PATCH net v9 00/14] rxrpc: Miscellaneous fixes David Howells
` (8 preceding siblings ...)
2026-09-07 11:37 ` [PATCH net v9 09/14] rxrpc: Fix generation of notifications after call completion David Howells
@ 2026-09-07 11:37 ` David Howells
2026-09-08 20:49 ` netdev-bot+sashiko
2026-09-07 11:37 ` [PATCH net v9 11/14] afs: Fix creation of RxGK CM channel token to have right size David Howells
` (3 subsequent siblings)
13 siblings, 1 reply; 29+ messages in thread
From: David Howells @ 2026-09-07 11:37 UTC (permalink / raw)
To: netdev
Cc: David Howells, Marc Dionne, Jakub Kicinski, David S. Miller,
Eric Dumazet, Paolo Abeni, Simon Horman, linux-afs, linux-kernel,
stable
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);
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH net v9 10/14] rxrpc: Fix RxGK key parser to check enctype is supported
2026-09-07 11:37 ` [PATCH net v9 10/14] rxrpc: Fix RxGK key parser to check enctype is supported David Howells
@ 2026-09-08 20:49 ` netdev-bot+sashiko
0 siblings, 0 replies; 29+ messages in thread
From: netdev-bot+sashiko @ 2026-09-08 20:49 UTC (permalink / raw)
To: dhowells
Cc: netdev, marc.dionne, kuba, davem, edumazet, pabeni, horms,
linux-afs, linux-kernel, nicoyip.dev, stable
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH net v9 11/14] afs: Fix creation of RxGK CM channel token to have right size
2026-09-07 11:37 [PATCH net v9 00/14] rxrpc: Miscellaneous fixes David Howells
` (9 preceding siblings ...)
2026-09-07 11:37 ` [PATCH net v9 10/14] rxrpc: Fix RxGK key parser to check enctype is supported David Howells
@ 2026-09-07 11:37 ` David Howells
2026-09-08 20:49 ` netdev-bot+sashiko
2026-09-07 11:37 ` [PATCH net v9 12/14] afs: Fix lack of setting call->server when doing FS.InlineBulkStatus David Howells
` (2 subsequent siblings)
13 siblings, 1 reply; 29+ messages in thread
From: David Howells @ 2026-09-07 11:37 UTC (permalink / raw)
To: netdev
Cc: David Howells, Marc Dionne, Jakub Kicinski, David S. Miller,
Eric Dumazet, Paolo Abeni, Simon Horman, linux-afs, linux-kernel,
stable
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);
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH net v9 11/14] afs: Fix creation of RxGK CM channel token to have right size
2026-09-07 11:37 ` [PATCH net v9 11/14] afs: Fix creation of RxGK CM channel token to have right size David Howells
@ 2026-09-08 20:49 ` netdev-bot+sashiko
0 siblings, 0 replies; 29+ messages in thread
From: netdev-bot+sashiko @ 2026-09-08 20:49 UTC (permalink / raw)
To: dhowells
Cc: netdev, marc.dionne, kuba, davem, edumazet, pabeni, horms,
linux-afs, linux-kernel, nicoyip.dev, stable
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH net v9 12/14] afs: Fix lack of setting call->server when doing FS.InlineBulkStatus
2026-09-07 11:37 [PATCH net v9 00/14] rxrpc: Miscellaneous fixes David Howells
` (10 preceding siblings ...)
2026-09-07 11:37 ` [PATCH net v9 11/14] afs: Fix creation of RxGK CM channel token to have right size David Howells
@ 2026-09-07 11:37 ` David Howells
2026-09-08 20:49 ` netdev-bot+sashiko
2026-09-07 11:37 ` [PATCH net v9 13/14] rxrpc: fix use-after-free in rxrpc_poke_conn() David Howells
2026-09-07 11:37 ` [PATCH net v9 14/14] rxrpc: Take write lock when publishing the initial RxGK key David Howells
13 siblings, 1 reply; 29+ messages in thread
From: David Howells @ 2026-09-07 11:37 UTC (permalink / raw)
To: netdev
Cc: David Howells, Marc Dionne, Jakub Kicinski, David S. Miller,
Eric Dumazet, Paolo Abeni, Simon Horman, linux-afs, linux-kernel,
Jeffrey Altman, Jarkko Sakkinen, keyrings, stable
Fix afs_make_op_call to set call->server. This will prevent
afs_done_fs_inline_bulk_status() from oopsing if the server does not
support the FS.InlineBulkStatus RPC due to call->server not being set.
Note that this requires afs_make_op_call() to be moved so that it can use
afs_use_server(); the server is un-used by afs_free_server().
Fixes: e49c7b2f6de7 ("afs: Build an abstraction around an "operation" concept")
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: 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: Jarkko Sakkinen <jarkko@kernel.org>
cc: linux-afs@lists.infradead.org
cc: keyrings@vger.kernel.org
cc: stable@kernel.org
---
fs/afs/internal.h | 33 +++++++++++++++++----------------
include/trace/events/afs.h | 1 +
2 files changed, 18 insertions(+), 16 deletions(-)
diff --git a/fs/afs/internal.h b/fs/afs/internal.h
index 290873bac89b..65a0866cd8b8 100644
--- a/fs/afs/internal.h
+++ b/fs/afs/internal.h
@@ -1416,22 +1416,6 @@ static inline void afs_see_call(struct afs_call *call, enum afs_call_trace why)
__builtin_return_address(0));
}
-static inline void afs_make_op_call(struct afs_operation *op, struct afs_call *call,
- gfp_t gfp)
-{
- struct afs_addr_list *alist = op->estate->addresses;
-
- op->call = afs_get_call(call, afs_call_trace_get);
- op->type = call->type;
- call->op = op;
- call->key = op->key;
- call->intr = !(op->flags & AFS_OPERATION_UNINTR);
- call->peer = rxrpc_kernel_get_peer(alist->addrs[op->addr_index].peer);
- call->service_id = op->server->service_id;
- afs_make_call(call, gfp);
- afs_put_call(call);
-}
-
static inline void afs_extract_begin(struct afs_call *call, void *buf, size_t size)
{
call->iov_len = size;
@@ -1763,6 +1747,23 @@ static inline struct inode *AFS_VNODE_TO_I(struct afs_vnode *vnode)
return &vnode->netfs.inode;
}
+static inline void afs_make_op_call(struct afs_operation *op, struct afs_call *call,
+ gfp_t gfp)
+{
+ struct afs_addr_list *alist = op->estate->addresses;
+
+ op->call = afs_get_call(call, afs_call_trace_get);
+ op->type = call->type;
+ call->op = op;
+ call->server = afs_use_server(op->server, false, afs_server_trace_use_call);
+ call->key = op->key;
+ call->intr = !(op->flags & AFS_OPERATION_UNINTR);
+ call->peer = rxrpc_kernel_get_peer(alist->addrs[op->addr_index].peer);
+ call->service_id = op->server->service_id;
+ afs_make_call(call, gfp);
+ afs_put_call(call);
+}
+
/*
* Note that a dentry got changed. We need to set d_fsdata to the data version
* number derived from the result of the operation. It doesn't matter if
diff --git a/include/trace/events/afs.h b/include/trace/events/afs.h
index 1b3c48b5591d..04b0bb682b81 100644
--- a/include/trace/events/afs.h
+++ b/include/trace/events/afs.h
@@ -148,6 +148,7 @@ enum yfs_cm_operation {
EM(afs_server_trace_unuse_slist_isort, "UNU isort") \
EM(afs_server_trace_update, "UPDATE ") \
EM(afs_server_trace_use_by_uuid, "USE uuid ") \
+ EM(afs_server_trace_use_call, "USE call ") \
EM(afs_server_trace_use_cm_call, "USE cm-cl") \
EM(afs_server_trace_use_get_caps, "USE gcaps") \
EM(afs_server_trace_use_give_up_cb, "USE gvupc") \
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH net v9 12/14] afs: Fix lack of setting call->server when doing FS.InlineBulkStatus
2026-09-07 11:37 ` [PATCH net v9 12/14] afs: Fix lack of setting call->server when doing FS.InlineBulkStatus David Howells
@ 2026-09-08 20:49 ` netdev-bot+sashiko
0 siblings, 0 replies; 29+ messages in thread
From: netdev-bot+sashiko @ 2026-09-08 20:49 UTC (permalink / raw)
To: dhowells
Cc: netdev, marc.dionne, kuba, davem, edumazet, pabeni, horms,
linux-afs, linux-kernel, nicoyip.dev, stable
Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider:
Critical: 0 · High: 1 · Medium: 1 · Low: 1
- [High] Use-after-free of struct afs_call via the stale op->call pointer
in afs_wait_for_operation() (fs/afs/fs_operation.c:281-301).
- [Medium] afs_make_op_call() (fs/afs/internal.h:1758) now makes every
fileserver-operation call hold an *active* count on op->server.
- [Low] Commit message names a non-existent function as the release site
of the new server reference.
--
afs: Fix lack of setting call->server when doing FS.InlineBulkStatus
This moves afs_make_op_call() further down fs/afs/internal.h so that it can
call afs_use_server(), and has it set call->server from op->server. A new
afs_server_trace_use_call trace enum is added for the new active count.
Link: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260824091645.415423-1-dhowells%40redhat.com
> Note that this requires afs_make_op_call() to be moved so that it can use
> afs_use_server(); the server is un-used by afs_free_server().
[Severity: Low]
Should this say afs_free_call() rather than afs_free_server()? There is no
afs_free_server() in fs/afs; the server teardown helpers are
afs_server_destroyer(), afs_server_rcu() and __afs_put_server() in
fs/afs/server.c.
The active count taken by the new afs_use_server() in afs_make_op_call()
is dropped on call teardown, in afs_free_call():
fs/afs/rxrpc.c:afs_free_call() {
...
afs_unuse_server_notime(call->net, call->server, afs_server_trace_unuse_call);
kfree(call->request);
...
}
> diff --git a/fs/afs/internal.h b/fs/afs/internal.h
> index 290873bac89b3..65a0866cd8b85 100644
> --- a/fs/afs/internal.h
> +++ b/fs/afs/internal.h
[ ... ]
> @@ -1763,6 +1747,23 @@ static inline struct inode *AFS_VNODE_TO_I(struct afs_vnode *vnode)
> return &vnode->netfs.inode;
> }
>
> +static inline void afs_make_op_call(struct afs_operation *op, struct afs_call *call,
> + gfp_t gfp)
> +{
> + struct afs_addr_list *alist = op->estate->addresses;
> +
> + op->call = afs_get_call(call, afs_call_trace_get);
> + op->type = call->type;
> + call->op = op;
> + call->server = afs_use_server(op->server, false, afs_server_trace_use_call);
[Severity: Medium]
Can this leave an idle afs_server with no garbage collection armed?
Every fileserver operation call now holds an active count on op->server,
and the only counterpart drop is the _notime variant in afs_free_call():
fs/afs/server.c:afs_unuse_server_notime() {
if (atomic_dec_and_test(&server->active)) {
if (test_bit(AFS_SERVER_FL_EXPIRED, &server->flags) ||
READ_ONCE(server->cell->state) >= AFS_CELL_REMOVING)
schedule_work(&server->destroyer);
}
afs_put_server(net, server, reason);
}
Unlike afs_unuse_server(), this never sets server->unuse_time and never
calls afs_set_server_timer(). So if the call's active count happens to be
the last one dropped, the server sits at active == 0 with no
AFS_SERVER_FL_EXPIRED bit, no armed server->timer and no queued destroyer.
In the synchronous path this doesn't arise, because afs_wait_for_operation()
drops the call ref before afs_put_operation() -> afs_put_serverlist(), so
the last active drop goes through the timing-aware afs_unuse_server().
The async read path has the opposite order:
fs/afs/file.c:afs_read_receive() {
op->call = NULL;
call->op = NULL;
afs_put_call(call);
...
afs_end_read(op); /* -> afs_put_operation -> afs_put_serverlist */
}
afs_fetch_data_async_rx() then drops the final call ref afterwards, so
afs_free_call()'s afs_unuse_server_notime() is the last active drop. If the
volume's server list was replaced concurrently (so afs_put_serverlist() drops
the list's last reference), does the afs_server, its endpoint state, address
list and peer bindings then stay in cell->fs_servers until the cell reaches
AFS_CELL_REMOVING or the netns goes away, rather than after
afs_server_gc_delay?
> + call->key = op->key;
> + call->intr = !(op->flags & AFS_OPERATION_UNINTR);
> + call->peer = rxrpc_kernel_get_peer(alist->addrs[op->addr_index].peer);
> + call->service_id = op->server->service_id;
> + afs_make_call(call, gfp);
> + afs_put_call(call);
> +}
> +
[Severity: High]
This assignment of op->call is the only producer for the rotation loop in
afs_wait_for_operation(), and that loop never clears op->call:
fs/afs/fs_operation.c:afs_wait_for_operation() {
while (afs_select_fileserver(op)) {
...
if (op->call) {
afs_wait_for_call_to_complete(op->call);
op->call_abort_code = op->call->abort_code;
op->call_error = op->call->error;
op->call_responded = op->call->responded;
afs_put_call(op->call);
}
}
For a synchronous call, the reference in op->call is the last one
(afs_alloc_flat_call sets ref to 1, afs_get_call() here makes it 2, and the
trailing afs_put_call() above brings it back to 1), so afs_put_call() ->
afs_free_call() -> kfree(call) while op->call still points at it.
Can a second loop iteration then use freed memory? If the issue function
returns without reaching afs_make_op_call(), the if (op->call) test is true
for the stale pointer, and afs_wait_for_call_to_complete() reads call->state,
call->net->socket and call->rxcall, followed by a second afs_put_call() on
the same freed call.
With this patch, that second put also drops a server active count that
afs_free_call() already released via afs_unuse_server_notime(), so does it
underflow server->active/server->ref too?
The early returns reachable on a second iteration look like:
fs/afs/fsclient.c:afs_fs_inline_bulk_status() {
if (test_bit(AFS_SERVER_FL_NO_IBULK, &op->server->flags)) {
afs_op_set_error(op, -ENOTSUPP);
return;
}
That NO_IBULK bit only becomes settable because of this patch, since
afs_done_fs_inline_bulk_status() is its sole setter and previously oopsed on
call->server == NULL:
fs/afs/fsclient.c:afs_done_fs_inline_bulk_status() {
set_bit(AFS_SERVER_FL_NO_IBULK, &call->server->flags);
Two more paths reach the same state: afs_alloc_flat_call() returning NULL
under GFP_NOFS, and the
else
op->call_error = -ENOTSUPP;
branch in afs_wait_for_operation() when the newly selected server has no
matching issue_*_rpc. Getting a second iteration is ordinary control flow -
afs_select_fileserver() returns true again for VBUSY/VOFFLINE, for
RX_CALL_TIMEOUT and for -ETIMEDOUT/-EHOSTUNREACH.
Would clearing op->call before the put fix this, matching what
afs_read_receive() already does with op->call = NULL?
A git diff of fs/afs between this commit and the tip of the series is empty,
so this doesn't look addressed by a later patch in the series.
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260907113743.1453210-1-dhowells%40redhat.com
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH net v9 13/14] rxrpc: fix use-after-free in rxrpc_poke_conn()
2026-09-07 11:37 [PATCH net v9 00/14] rxrpc: Miscellaneous fixes David Howells
` (11 preceding siblings ...)
2026-09-07 11:37 ` [PATCH net v9 12/14] afs: Fix lack of setting call->server when doing FS.InlineBulkStatus David Howells
@ 2026-09-07 11:37 ` David Howells
2026-09-08 20:49 ` netdev-bot+sashiko
2026-09-07 11:37 ` [PATCH net v9 14/14] rxrpc: Take write lock when publishing the initial RxGK key David Howells
13 siblings, 1 reply; 29+ messages in thread
From: David Howells @ 2026-09-07 11:37 UTC (permalink / raw)
To: netdev
Cc: David Howells, Marc Dionne, Jakub Kicinski, David S. Miller,
Eric Dumazet, Paolo Abeni, Simon Horman, linux-afs, linux-kernel,
Seungwon Bae, stable
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);
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH net v9 13/14] rxrpc: fix use-after-free in rxrpc_poke_conn()
2026-09-07 11:37 ` [PATCH net v9 13/14] rxrpc: fix use-after-free in rxrpc_poke_conn() David Howells
@ 2026-09-08 20:49 ` netdev-bot+sashiko
0 siblings, 0 replies; 29+ messages in thread
From: netdev-bot+sashiko @ 2026-09-08 20:49 UTC (permalink / raw)
To: dhowells
Cc: netdev, marc.dionne, kuba, davem, edumazet, pabeni, horms,
linux-afs, linux-kernel, nicoyip.dev, stable
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH net v9 14/14] rxrpc: Take write lock when publishing the initial RxGK key
2026-09-07 11:37 [PATCH net v9 00/14] rxrpc: Miscellaneous fixes David Howells
` (12 preceding siblings ...)
2026-09-07 11:37 ` [PATCH net v9 13/14] rxrpc: fix use-after-free in rxrpc_poke_conn() David Howells
@ 2026-09-07 11:37 ` David Howells
2026-09-08 20:49 ` netdev-bot+sashiko
13 siblings, 1 reply; 29+ messages in thread
From: David Howells @ 2026-09-07 11:37 UTC (permalink / raw)
To: netdev
Cc: David Howells, Marc Dionne, Jakub Kicinski, David S. Miller,
Eric Dumazet, Paolo Abeni, Simon Horman, linux-afs, linux-kernel,
Chengfeng Ye, stable
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:
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH net v9 14/14] rxrpc: Take write lock when publishing the initial RxGK key
2026-09-07 11:37 ` [PATCH net v9 14/14] rxrpc: Take write lock when publishing the initial RxGK key David Howells
@ 2026-09-08 20:49 ` netdev-bot+sashiko
0 siblings, 0 replies; 29+ messages in thread
From: netdev-bot+sashiko @ 2026-09-08 20:49 UTC (permalink / raw)
To: dhowells
Cc: netdev, marc.dionne, kuba, davem, edumazet, pabeni, horms,
linux-afs, linux-kernel, nicoyip.dev, stable
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
^ permalink raw reply [flat|nested] 29+ messages in thread