Re: [PATCH net v10 09/14] rxrpc: Fix generation of notifications after call completion
From: netdev-bot+sashiko@kernel.org
Date: 2026-09-15 22:57:43
Also in:
lkml
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: Critical: 0 · High: 0 · Medium: 1 · Low: 0 - [Medium] Undisclosed behaviour change: the new helper rxrpc_requeue_call() (net/rxrpc/recvmsg.c:24) drops the `sk->sk_state… -- rxrpc: Fix generation of notifications after call completion This splits rxrpc_notify_socket() into __rxrpc_notify_socket() in call_state.c plus a wrapper that skips notification for completed calls, and adds a lighter rxrpc_requeue_call() helper used by rxrpc_recvmsg() to put a call back on rx->recvmsg_q without generating a notification.
quoted hunk ↗ jump to hunk
diff --git a/net/rxrpc/call_state.c b/net/rxrpc/call_state.c index 6afb54373ebbf..52465e88a0440 100644 --- a/net/rxrpc/call_state.c +++ b/net/rxrpc/call_state.c@@ -7,6 +7,61 @@ #include "ar-internal.h" +/* + * Post a call for attention by the socket or kernel service. + */ +static void __rxrpc_notify_socket(struct rxrpc_call *call) +{
[ ... ]
+ rx = rcu_dereference(call->socket);
+ sk = &rx->sk;
+ if (rx && sk->sk_state < RXRPC_CLOSE) {[ ... ]
quoted hunk ↗ jump to hunk
diff --git a/net/rxrpc/recvmsg.c b/net/rxrpc/recvmsg.c index 56fa324d0962e..22afc71ea474f 100644 --- a/net/rxrpc/recvmsg.c +++ b/net/rxrpc/recvmsg.c@@ -17,14 +17,14 @@ #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. We ignore RXRPC_CLOSE, allowing + * recvmsg() to continue picking up calls that are already on the queue if it + * wants to, but no new calls will get added. */ -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 +33,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(""); }
[Severity: Medium]
Should the changelog mention that this changes behaviour for sockets that
have already been shut down?
The code being replaced only queued the call and rang sk_data_ready() when
the socket was still open:
rx = rcu_dereference(call->socket);
sk = &rx->sk;
if (rx && sk->sk_state < RXRPC_CLOSE) {
...
list_add_tail(&call->recvmsg_link, &rx->recvmsg_q);
...
sk->sk_data_ready(sk);
rxrpc_requeue_call() has no sk_state test, while the extracted
__rxrpc_notify_socket() in call_state.c keeps it, so the two paths now
differ.
The gate looks deliberate, since rxrpc_shutdown() in af_rxrpc.c sets the
state under the same lock that guards recvmsg_q:
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);
So after shutdown(fd, SHUT_RDWR) followed by further recvmsg() calls, a
partially-read call with data still in call->recvmsg_queue is now re-added
to rx->recvmsg_q with a rxrpc_call_get_notify_socket ref and sk_data_ready()
is called on the closing socket, where before the requeue was dropped and
the buffered data was never delivered. The extra ref does appear to be
accounted for by rxrpc_release_calls_on_socket() draining rx->recvmsg_q at
close, so this looks like a visible semantic change rather than a leak, but
the patch is cc'd to stable and only the new function comment hints at it.
Also, is the new comment self-consistent? It says "no new calls will get
added" while describing a function whose job is to add the call back onto
rx->recvmsg_q.
quoted hunk ↗ jump to hunk
@@ -562,7 +549,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;
[Severity: Medium] Is item (1) of the changelog accurate for this call site? It describes the helper as requeuing "a call for rxrpc_recvmsg() without creating another notification", but rxrpc_requeue_call() still calls sk->sk_data_ready(sk). What it actually drops is the call->notify_rx dispatch, which is unreachable from rxrpc_recvmsg() anyway since calls with notify_rx set are not put on rx->recvmsg_q. Could the wording be adjusted to say that? -- Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260914151340.3227501-1-dhowells%40redhat.com