Re: [PATCH 3/3] xprtrdma: Wake connect worker on device removal

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] xprtrdma: use-after-free of ep in rpcrdma_cm_event_handler() on teardown
- [High] xprtrdma: ep kref leak and removal hang on UNREACHABLE or REJECTED event
- [High] xprtrdma: use-after-free or spurious disconnect of re_xprt on ESTABLISHED
--

commit ab99aeccfc2a88d68d44010bf5a53935dda9cc76
Author: Li Xiasong <lixiasong1@huawei.com>

xprtrdma: Wake connect worker on device removal

This commit updates rpcrdma_cm_event_handler() and rpcrdma_ep_removal_done()
to use atomic state transitions on ep->re_connect_status so that device
removal wakes the connect worker when not yet established and the endpoint
reference taken on RDMA_CM_EVENT_ESTABLISHED is released once on state
change.

> diff --git a/net/sunrpc/xprtrdma/verbs.c b/net/sunrpc/xprtrdma/verbs.c
> index 9b00e71c80a7a..ff65bb9e28ff6 100644
> --- a/net/sunrpc/xprtrdma/verbs.c
> +++ b/net/sunrpc/xprtrdma/verbs.c
> @@ -229,6 +229,7 @@ static int
>  rpcrdma_cm_event_handler(struct rdma_cm_id *id, struct rdma_cm_event *event)
>  {
>  	struct rpcrdma_ep *ep = id->context;
> +	int status;
>  
>  	might_sleep();
>  
> @@ -251,16 +252,21 @@ rpcrdma_cm_event_handler(struct rdma_cm_id *id, struct rdma_cm_event *event)
>  		case 0:
>  			goto wake_connect_worker;
>  		case 1:
> -			/* The later DISCONNECTED event balances the
> -			 * ESTABLISHED get; do not put here.
> -			 */
>  			rpcrdma_force_disconnect(ep);
> -			return 0;
> +			return rpcrdma_ep_put(ep);
>  		}
>  		return 0;
>  	case RDMA_CM_EVENT_ESTABLISHED:
>  		rpcrdma_ep_get(ep);
> -		ep->re_connect_status = 1;
> +		switch (cmpxchg(&ep->re_connect_status, 0, 1)) {
> +		case 0:
> +			break;
> +		case -ENODEV:
> +			rpcrdma_force_disconnect(ep);
> +			return rpcrdma_ep_put(ep);

[Severity: High]
Can calling rpcrdma_force_disconnect(ep) here cause a use-after-free on
ep->re_xprt or a spurious disconnect on a subsequent connection attempt?

When ep->re_connect_status transitions from 0 to -ENODEV in
RDMA_CM_EVENT_ADDR_CHANGE or rpcrdma_ep_removal_done(), the endpoint was
not yet established and ep->re_connect_wait is woken so that
rpcrdma_xprt_connect() returns -ENODEV to xprt_rdma_connect_worker():

net/sunrpc/xprtrdma/transport.c:xprt_rdma_connect_worker() {
	...
	rc = rpcrdma_xprt_connect(r_xprt);
	xprt_clear_connecting(xprt);
	if (!rc) {
		...
	} else
		rpcrdma_xprt_disconnect(r_xprt);
	xprt_unlock_connect(xprt, r_xprt);
}

If RDMA_CM_EVENT_ESTABLISHED races with that teardown and takes a temporary
reference via rpcrdma_ep_get(ep) before rpcrdma_xprt_disconnect() calls
rpcrdma_ep_put(ep), rpcrdma_xprt_disconnect() sees a non-zero kref, clears
r_xprt->rx_ep = NULL, and returns:

net/sunrpc/xprtrdma/verbs.c:rpcrdma_xprt_disconnect() {
	...
	if (rpcrdma_ep_put(ep))
		rdma_destroy_id(id);

	r_xprt->rx_ep = NULL;
}

Once xprt_rdma_connect_worker() finishes, a concurrent xprt_rdma_destroy()
can complete cancel_delayed_work_sync(&r_xprt->rx_connect_worker), observe
r_xprt->rx_ep == NULL, and free xprt via xprt_free(xprt) (or if not
destroyed, a new connection attempt can start on xprt) before
RDMA_CM_EVENT_ESTABLISHED executes rpcrdma_force_disconnect(ep) ->
xprt_force_disconnect(ep->re_xprt).

> +		default:
> +			return rpcrdma_ep_put(ep);
> +		}
>  		rpcrdma_update_cm_private(ep, &event->param.conn);
>  		trace_xprtrdma_inline_thresh(ep);
>  		wake_up_all(&ep->re_connect_wait);
> @@ -279,9 +285,11 @@ rpcrdma_cm_event_handler(struct rdma_cm_id *id, struct rdma_cm_event *event)
>  		wake_up_all(&ep->re_connect_wait);
>  		return 0;
>  	case RDMA_CM_EVENT_DISCONNECTED:
> -		ep->re_connect_status = -ECONNABORTED;
> +		status = xchg(&ep->re_connect_status, -ECONNABORTED);
>  		rpcrdma_force_disconnect(ep);
> -		return rpcrdma_ep_put(ep);
> +		if (status == 1)
> +			return rpcrdma_ep_put(ep);
> +		return 0;

