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

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

[Eduard Zateev]

RDS device teardown queues rds_ib_dev_free() on rds_wq. Hold a
reference to the underlying IB device until deferred cleanup releases
its resources. This prevents device removal from freeing it while the
work is running.

Reported-by: syzbot+221e213bf17f17e0d6cd@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=221e213bf17f17e0d6cd
Signed-off-by: Eduard Zateev <hackerowskiy@gmail.com>
---
 net/rds/ib.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/net/rds/ib.c b/net/rds/ib.c
index 786f39169bc..597872e4fe9 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;
 
 	spin_lock_init(&rds_ibdev->spinlock);
 	refcount_set(&rds_ibdev->refcount, 1);
@@ -180,7 +187,6 @@ static int rds_ib_add_one(struct ib_device *device)
 		goto put_dev;
 	}
 
-	rds_ibdev->dev = device;
 	rds_ibdev->pd = ib_alloc_pd(device, 0);
 	if (IS_ERR(rds_ibdev->pd)) {
 		ret = PTR_ERR(rds_ibdev->pd);
-- 
2.55.0

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

[Allison Henderson]

On Fri, 2026-10-23 at 12:02 +0200, Eduard Zateev wrote:
> RDS device teardown queues rds_ib_dev_free() on rds_wq. Hold a
> reference to the underlying IB device until deferred cleanup releases
> its resources. This prevents device removal from freeing it while the
> work is running.

Hi Eduard,

Thanks for the patch.  Some comments below:

> 
> Reported-by: syzbot+221e213bf17f17e0d6cd@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=221e213bf17f17e0d6cd
The report referenced here doesnt actually point to RDS, it's a warning in rxe_pool_cleanup.
Do you have a trace or a syz test result showing that the leftover rxe object is the RDS PD
or MR and that this patch resolves the warning? Otherwise, I think the closes tag should be
removed.

> Signed-off-by: Eduard Zateev <hackerowskiy@gmail.com>
> ---
>  net/rds/ib.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/net/rds/ib.c b/net/rds/ib.c
> index 786f39169bc..597872e4fe9 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)) {


The commit message doesn't quite describe what this does. Holding the reference doesn't stop
the device from being freed while the work runs. It makes ib_unregister_device() block in
disable_device() until the refcount reaches zero, which now only happens when rds_ib_dev_free()
calls ib_device_put(). For connections that wait is short, since rdma_cm tears them down during
the same removal, but connections are not the only things that hold the rds_ib_device.

A registered MR keeps the reference it takes in rds_ib_get_mr() until rds_ib_free_mr() drops
it. That only happens once everything using the MR has let go. Including the socket (when it
frees the MR or closes), and every queued message that uses it (when the peer acks it or the
socket closes).  Those messages also survive connection drops (rds_send_path_reset() requeues
them), and with the  device gone the connection can't come back up over it to get them acked. 

So with this patch, whoever is unregistering the device (rdma link delete, driver unload, PCI
hot-unplug) blocks in disable_device() until the process that owns the socket lets go, which
it may never do. It  also turns any leaked rds_ib_device reference into a permanent unregister
hang. I have a fix for one such leak, a connection left attached at module unload, in the
connection-lifetime series I've been iterating on, but it hasn't landed yet.

Unfortunately this is not a simple fix, even after kref lands. To wait for the free safely,
remove_one first needs a way to make the device's MRs be released, so that the wait is
guaranteed to finish.  The uek tree approach is a socket accounting feature:

https://github.com/oracle/linux-uek/commit/ba92066dbf13e573a292252ea07e686b5104f091
https://github.com/oracle/linux-uek/commit/dc4c8c665368bca276db02ba7071d2d3fd653a10
https://github.com/oracle/linux-uek/commit/fcd394a1095625a39c44aa709bd976c6b9b98373

You're certainly welcome to look at it, but it is a bigger rework of the removal path than
this patch, and I'd rather not take a partial fix that turns the warning into a hang.

Thank you for looking at this though!

Allison

> +		kfree(rds_ibdev);
> +		return -ENODEV;
> +	}
> +	rds_ibdev->dev = device;
>  
>  	spin_lock_init(&rds_ibdev->spinlock);
>  	refcount_set(&rds_ibdev->refcount, 1);
> @@ -180,7 +187,6 @@ static int rds_ib_add_one(struct ib_device *device)
>  		goto put_dev;
>  	}
>  
> -	rds_ibdev->dev = device;
>  	rds_ibdev->pd = ib_alloc_pd(device, 0);
>  	if (IS_ERR(rds_ibdev->pd)) {
>  		ret = PTR_ERR(rds_ibdev->pd);

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

[netdev-bot+sashiko]

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

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

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 takes an ib_device registration reference
  (ib_device_try_get) in rds_ib_add_one() and releases it only in…
- [High] In rds_ib_get_mr() (net/rds/ib_rdma.c), the ODP branch (need_odp
  == ODP_ZEROBASED or ODP_VIRTUAL) takes an rds_ibdev reference through…
- [Low] The patch is presented as a fix for a syzbot-reported use-after-
  free: rds_ib_dev_free() runs on rds_wq after rds_ib_remove_one() has…

--

net/rds: hold IB device reference through deferred cleanup

This patch takes an ib_device registration reference in rds_ib_add_one()
and drops it at the end of rds_ib_dev_free(). The IB device then stays
registered until the deferred cleanup on rds_wq has run. It addresses a
syzbot-reported use-after-free in that deferred cleanup.

Closes: https://syzkaller.appspot.com/bug?extid=221e213bf17f17e0d6cd

> Reported-by: syzbot+221e213bf17f17e0d6cd@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=221e213bf17f17e0d6cd
> Signed-off-by: Eduard Zateev <hackerowskiy@gmail.com>

[Severity: Low]
This isn't a bug, but should there be a Fixes: tag here?

git blame of rds_ib_dev_free() and rds_ib_dev_put() points the deferred
free design at commit 3e0249f9c05c ("RDS/IB: add refcount tracking to
struct rds_ib_device").

Should this also have a Cc: stable?

> 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 this make ib_unregister_device() wait on userspace?

The reference taken here is dropped only in rds_ib_dev_free(), which runs
only after rds_ibdev->refcount reaches zero. rds_ib_remove_one() drops
just the client data and device list references:

net/rds/ib.c:rds_ib_remove_one() {
    ...
	synchronize_rcu();
	rds_ib_dev_put(rds_ibdev);
	rds_ib_dev_put(rds_ibdev);
}

Sockets can hold other references. For example, an FRMR MR created
through an RDS_CMSG_RDMA_MAP cmsg holds one:

sendmsg()
  rds_cmsg_rdma_map()
    __rds_rdma_map()
      rds_ib_get_mr()
        rds_ib_get_device()        <- takes an rds_ibdev reference
        rds_ib_reg_frmr()
          ibmr->device = rds_ibdev;

The MR is stored in rs->rs_rdma_keys. Its reference is dropped only in
rds_ib_free_mr(), which runs when userspace issues RDS_FREE_MR or closes
the socket.

Neither rds_ib_remove_one() nor rds_ib_dev_shutdown() revokes these MRs.
Suppose the HCA is unregistered (driver unload, hot-unplug, rxe/siw link
delete) while such a socket is open. disable_device() then does:

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

Wouldn't that wait block uninterruptibly, with unregistration_lock held,
until the user frees the MR or closes the socket?

The comment above add_client_context() and remove_client_context() says
client add and remove must be a fence. Keeping the registration refcount
past .remove() seems to break that rule.

Would it work better to tear down the PD and MR pools synchronously in
rds_ib_remove_one() instead?

The commit message also doesn't mention that unregistration now waits on
RDS reference holders.

[Severity: High]
Does the ODP path in rds_ib_get_mr() turn this into a permanent hang?

In net/rds/ib_rdma.c, the ODP branch takes a reference with
rds_ib_get_device(). It returns without storing that reference in
ibmr->device and without dropping it:

net/rds/ib_rdma.c:rds_ib_get_mr() {
    ...
	rds_ibdev = rds_ib_get_device(rs->rs_bound_addr.s6_addr32[3]);
    ...
	if (need_odp == ODP_ZEROBASED || need_odp == ODP_VIRTUAL) {
    ...
		ibmr = kzalloc_obj(*ibmr);
    ...
		ibmr->u.mr = ib_mr;
		ibmr->odp = 1;
    ...
		return ibmr;
	}
    ...
}

When the MR is freed, rds_ib_free_mr() returns early for ODP MRs, before
it reaches rds_ib_dev_put():

	if (ibmr->odp) {
    ...
		INIT_DELAYED_WORK(&ibmr->work, rds_ib_odp_mr_worker);
		queue_delayed_work(rds_ib_mr_wq, &ibmr->work, 0);
		return;
	}

rds_ib_odp_mr_worker() then only calls ib_dereg_mr() and kfree(ibmr).

Before this patch, that imbalance only leaked the rds_ibdev and the PD.
With this patch, a single ODP MR keeps rds_ibdev->refcount from ever
reaching zero, so the new ib_device_put() in rds_ib_dev_free() never
runs.

Any later unregistration of that HCA would then block forever in the
wait_for_completion() in disable_device(), even after the socket is
closed.

A local user can reach the ODP branch on an ODP-capable HCA such as mlx5.
It is taken when rds_pin_pages() returns -EOPNOTSUPP in __rds_rdma_map()
or rds_cmsg_rdma_args(). One case is an FS-DAX mapping, where
FOLL_LONGTERM pinning is refused.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261023100206.126692-1-hackerowskiy%40gmail.com