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

[netdev-bot+sashiko@kernel.org]

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