Re: [PATCH v3] RDMA/siw: Fix early peer close handling on listeners

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] RDMA/siw: premature listener CEP release before sock_release() on error
--

commit 9e6e7c6fb2387746734048153461994802a461df
Author: Jaeyeong Lee <lee@jaeyeong.cc>

RDMA/siw: Fix early peer close handling on listeners

This commit fixes a use-after-free in siw_destroy_listen() and a race during
listener creation by ignoring early peer close events on listening sockets,
serializing accepted child socket handoff with socket state changes, and
holding the listener endpoint in use until setup is complete.

Link: https://lore.kernel.org/r/33abff2233b36a51e468d691cb4327d0294d2734.1620343860.git.metze@samba.org

> diff --git a/drivers/infiniband/sw/siw/siw_cm.c b/drivers/infiniband/sw/siw/siw_cm.c
> index ed49818793dde..90adb9543d9d0 100644
> --- a/drivers/infiniband/sw/siw/siw_cm.c
> +++ b/drivers/infiniband/sw/siw/siw_cm.c

[ ... ]

> @@ -1361,6 +1374,10 @@ static void siw_cm_llp_state_change(struct sock *sk)
>  
>  	case TCP_CLOSE:
>  	case TCP_CLOSE_WAIT:
> +		if (cep->state == SIW_EPSTATE_LISTENING ||
> +		    (cep->state == SIW_EPSTATE_IDLE &&
> +		     !READ_ONCE(sk->sk_socket)))
> +			break;
>  		if (cep->qp)
>  			cep->qp->tx_ctx.tx_suspend = 1;
>  		siw_cm_queue_work(cep, SIW_CM_WORK_PEER_CLOSE);
> @@ -1880,6 +1897,7 @@ int siw_create_listen(struct iw_cm_id *id, int backlog)
>  		rv = -ENOMEM;
>  		goto error;
>  	}
> +	siw_cep_set_inuse(cep);
>  	siw_cep_socket_assoc(cep, s);
>  
>  	rv = siw_cm_alloc_work(cep, backlog);
> @@ -1932,14 +1950,13 @@ int siw_create_listen(struct iw_cm_id *id, int backlog)
>  
>  	siw_dbg(id->device, "Listen at laddr %pISp\n", &id->local_addr);
>  
> +	siw_cep_set_free(cep);
>  	return 0;
>  
>  error:
>  	siw_dbg(id->device, "failed: %d\n", rv);
>  
>  	if (cep) {
> -		siw_cep_set_inuse(cep);
> -
>  		siw_free_cm_id(cep);
>  		siw_socket_disassoc(s);
>  		cep->state = SIW_EPSTATE_CLOSED;

[Severity: High]
Can the error cleanup path in siw_create_listen() trigger a use-after-free on
cep if id->provider_data allocation fails after s->ops->listen() has started?

In siw_create_listen(), s->ops->listen(s, backlog) places s->sk into
TCP_LISTEN before id->provider_data is allocated:

	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;
		}

During this window, an incoming connection can complete the TCP handshake in
softirq context, creating a child socket in the accept queue that inherits
sk_user_data = cep and the siw_cm_llp_* callbacks, and queueing
SIW_CM_WORK_ACCEPT on cep.

If kmalloc_obj() then fails and jumps to error, siw_socket_disassoc(s) only
clears sk_user_data and restores callbacks on the parent listening socket
s->sk, leaving the unaccepted child socket with sk_user_data = cep:

error:
	if (cep) {
		siw_free_cm_id(cep);
		siw_socket_disassoc(s);
		cep->state = SIW_EPSTATE_CLOSED;

		siw_cep_set_free_and_put(cep);
	}
	sock_release(s);

Because cep->state = SIW_EPSTATE_CLOSED and siw_cep_set_free_and_put(cep) run
before sock_release(s) (unlike in siw_drop_listeners(), where sock_release(s)
is called while cep->in_use is held and before cep->state is set to
SIW_EPSTATE_CLOSED), two problems can occur before sock_release(s) finishes
tearing down the unaccepted child socket:

1. Once siw_cep_set_free_and_put(cep) releases cep->in_use and drops the
listener reference, the pending SIW_CM_WORK_ACCEPT work item on siw_cm_wq
wakes up, sees cep->state != SIW_EPSTATE_LISTENING in siw_accept_newconn(),
and drops the last reference on cep, freeing it. When sock_release(s) then
runs:

  sock_release(s)
    -> inet_release()
      -> tcp_close()
        -> inet_csk_listen_stop()
          -> inet_child_forget()
            -> tcp_disconnect()
              -> siw_cm_llp_error_report(child)

siw_cm_llp_error_report() dereferences the freed cep via sk_to_cep(child).

2. If the peer closes or resets the unaccepted child after cep->state is set
to SIW_EPSTATE_CLOSED, siw_cm_llp_state_change(child) only ignores
SIW_EPSTATE_LISTENING and SIW_EPSTATE_IDLE, so it will proceed to queue
SIW_CM_WORK_PEER_CLOSE on cep (or on already-freed memory).

Should siw_create_listen() call sock_release(s) before setting cep->state =
SIW_EPSTATE_CLOSED and calling siw_cep_set_free_and_put(cep) (matching
siw_drop_listeners()), or allocate id->provider_data before calling
s->ops->listen()?

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