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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] xprtrdma: rpcrdma_cm_event_handler() forces disconnect on unpinned xprt
- [High] xprtrdma: rpcrdma_ep_disconnect() fails to update established status
--

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

xprtrdma: Wake connect worker on device removal

This commit updates endpoint connection status transitions and CM ID teardown
during RDMA device removal and CM events so that pending connect workers are
woken and endpoint destruction synchronizes with CM callbacks.

> diff --git a/net/sunrpc/xprtrdma/verbs.c b/net/sunrpc/xprtrdma/verbs.c
> index 69d1796bb4f3b..9eec462cffd92 100644
> --- a/net/sunrpc/xprtrdma/verbs.c
> +++ b/net/sunrpc/xprtrdma/verbs.c
> @@ -75,7 +75,7 @@ static void rpcrdma_reps_unmap(struct rpcrdma_xprt *r_xprt);
>  static void rpcrdma_mrs_create(struct rpcrdma_xprt *r_xprt);
>  static void rpcrdma_mrs_destroy(struct rpcrdma_xprt *r_xprt);
>  static void rpcrdma_ep_get(struct rpcrdma_ep *ep);
> -static int rpcrdma_ep_put(struct rpcrdma_ep *ep);
> +static void rpcrdma_ep_put(struct rpcrdma_ep *ep);
>  static struct rpcrdma_regbuf *
>  rpcrdma_regbuf_alloc_node(size_t size, enum dma_data_direction direction,
>  			  int node);
> @@ -124,6 +124,20 @@ void rpcrdma_force_disconnect(struct rpcrdma_ep *ep)
>  		xprt_force_disconnect(ep->re_xprt);
>  }
>  
> +static void rpcrdma_ep_disconnect(struct rpcrdma_ep *ep, int status)
> +{
> +	switch (cmpxchg(&ep->re_connect_status, 0, status)) {
> +	case 0:
> +		wake_up_all(&ep->re_connect_wait);
> +		break;
> +	case 1:
> +		rpcrdma_force_disconnect(ep);
> +		break;
> +	default:
> +		break;
> +	}
> +}

[Severity: High]
When ep->re_connect_status is already 1 (established), cmpxchg(&ep->re_connect_status, 0, status) in rpcrdma_ep_disconnect() returns 1 without updating ep->re_connect_status to status.

Unlike the previous xchg(&ep->re_connect_status, -ENODEV) in RDMA_CM_EVENT_ADDR_CHANGE (and the xchg() calls used for RDMA_CM_EVENT_REJECTED and RDMA_CM_EVENT_DISCONNECTED), calling rpcrdma_ep_disconnect() from rpcrdma_ep_removal_done(), RDMA_CM_EVENT_ADDR_CHANGE, or RDMA_CM_EVENT_UNREACHABLE leaves ep->re_connect_status set to 1.

If RDMA_CM_EVENT_ESTABLISHED transitions ep->re_connect_status to 1 and wakes ep->re_connect_wait, and then rpcrdma_ep_disconnect() (or rpcrdma_ep_removal_get() when xprt_get() fails) runs before rpcrdma_xprt_connect() checks the status:

net/sunrpc/xprtrdma/verbs.c:rpcrdma_xprt_connect() {
	...
	wait_event_interruptible(ep->re_connect_wait,
				 ep->re_connect_status != 0);
	if (ep->re_connect_status <= 0) {
		rc = ep->re_connect_status;
		goto out;
	}
	...
}

won't rpcrdma_xprt_connect() observe ep->re_connect_status == 1 and proceed to allocate and post send contexts, receive buffers, and MRs on the disconnecting or removing device while holding XPRT_LOCKED?

Leaving ep->re_connect_status == 1 also allows rpcrdma_mrs_refresh() to continue queueing buf->rb_refresh_worker until xprt_autoclose() runs:

net/sunrpc/xprtrdma/verbs.c:rpcrdma_mrs_refresh() {
	...
	if (ep->re_connect_status != 1)
		return;
	queue_work(system_highpri_wq, &buf->rb_refresh_worker);
}

Should rpcrdma_ep_disconnect() update ep->re_connect_status when transitioning from 1 as well (for example, using xchg() or a second transition from 1 to status)?

[ ... ]

> @@ -247,20 +262,18 @@ rpcrdma_cm_event_handler(struct rdma_cm_id *id, struct rdma_cm_event *event)
>  		complete(&ep->re_done);
>  		return 0;
>  	case RDMA_CM_EVENT_ADDR_CHANGE:
> -		switch (xchg(&ep->re_connect_status, -ENODEV)) {
> +		rpcrdma_ep_disconnect(ep, -ENODEV);
> +		return 0;
> +	case RDMA_CM_EVENT_ESTABLISHED:
> +		switch (cmpxchg(&ep->re_connect_status, 0, 1)) {
>  		case 0:
> -			goto wake_connect_worker;
> -		case 1:
> -			/* The later DISCONNECTED event balances the
> -			 * ESTABLISHED get; do not put here.
> -			 */
> +			break;
> +		case -ENODEV:
>  			rpcrdma_force_disconnect(ep);
>  			return 0;
> +		default:
> +			return 0;
>  		}

[Severity: Critical]
As reported by Sashiko on v1 [1], when the final xprt_put() has already dropped xprt->kref to 0 and xprt_destroy() is waiting on XPRT_LOCKED, rpcrdma_ep_removal_get() sees !xprt_get(xprt) and sets ep->re_connect_status to -ENODEV via cmpxchg() to wake the connect worker without touching the unpinned xprt:

net/sunrpc/xprtrdma/verbs.c:rpcrdma_ep_removal_get() {
	if (!xprt_get(xprt)) {
		if (cmpxchg(&ep->re_connect_status, 0, -ENODEV) == 0)
			wake_up_all(&ep->re_connect_wait);
		return false;
	}
}

If RDMA_CM_EVENT_ESTABLISHED arrives in rpcrdma_cm_event_handler() before the connect worker finishes rpcrdma_xprt_disconnect(), cmpxchg(&ep->re_connect_status, 0, 1) returns -ENODEV and executes:

	rpcrdma_force_disconnect(ep);
	  -> xprt_force_disconnect(ep->re_xprt);
	    -> xprt_schedule_autoclose_locked(xprt);

which sets XPRT_CLOSE_WAIT on the unpinned xprt whose kref is already 0.

When xprt_rdma_connect_worker() subsequently unlocks the transport via xprt_unlock_connect() -> xprt_release_xprt_cong() -> xprt_clear_locked(), seeing XPRT_CLOSE_WAIT causes xprt_clear_locked() to queue xprt->task_cleanup (xprt_autoclose) instead of clearing XPRT_LOCKED. Once xprt_autoclose() calls xprt_release_write() and clears XPRT_LOCKED, xprt_destroy() wakes from wait_on_bit_lock(), reinitializes xprt->task_cleanup with INIT_WORK(&xprt->task_cleanup, xprt_destroy_cb), and frees xprt while xprt_autoclose() is still finishing.

Since ep->re_connect_status was never 1 when cmpxchg(&ep->re_connect_status, 0, 1) observes -ENODEV, rpcrdma_xprt_connect() is already guaranteed to see ep->re_connect_status <= 0 and trigger rpcrdma_xprt_disconnect() from xprt_rdma_connect_worker(). Can the -ENODEV case in RDMA_CM_EVENT_ESTABLISHED simply return 0 without calling rpcrdma_force_disconnect(ep)?

[1] https://lore.kernel.org/r/20261023125033.528835-4-lixiasong1@huawei.com

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