Re: [PATCH 2/3] rpcrdma: Pin notifications during removal callbacks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] xprtrdma: use-after-free of ep->re_xprt in rpcrdma_ep_removal_done()
--

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

rpcrdma: Pin notifications during removal callbacks

This commit adds reference-counting get and put callbacks to
rpcrdma_notification and serializes rpcrdma_rn_unregister() with
rpcrdma_device_mutex so device removal callbacks can safely run outside the
mutex.

[ ... ]

> diff --git a/net/sunrpc/xprtrdma/verbs.c b/net/sunrpc/xprtrdma/verbs.c
> index be335eed329d1..9b00e71c80a7a 100644
> --- a/net/sunrpc/xprtrdma/verbs.c
> +++ b/net/sunrpc/xprtrdma/verbs.c
> @@ -297,6 +297,22 @@ static void rpcrdma_ep_removal_done(struct rpcrdma_notification *rn)
>  	xprt_force_disconnect(ep->re_xprt);
>  }
>  
> +static bool rpcrdma_ep_removal_get(struct rpcrdma_notification *rn)
> +{
> +	struct rpcrdma_ep *ep = container_of(rn, struct rpcrdma_ep, re_rn);
> +
> +	return kref_get_unless_zero(&ep->re_kref);
> +}
> +
> +static void rpcrdma_ep_removal_put(struct rpcrdma_notification *rn)
> +{
> +	struct rpcrdma_ep *ep = container_of(rn, struct rpcrdma_ep, re_rn);
> +	struct rdma_cm_id *id = ep->re_id;
> +
> +	if (rpcrdma_ep_put(ep))
> +		rdma_destroy_id(id);
> +}

[Severity: High]
Can pinning only ep->re_kref in rpcrdma_ep_removal_get() lead to a
use-after-free of ep->re_xprt in rpcrdma_ep_removal_done()?

ep->re_xprt is an unreferenced pointer to &r_xprt->rx_xprt. When
rpcrdma_remove_one() acquires a reference on ep->re_kref via
rpcrdma_ep_removal_get() and drops rpcrdma_device_mutex before calling
rpcrdma_ep_removal_done(), a concurrent transport teardown in
xprt_destroy() -> xprt_destroy_cb() -> xprt_rdma_destroy() can run:

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

	r_xprt->rx_ep = NULL;
}

Because rpcrdma_remove_one() is holding the extra reference on ep->re_kref,
rpcrdma_ep_put(ep) does not reach zero and returns 0 without calling
rpcrdma_ep_destroy() or rpcrdma_rn_unregister(). rpcrdma_xprt_disconnect()
then returns immediately to xprt_rdma_destroy():

net/sunrpc/xprtrdma/transport.c:xprt_rdma_destroy() {
    ...
	rpcrdma_xprt_disconnect(r_xprt);
    ...
	xprt_rdma_free_addresses(xprt);
	xprt_free(xprt);
}

xprt_rdma_destroy() frees xprt->address_strings[] and schedules xprt for
destruction via kfree_rcu() while rpcrdma_remove_one() is executing in
sleepable process context outside any RCU read-side section. When
rpcrdma_ep_removal_done() then calls xprt_force_disconnect(ep->re_xprt), it
accesses the freed address_strings[] in trace_xprt_disconnect_force(), locks
xprt->transport_lock, and schedules xprt->task_cleanup on the freed
rpc_xprt.

The follow-up change ("xprtrdma: Wake connect worker on device removal")
still leaves this window open when rpcrdma_ep_removal_done() observes
xchg(&ep->re_connect_status, -ENODEV) == 1 and transitions
ep->re_force_disconnect from 0 to 1 via rpcrdma_force_disconnect(ep) before
rpcrdma_xprt_drain() flushes the receive queue (or when no receive WRs are
posted), as rpcrdma_xprt_disconnect() will still return with ep->re_kref > 0
while rpcrdma_force_disconnect() proceeds to call
xprt_force_disconnect(ep->re_xprt).

Should rpcrdma_ep_removal_get() also acquire a reference on ep->re_xprt (for
example via xprt_get_unless_zero(), or by ensuring transport teardown waits
for in-flight removal callbacks) before rpcrdma_ep_removal_done()
dereferences ep->re_xprt?

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