Re: [PATCH] RDMA/siw: Ignore peer close events on listening endpoints

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] RDMA/siw: listen() before setting SIW_EPSTATE_LISTENING in siw_create_listen()
- [High] RDMA/siw: lost peer close when child socket closes before kernel_accept()
--

commit 4ef3f34aa3f1c3ee451c3e1beb79ef0ea51f6cdf
Author: Jaeyeong Lee <lee@jaeyeong.cc>

RDMA/siw: Ignore peer close events on listening endpoints

This commit ignores TCP_CLOSE and TCP_CLOSE_WAIT state change notifications in
siw_cm_llp_state_change() when cep->state is SIW_EPSTATE_LISTENING to prevent
a child socket closed before kernel_accept() from queueing a PEER_CLOSE work
item against the listening endpoint.

> diff --git a/drivers/infiniband/sw/siw/siw_cm.c b/drivers/infiniband/sw/siw/siw_cm.c
> index ed49818793dde..518f09a2d37cf 100644
> --- a/drivers/infiniband/sw/siw/siw_cm.c
> +++ b/drivers/infiniband/sw/siw/siw_cm.c
> @@ -1361,6 +1361,8 @@ static void siw_cm_llp_state_change(struct sock *sk)
>  
>  	case TCP_CLOSE:
>  	case TCP_CLOSE_WAIT:
> +		if (cep->state == SIW_EPSTATE_LISTENING)
> +			break;

[Severity: Critical]
Does this check leave a window during listener creation where the same
use-after-free can still happen?

In drivers/infiniband/sw/siw/siw_cm.c:siw_create_listen(), the socket is
associated with cep and placed into TCP_LISTEN while cep->state is still
SIW_EPSTATE_IDLE, before a sleepable kmalloc_obj() call and before cep->state
is set to SIW_EPSTATE_LISTENING:

drivers/infiniband/sw/siw/siw_cm.c:siw_create_listen() {
	...
	cep = siw_cep_alloc(sdev);
	if (!cep) {
		rv = -ENOMEM;
		goto error;
	}
	siw_cep_socket_assoc(cep, s);
	...
	rv = s->ops->listen(s, backlog);
	if (rv) {
		siw_dbg(id->device, "listen error %d\n", rv);
		goto error;
	}
	cep->cm_id = id;
	id->add_ref(id);
	...
	if (!id->provider_data) {
		id->provider_data =
			kmalloc_obj(struct list_head);
		if (!id->provider_data) {
			rv = -ENOMEM;
			goto error;
		}
		INIT_LIST_HEAD((struct list_head *)id->provider_data);
	}
	list_add_tail(&cep->listenq, (struct list_head *)id->provider_data);
	cep->state = SIW_EPSTATE_LISTENING;
	...
}

If a remote peer completes a TCP handshake and sends RST or FIN after
s->ops->listen() but before cep->state = SIW_EPSTATE_LISTENING,
siw_cm_llp_state_change() runs on the child socket while cep->state is still
SIW_EPSTATE_IDLE. Wouldn't that bypass the SIW_EPSTATE_LISTENING check and
still queue SIW_CM_WORK_PEER_CLOSE against the listening cep, freeing it via
siw_cm_work_handler() while it remains on id->provider_data?

[Severity: High]
Also, if the peer closes or resets the child socket before siw_accept_newconn()
runs, won't the SIW_CM_WORK_PEER_CLOSE notification for the accepted connection
be lost completely?

The commit message notes that once the child is accepted, its sk_user_data
points to the new endpoint and close notifications continue through the normal
path. However, kernel_accept() succeeds and dequeues the child socket from
icsk_accept_queue even when new_s->sk->sk_state is already TCP_CLOSE or
TCP_CLOSE_WAIT:

drivers/infiniband/sw/siw/siw_cm.c:siw_accept_newconn() {
	...
	rv = kernel_accept(s, &new_s, O_NONBLOCK);
	if (rv != 0) {
		/*
		 * Connection already aborted by peer..?
		 */
		siw_dbg_cep(cep, "kernel_accept() error: %d\n", rv);
		goto error;
	}
	new_cep->sock = new_s;
	siw_cep_get(new_cep);
	new_s->sk->sk_user_data = new_cep;
	...
	if (atomic_read(&new_s->sk->sk_rmem_alloc)) {
		/*
		 * MPA REQ already queued
		 */
		...
		rv = siw_proc_mpareq(new_cep);
	...
}

Unlike the receive data path where siw_accept_newconn() checks sk_rmem_alloc
after setting sk_user_data = new_cep, siw_accept_newconn() neither holds
sk_callback_lock while updating sk_user_data nor checks new_s->sk->sk_state
afterward.

If the transition to TCP_CLOSE_WAIT or TCP_CLOSE already fired while
sk_user_data pointed to the listening cep, no further sk_state_change callback
will fire for that transition. If the peer sent an MPA request with private
data followed by FIN before kernel_accept(), siw_recv_mpa_rr() reads the MPA
request without consuming the EOF, siw_proc_mpareq() delivers
IW_CM_EVENT_CONNECT_REQUEST, and siw_accept() cancels mpa_timer and moves the
QP to RTS on a half-closed TCP_CLOSE_WAIT socket that never receives
SIW_CM_WORK_PEER_CLOSE. If the peer sent RST or FIN without a complete MPA
request, new_cep still holds new_s and listen_cep until MPAREP_TIMEOUT
expires.

Should siw_accept_newconn() serialize the sk_user_data update with
sk_callback_lock and inspect new_s->sk->sk_state after associating new_cep?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/179378222423.73361.5194383282780549988@jaeyeong.cc?part=1