Re: [PATCH net-next v6 09/12] net/rds: take cp_lock to purge cp_send_queue in the quiesce

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] rds: BUG_ON panic in rds_conn_path_quiesce() from concurrent sendmsg
- [Low] rds: lockless read of rs->rs_tos introduces KCSAN data race
--

commit 9489b74a07c0708162de3f8489ff10b9010c01f0
Author: Allison Henderson <achender@kernel.org>

net/rds: take cp_lock to purge cp_send_queue in the quiesce

This commit modifies rds_conn_path_quiesce() to take the cp_lock when
purging the cp_send_queue. It splices the queue to a local list under
the lock to safely handle concurrent additions during the teardown walk.

> diff --git a/net/rds/connection.c b/net/rds/connection.c
> --- a/net/rds/connection.c
> +++ b/net/rds/connection.c
> @@ -617,10 +619,15 @@ static void rds_conn_path_quiesce(struct rds_conn_path *cp)
>  	rds_conn_path_drop(cp, true);
>  	flush_work(&cp->cp_down_w);
>  
> -	/* tear down queued messages */
> -	list_for_each_entry_safe(rm, rtmp,
> -				 &cp->cp_send_queue,
> -				 m_conn_item) {
> +	/* Tear down queued messages.  Every path that adds to
> +	 * cp_send_queue does so under cp_lock; take it here too rather
> +	 * than rely on the argument that nothing can be adding at this
> +	 * point.
> +	 */
> +	spin_lock_irqsave(&cp->cp_lock, flags);
> +	list_splice_init(&cp->cp_send_queue, &purge);
> +	spin_unlock_irqrestore(&cp->cp_lock, flags);
> +	list_for_each_entry_safe(rm, rtmp, &purge, m_conn_item) {
>  		list_del_init(&rm->m_conn_item);
>  		BUG_ON(!list_empty(&rm->m_sock_item));

[Severity: High]
If a concurrent sender is executing rds_sendmsg(), can it add a message to
both the socket queue and connection queue via rds_send_queue_rm() after
the rds_destroy_pending() check?

If the newly added message is spliced here into the purge list, it will
still be present on the socket queue (m_sock_item). Will that cause this
BUG_ON() to trigger a kernel panic when the list_empty() check evaluates
to false?

>  		rds_message_put(rm);

[ ... ]

> diff --git a/net/rds/send.c b/net/rds/send.c
> --- a/net/rds/send.c
> +++ b/net/rds/send.c
> @@ -1366,8 +1366,7 @@ int rds_sendmsg(struct socket *sock, struct msghdr *msg, size_t payload_len)
>  
>  		conn = rds_conn_create_outgoing(sock_net(sock->sk),
>  						&rs->rs_bound_addr, &daddr,
> -						rs->rs_transport,
> -						READ_ONCE(rs->rs_tos),
> +						rs->rs_transport, rs->rs_tos,

[Severity: Low]
Does removing the READ_ONCE() wrapper here introduce a KCSAN data race
warning?

Since rs_tos is read outside of the rs_lock and could be modified
concurrently by SIOCRDSSETTOS, it seems a lockless read without READ_ONCE()
might be unsafe.

>  						sock->sk->sk_allocation,
>  						scope_id);
>  		if (IS_ERR(conn)) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260922085410.391323-1-achender@kernel.org?part=9