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

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The patch adds rds_ib_dev_put(ibmr->device) to
  rds_ib_odp_mr_worker() (net/rds/ib_rdma.c:701).

--

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

The ODP path in rds_ib_get_mr() takes a reference on the rds_ib_device
but never stores or drops it, so the reference leaks. This patch stores
the device in ibmr->device. It drops the reference in
rds_ib_odp_mr_worker() after the MR has been deregistered.

> 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

[ ... ]

> @@ -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]

Can this put now race with unloading the rds_rdma module?

After this change, the ODP worker on rds_ib_mr_wq can drop the last
rds_ib_device reference. The final put queues module code on rds_wq,
and rds_wq belongs to rds core:

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

The exit path flushes rds_wq only once, in rds_ib_unregister_client().
That happens before rds_ib_mr_wq is drained:

rds_ib_exit()
    rds_ib_unregister_client()
        ib_unregister_client(&rds_ib_client);
        /* wait for rds_ib_dev_free() to complete */
        flush_workqueue(rds_wq);
    rds_ib_destroy_nodev_conns();
    ...
    rds_ib_mr_exit()
        destroy_workqueue(rds_ib_mr_wq);

An ODP worker can still be pending when that flush runs. For example,
closing an RDS socket that owns an ODP MR goes through:

rds_release()
    rds_rdma_drop_keys()
        __rds_put_mr_final()
            rds_destroy_mr()
                rds_ib_free_mr()
                    queue_delayed_work(rds_ib_mr_wq, &ibmr->work, 0);
    ...
    rds_trans_put(rs->rs_transport);

Once rds_trans_put() drops the module reference, rmmod rds_rdma can go
ahead while that worker is still queued.

ODP workers can also be queued after the rds_wq flush. Messages on
nodev connections that carry op_odp_mr go through
rds_ib_destroy_nodev_conns() -> rds_conn_destroy() -> rds_rdma_free_op().

The worker might run on its own, or be drained by destroy_workqueue() in
rds_ib_mr_exit(). Either way, if it drops the final reference,
rds_ib_dev_free() is queued on rds_wq and nothing waits for it.

Could that work then run after the rds_rdma module text has been freed,
and touch the pools and PD after the module is gone?

Before this patch the ODP reference was leaked, so this ordering could
not happen. The comment above rds_ib_mr_exit() says the pool flushing
work queue should be idle by then, and that no longer seems to hold.

Would it make sense to flush rds_ib_mr_wq before the
flush_workqueue(rds_wq) in rds_ib_unregister_client()? Another option
would be to flush rds_wq again after rds_ib_mr_exit().

>  	kfree(ibmr);
>  }

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261104022810.1484182-1-vulab%40iscas.ac.cn