[Severity: High]
What happens if RDMA_CM_EVENT_UNREACHABLE or RDMA_CM_EVENT_REJECTED arrives
after RDMA_CM_EVENT_ESTABLISHED has already set ep->re_connect_status to 1?

In rpcrdma_cm_event_handler(), RDMA_CM_EVENT_CONNECT_ERROR,
RDMA_CM_EVENT_UNREACHABLE, and RDMA_CM_EVENT_REJECTED still overwrite
ep->re_connect_status unconditionally without checking whether the previous
value was 1:

net/sunrpc/xprtrdma/verbs.c:rpcrdma_cm_event_handler() {
	...
	case RDMA_CM_EVENT_CONNECT_ERROR:
		ep->re_connect_status = -ENOTCONN;
		goto wake_connect_worker;
	case RDMA_CM_EVENT_UNREACHABLE:
		ep->re_connect_status = -ENETUNREACH;
		goto wake_connect_worker;
	case RDMA_CM_EVENT_REJECTED:
		ep->re_connect_status = -ECONNREFUSED;
		if (event->status == IB_CM_REJ_STALE_CONN)
			ep->re_connect_status = -ENOTCONN;
wake_connect_worker:
		wake_up_all(&ep->re_connect_wait);
		return 0;
}

In RDMA CMA, both events can be delivered on a client CM ID after
RDMA_CM_EVENT_ESTABLISHED:

1. On RoCE, rdma_resolve_route() inserts id_priv into id_table via
   cma_add_id_to_tree(), where it remains in state RDMA_CM_CONNECT until
   _destroy_id(). On a NETEVENT_NEIGH_UPDATE event,
   cma_netevent_callback() queues id_priv->id.net_work and
   cma_netevent_work_handler() delivers RDMA_CM_EVENT_UNREACHABLE unless
   the state is RDMA_CM_DESTROYING or RDMA_CM_DEVICE_REMOVAL.

2. On IB/RoCE, after cma_ib_handler() processes IB_CM_REP_RECEIVED via
   cma_rep_recv() -> ib_send_cm_rtu() and delivers
   RDMA_CM_EVENT_ESTABLISHED, cm_rej_handler() still accepts a peer REJ MAD
   while in IB_CM_ESTABLISHED (with lap_state == IB_CM_LAP_UNINIT) or
   IB_CM_DREQ_SENT, and cma_ib_handler() delivers RDMA_CM_EVENT_REJECTED.

If either event overwrites ep->re_connect_status from 1 to a negative
errno, neither that event nor a subsequent RDMA_CM_EVENT_DISCONNECTED,
RDMA_CM_EVENT_ADDR_CHANGE, or rpcrdma_ep_removal_done() will observe
status == 1.

Wouldn't that skip rpcrdma_ep_put(ep) (and skip rpcrdma_force_disconnect(ep)
in RDMA_CM_EVENT_ADDR_CHANGE and rpcrdma_ep_removal_done()), leaking
ep->re_kref and causing rpcrdma_remove_one() to hang indefinitely at
wait_for_completion(&rd->rd_done)?

