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

[netdev-bot+sashiko@kernel.org]

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

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

- [High] rxrpc_send_data() (net/rxrpc/sendmsg.c) leaks call->user_mutex
  on the RXRPC_CALL_TX_NO_MORE early-exit path.

--

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

This moves the release of call->user_mutex down into rxrpc_send_data(),
drops the _dropped_lock indicator parameter, and keeps call->tx_pending set
until the txbuf has been queued.  Both callers no longer unlock the call
mutex themselves.

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

> diff --git a/net/rxrpc/sendmsg.c b/net/rxrpc/sendmsg.c
> index bbb39835ef9e7..b370e440e2fdb 100644
> --- a/net/rxrpc/sendmsg.c
> +++ b/net/rxrpc/sendmsg.c
> @@ -320,10 +320,9 @@ static int rxrpc_alloc_txqueue(struct sock *sk, struct rxrpc_call *call)
>  static int rxrpc_send_data(struct rxrpc_sock *rx,
>  			   struct rxrpc_call *call,
>  			   struct msghdr *msg, size_t len,
> -			   rxrpc_notify_end_tx_t notify_end_tx,
> -			   bool *_dropped_lock)
> +			   rxrpc_notify_end_tx_t notify_end_tx)
> +	__releases(&call->user_mutex)
>  {
> -	struct rxrpc_txbuf *txb;
>  	struct sock *sk = &rx->sk;
>  	enum rxrpc_call_state state;
>  	long timeo;

[Severity: High]
Now that rxrpc_send_data() is annotated __releases(&call->user_mutex) and
owns the unlock, does the RXRPC_CALL_TX_NO_MORE test just below these
declarations still need converting?  It is not touched by this patch and
still returns without reaching out_unlock:

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

Since the callers no longer compensate with their own mutex_unlock(), can
this path leave call->user_mutex held on return?

RXRPC_CALL_TX_NO_MORE is set when the last DATA packet is queued:

net/rxrpc/sendmsg.c:rxrpc_queue_packet() {
	...
	if (last) {
		set_bit(RXRPC_CALL_TX_NO_MORE, &call->flags);
	...
}

So a sendmsg() without MSG_MORE followed by a second sendmsg() on the same
user_call_ID would take the mutex in rxrpc_do_sendmsg() via
mutex_lock_interruptible(&call->user_mutex), pass the
rxrpc_call_is_complete() pre-check (a client call is in AWAIT_REPLY at this
point), and then hit the -EPROTO return above.

Would that leave subsequent users of that call blocked indefinitely, given
that recvmsg() and the kernel-side users such as rxrpc_kernel_abort_call()
use an uninterruptible mutex_lock(&call->user_mutex), and would the task
then be able to exit while still owning the mutex?

Would "ret = -EPROTO; goto out_unlock;" be the intended form here?

> @@ -457,45 +456,46 @@ static int rxrpc_send_data(struct rxrpc_sock *rx,
>  	if (rxrpc_call_is_complete(call) &&
>  	    call->error < 0)
>  		ret = call->error;
> -out:
> -	call->tx_pending = txb;
> +out_unlock:
> +	mutex_unlock(&call->user_mutex);
>  	_leave(" = %d", ret);
>  	return ret;

[ ... ]

> @@ -770,16 +769,15 @@ int rxrpc_do_sendmsg(struct rxrpc_sock *rx, struct msghdr *msg, size_t len)
>  		ret = 0;
>  		break;
>  	case RXRPC_CMD_SEND_DATA:
> -		ret = rxrpc_send_data(rx, call, msg, len, NULL, &dropped_lock);
> -		break;
> +		ret = rxrpc_send_data(rx, call, msg, len, NULL);
> +		goto error_put;
>  	default:
>  		ret = -EINVAL;
>  		break;
>  	}
>  
>  out_put_unlock:
> -	if (!dropped_lock)
> -		mutex_unlock(&call->user_mutex);
> +	mutex_unlock(&call->user_mutex);
>  error_put:
>  	rxrpc_put_call(call, rxrpc_call_put_sendmsg);
>  	_leave(" = %d", ret);

[ ... ]

> @@ -819,13 +816,9 @@ int rxrpc_do_sendmsg(struct rxrpc_sock *rx, struct msghdr *msg, size_t len)
>  		mutex_lock(&call->user_mutex);
>  
>  		ret = rxrpc_send_data(rxrpc_sk(sock->sk), call, msg,
> -				      msg_data_left(msg),
> -				      notify_end_tx, &dropped_lock);
> +				      msg_data_left(msg), notify_end_tx);
>  		if (ret == -ESHUTDOWN)
>  			ret = call->error;
> -
> -		if (!dropped_lock)
> -			mutex_unlock(&call->user_mutex);
>  		if (ret < 0)
>  			break;

For the record, the same unconverted RXRPC_CALL_TX_NO_MORE return is moved
below the reload: label and changed to "ret = -EPROTO; goto out_unlock;" by
the following patch in the series, "rxrpc: Fix error handling in
rxrpc_send_data()", so the final state of the series does not have this
problem.

Could that conversion be folded into this patch instead, so that this commit
on its own does not leave call->user_mutex held for anyone bisecting through
it?

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