Re: [PATCH] net/rds: hold IB device reference through deferred cleanup

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] net/rds: holding ib_device ref until rds_ib_dev_free() hangs unregister
--

commit 3160014216e205f18977de718fd96d4297fa02da
Author: Eduard Zateev <hackerowskiy@gmail.com>

net/rds: hold IB device reference through deferred cleanup

This commit holds a registration reference on struct ib_device via
ib_device_try_get() in rds_ib_add_one() and releases it via
ib_device_put() in rds_ib_dev_free() so the underlying IB device is not
freed before deferred cleanup runs on rds_wq.

> diff --git a/net/rds/ib.c b/net/rds/ib.c
> index 4ea9838d090c3..ca6be8b2c3227 100644
> --- a/net/rds/ib.c
> +++ b/net/rds/ib.c
> @@ -115,6 +115,7 @@ static void rds_ib_dev_free(struct work_struct *work)
>  
>  	kfree(rds_ibdev->vector_load);
>  
> +	ib_device_put(rds_ibdev->dev);
>  	kfree(rds_ibdev);
>  }
>  
> @@ -142,6 +143,12 @@ static int rds_ib_add_one(struct ib_device *device)
>  				 ibdev_to_node(device));
>  	if (!rds_ibdev)
>  		return -ENOMEM;
> +	/* Keep the device registered until the deferred cleanup completes. */
> +	if (!ib_device_try_get(device)) {
> +		kfree(rds_ibdev);
> +		return -ENODEV;
> +	}
> +	rds_ibdev->dev = device;

[Severity: High]
Can holding this device reference until rds_ib_dev_free() cause
disable_device() to hang indefinitely during IB device unregistration?

During __ib_unregister_device() -> disable_device() in
drivers/infiniband/core/device.c, the core invokes
remove_client_context() -> rds_ib_remove_one() and then blocks waiting
for device->refcount to reach zero:

drivers/infiniband/core/device.c:disable_device() {
	...
	/* Pairs with refcount_set in enable_device */
	ib_device_put(device);
	wait_for_completion(&device->unreg_completion);
	...
}

With this change, the reference taken in rds_ib_add_one() is only
dropped in rds_ib_dev_free(), which is queued by rds_ib_dev_put() when
rds_ibdev->refcount drops to zero. However, there appear to be two paths
where rds_ibdev->refcount is not released during rds_ib_remove_one():

First, in net/rds/ib_rdma.c, rds_ib_get_mr() calls rds_ib_get_device(),
which increments rds_ibdev->refcount. When creating an ODP MR,
rds_ib_get_mr() returns without setting ibmr->device = rds_ibdev or
calling rds_ib_dev_put(rds_ibdev):

net/rds/ib_rdma.c:rds_ib_get_mr() {
	...
	rds_ibdev = rds_ib_get_device(rs->rs_bound_addr.s6_addr32[3]);
	if (!rds_ibdev) {
		ret = -ENODEV;
		goto out;
	}

	if (need_odp == ODP_ZEROBASED || need_odp == ODP_VIRTUAL) {
		...
		ib_advise_mr(rds_ibdev->pd,
			     IB_UVERBS_ADVISE_MR_ADVICE_PREFETCH_WRITE,
			     IB_UVERBS_ADVISE_MR_FLAG_FLUSH, &sge, 1);
		return ibmr;
	}
	...
}

When the ODP MR is later freed, rds_ib_free_mr() queues
rds_ib_odp_mr_worker() and returns early without calling
rds_ib_dev_put():

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;
	}
	...
}

Because every ODP MR leaks a reference on rds_ibdev->refcount, will
rds_ib_dev_free() ever run if an ODP MR was allocated on the device?

Second, for non-ODP FRMRs mapped via RDS_CMSG_RDMA_MAP (without
RDS_RDMA_USE_ONCE), each MR stored in an open socket's rs->rs_rdma_keys
rbtree holds a reference on rds_ibdev->refcount until rds_ib_free_mr()
is called, and keeps ic->i_fastreg_inuse_count > 0. Because
rds_ib_remove_one() does not revoke socket MRs and removes rds_ibdev
from rds_ib_devices before cp->cp_down_w runs
rds_ib_conn_path_shutdown() -> rds_ib_flush_mrs() on rds_wq,
rds_ib_conn_path_shutdown() stalls waiting for
i_fastreg_inuse_count == 0 while rds_ibdev->refcount remains elevated.

Would flushing or waiting on rds_wq in rds_ib_remove_one() after
rds_ib_dev_put(rds_ibdev) (and fixing the rds_ibdev reference handling)
avoid holding an ib_device registration reference across
rds_ib_remove_one()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261023100206.126692-1-hackerowskiy@gmail.com?part=1