>  	default:
>  		break;
>  	}
> @@ -294,7 +302,18 @@ static void rpcrdma_ep_removal_done(struct rpcrdma_notification *rn)
>  	struct rpcrdma_ep *ep = container_of(rn, struct rpcrdma_ep, re_rn);
>  
>  	trace_xprtrdma_device_removal(ep->re_id);
> -	xprt_force_disconnect(ep->re_xprt);
> +	switch (xchg(&ep->re_connect_status, -ENODEV)) {
> +	case 0:
> +		wake_up_all(&ep->re_connect_wait);
> +		break;
> +	case 1:
> +		rpcrdma_force_disconnect(ep);
> +		/* DEVICE_REMOVAL prevents a later DISCONNECTED event. */
> +		rpcrdma_ep_put(ep);
> +		break;
> +	default:
> +		break;
> +	}
>  }

[Severity: Critical]
Does dropping the ESTABLISHED reference in RDMA_CM_EVENT_ADDR_CHANGE and
rpcrdma_ep_removal_done() (or waking the connect worker on case 0 while CM
events can still fire) allow ep to be freed while rpcrdma_cm_event_handler()
is still using id->context?

In rpcrdma_xprt_disconnect(), rpcrdma_ep_put(ep) is called before
rdma_destroy_id(id):

net/sunrpc/xprtrdma/verbs.c:rpcrdma_xprt_disconnect() {
	...
	id = ep->re_id;
	rc = rdma_disconnect(id);
	trace_xprtrdma_disconnect(r_xprt, rc);

	rpcrdma_xprt_drain(r_xprt);
	rpcrdma_reps_unmap(r_xprt);
	rpcrdma_sendctxs_destroy(r_xprt);
	rpcrdma_reqs_reset(r_xprt);
	rpcrdma_mrs_destroy(r_xprt);

	if (rpcrdma_ep_put(ep))
		rdma_destroy_id(id);
}

When rpcrdma_ep_put(ep) drops the last reference on ep->re_kref,
rpcrdma_ep_destroy() runs immediately and calls kfree(ep) before
rdma_destroy_id(id) acquires id_priv->handler_mutex to synchronize with
rpcrdma_cm_event_handler().

Prior to this change, commit af9b65b29af3 ("xprtrdma: Fix ep kref imbalance
on ADDR_CHANGE") kept the ESTABLISHED reference on RDMA_CM_EVENT_ADDR_CHANGE
until RDMA_CM_EVENT_DISCONNECTED arrived. Neither RDMA_CM_EVENT_ADDR_CHANGE
nor rpcrdma_ep_removal_done() stops further CM events from firing on id:

1. RDMA_CM_EVENT_ADDR_CHANGE leaves id_priv->state as RDMA_CM_CONNECT.

2. During device removal, disable_device() invokes ib_client->remove
   callbacks in reverse registration order. Because rpcrdma_ib_client is
   registered after cma_client, rpcrdma_remove_one() runs and waits in
   wait_for_completion(&rd->rd_done) before cma_remove_one() ->
   cma_process_remove() transitions id_priv->state to
   RDMA_CM_DEVICE_REMOVAL.

Once RDMA_CM_EVENT_ADDR_CHANGE or rpcrdma_ep_removal_done() drops the
ESTABLISHED reference (or wakes the connect worker from case 0 while a
connection attempt is still in flight), rpcrdma_xprt_disconnect() calls
rdma_disconnect(id) (which triggers RDMA_CM_EVENT_DISCONNECTED), drains the
QP, and drops the final ep->re_kref reference in rpcrdma_ep_put(ep),
freeing ep before rdma_destroy_id(id) is reached.

Won't a concurrent rpcrdma_cm_event_handler() invocation (such as
RDMA_CM_EVENT_DISCONNECTED triggered by rdma_disconnect(id), or a racing
RDMA_CM_EVENT_ESTABLISHED on the case 0 path) then dereference id->context
after ep has already been freed?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261023125033.528835-1-lixiasong1@huawei.com?part=3