Re: [PATCH net v9 09/14] rxrpc: Fix generation of notifications after call completion

[netdev-bot+sashiko@kernel.org]

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