Re: [PATCH] net/rds: drop the device reference for ODP memory regions

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] net/rds: rds_ib_odp_mr_worker() queues rds_ib_dev_free() after rds_wq flush
--

commit 867b786572304fe8059ffdca78162ac460e2ac8e
Author: Wentao Liang <vulab@iscas.ac.cn>

net/rds: drop the device reference for ODP memory regions

This patch stores the rds_ib_device reference in ibmr->device when registering
an ODP memory region in rds_ib_get_mr() and drops it in rds_ib_odp_mr_worker()
after deregistering the MR.

> diff --git a/net/rds/ib_rdma.c b/net/rds/ib_rdma.c
> index db7e92e7bd29f..8cd8295f66a39 100644
> --- a/net/rds/ib_rdma.c
> +++ b/net/rds/ib_rdma.c
> @@ -591,6 +591,7 @@ void *rds_ib_get_mr(struct scatterlist *sg, unsigned long nents,
>  		}
>  		ibmr->u.mr = ib_mr;
>  		ibmr->odp = 1;
> +		ibmr->device = rds_ibdev;
>  
>  		sge.addr = virt_addr;
>  		sge.length = length;
> @@ -697,5 +698,6 @@ static void rds_ib_odp_mr_worker(struct work_struct  *work)
>  
>  	ibmr = container_of(work, struct rds_ib_mr, work.work);
>  	ib_dereg_mr(ibmr->u.mr);
> +	rds_ib_dev_put(ibmr->device);

[Severity: High]
Could dropping ibmr->device from rds_ib_odp_mr_worker() on rds_ib_mr_wq cause
rds_ib_dev_free() to be queued onto rds_wq after rds_ib_unregister_client()
has already flushed rds_wq during module unload?

Unlike FRMRs, which call rds_ib_dev_put() synchronously in rds_ib_free_mr(),
ODP MRs defer teardown to rds_ib_mr_wq:

net/rds/ib_rdma.c:rds_ib_free_mr() {
	if (ibmr->odp) {
		INIT_DELAYED_WORK(&ibmr->work, rds_ib_odp_mr_worker);
		queue_delayed_work(rds_ib_mr_wq, &ibmr->work, 0);
		return;
	}
}

When the last RDS socket closes via rds_release() -> rds_rdma_drop_keys() ->
rds_ib_free_mr() (or rds_send_drop_to() -> rds_rdma_free_op() ->
rds_ib_free_mr()), rds_ib_flush_mrs() only flushes mr_8k_pool and mr_1m_pool
without flushing rds_ib_mr_wq before rds_release() drops the transport module
reference.

If the rds_rdma module is then unloaded, rds_ib_exit() runs
rds_ib_unregister_client() before rds_ib_mr_exit():

net/rds/ib.c:rds_ib_unregister_client() {
	ib_unregister_client(&rds_ib_client);
	/* wait for rds_ib_dev_free() to complete */
	flush_workqueue(rds_wq);
}

net/rds/ib.c:rds_ib_exit() {
	...
	rds_ib_unregister_client();
	rds_ib_destroy_nodev_conns();
	rds_ib_sysctl_exit();
	rds_ib_recv_exit();
	rds_trans_unregister(&rds_ib_transport);
	rds_ib_mr_exit();
}

If rds_ib_odp_mr_worker() is still pending on rds_ib_mr_wq when
rds_ib_unregister_client() runs, rds_ibdev->refcount will not drop to zero in
rds_ib_remove_one(), so rds_ibdev->free_work is not queued yet when
flush_workqueue(rds_wq) returns.

Then, at the end of rds_ib_exit(), rds_ib_mr_exit() calls
destroy_workqueue(rds_ib_mr_wq), which drains rds_ib_odp_mr_worker() and calls
rds_ib_dev_put(ibmr->device):

net/rds/ib.c:rds_ib_dev_put() {
	BUG_ON(refcount_read(&rds_ibdev->refcount) == 0);
	if (refcount_dec_and_test(&rds_ibdev->refcount))
		queue_work(rds_wq, &rds_ibdev->free_work);
}

Wouldn't that queue rds_ibdev->free_work (rds_ib_dev_free) onto rds_wq right
as rds_ib_exit() returns, allowing rds_ib_dev_free() to execute after
rds_ib_mr_wq is destroyed, after ib_unregister_client() has completed, and
after the rds_rdma module is unmapped?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104022810.1484182-1-vulab@iscas.ac.cn?part=1