Re: [PATCH rdma-next v3 13/13] RDMA/core: attach a memory region to a queue pair

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] RDMA/core: mr->usecnt release ordering race in QP_DETACH_MR
  and destroy_qp
--

commit beeb942760f9f2e71c333c8b383858123a5c467f
Author: Eric Davis <eric.davis@broadcom.com>

RDMA/core: attach a memory region to a queue pair

This commit adds uverbs methods and driver callbacks to attach a user
memory region to a single RU queue pair and detach it again.

> diff --git a/drivers/infiniband/core/uverbs_std_types_qp.c b/drivers/infiniband/core/uverbs_std_types_qp.c
> index a1112083fb73f..88a6050d1d046 100644
> --- a/drivers/infiniband/core/uverbs_std_types_qp.c
> +++ b/drivers/infiniband/core/uverbs_std_types_qp.c
> @@ -683,12 +683,126 @@ DECLARE_UVERBS_NAMED_METHOD(
>  			     enum ib_uverbs_qp_attach_comp_cntr_op,
>  			     UA_MANDATORY));
>  
> +/* bind a region to the QP that will use it */
> +static int UVERBS_HANDLER(UVERBS_METHOD_QP_ATTACH_MR)(
> +	struct uverbs_attr_bundle *attrs)
> +{
[ ... ]
> +	/* the QP holds the region until detach or its own destroy */
> +	mr->attach_id = id;
> +	atomic_inc(&mr->usecnt);
> +	/* the MR's read lock drops with a relaxed atomic, order the hold
> +	 * before it so a dereg that gets in next sees the region in use
> +	 */
> +	smp_mb__after_atomic();
> +
> +	return 0;
> +
> +err_xa:
> +	xa_erase(&qp->attached_mrs, id);
> +err_claim:
> +	WRITE_ONCE(mr->attached_qp, NULL);
> +	return ret;
> +}
> +
> +static int UVERBS_HANDLER(UVERBS_METHOD_QP_DETACH_MR)(
> +	struct uverbs_attr_bundle *attrs)
> +{
> +	struct ib_qp *qp =
> +		uverbs_attr_get_obj(attrs, UVERBS_ATTR_QP_DETACH_MR_QP_HANDLE);
> +	struct ib_mr *mr =
> +		uverbs_attr_get_obj(attrs, UVERBS_ATTR_QP_DETACH_MR_MR_HANDLE);
> +	int ret;
> +
> +	if (!qp->device->ops.qp_detach_mr)
> +		return -EOPNOTSUPP;
> +
> +	if (qp->real_qp != qp)
> +		return -EINVAL;
> +
> +	if (READ_ONCE(mr->attached_qp) != qp)
> +		return -EINVAL;
> +
> +	ret = qp->device->ops.qp_detach_mr(qp, mr);
> +	if (ret)
> +		return ret;
> +
> +	xa_erase(&qp->attached_mrs, mr->attach_id);
> +	WRITE_ONCE(mr->attached_qp, NULL);
> +	/* once usecnt reads zero the MR can be freed under us */
> +	smp_mb__before_atomic();
> +	atomic_dec(&mr->usecnt);
> +
> +	return 0;
> +}

[Severity: Medium]
Can uverbs_free_mr() observe a stale non-zero mr->usecnt after
UVERBS_METHOD_QP_DETACH_MR or ib_destroy_qp_user() detaches the region and
spuriously fail with -EBUSY?

When UVERBS_METHOD_QP_DETACH_MR finishes, rdma_lookup_put_uobject() drops
UVERBS_LOOKUP_READ on the MR with a relaxed atomic_dec(&uobj->usecnt).
Because UVERBS_METHOD_QP_DETACH_MR places smp_mb__before_atomic() before
atomic_dec(&mr->usecnt) instead of smp_mb__after_atomic() after it, the
two relaxed atomic_dec() calls can be reordered on weakly-ordered
architectures:

CPU0                                  CPU1
UVERBS_METHOD_QP_DETACH_MR
  WRITE_ONCE(mr->attached_qp, NULL)
  smp_mb__before_atomic()
  [atomic_dec(&mr->usecnt) delayed]
  rdma_lookup_put_uobject(mr_uobj)
    atomic_dec(&uobj->usecnt)
                                      uverbs_free_mr()
                                        rdma_lookup_get_uobject()
                                          atomic_cmpxchg(&uobj->usecnt, 0, -1)
                                        atomic_read(&mr->usecnt) -> 1
                                        return -EBUSY;  <-- spurious -EBUSY
  atomic_dec(&mr->usecnt)

In addition, both UVERBS_METHOD_QP_DETACH_MR and ib_destroy_qp_user()
clear mr->attached_qp before decrementing mr->usecnt.  Once
mr->attached_qp is NULL, another thread can attach the MR to a second QP
(which only takes UVERBS_ACCESS_READ on the MR), detach it, and call
uverbs_free_mr() before the first thread runs atomic_dec(&mr->usecnt),
again seeing mr->usecnt == 1 and failing with -EBUSY.

Decrementing mr->usecnt with atomic_dec_return_release() before
smp_store_release(&mr->attached_qp, NULL) in both detach paths, paired
with smp_mb__after_atomic() in UVERBS_METHOD_QP_DETACH_MR, would close
both windows.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109-uet-ib-core-v3-0-3e7e7ffed2e9@broadcom.com?part=13