Re: [PATCH rdma-next v3 11/13] RDMA/core: extended memory registration

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] RDMA/core: incomplete MR field initialization in
  ib_mr_init_core_refs()
--

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

RDMA/core: extended memory registration

This commit adds the reg_user_mr_ex device operation and extended UVERBS_METHOD_REG_MR ioctl attributes to support registering memory regions bound to a job, derived from a parent MR, or using caller-chosen 64-bit keys.

> diff --git a/drivers/infiniband/core/uverbs_std_types_mr.c b/drivers/infiniband/core/uverbs_std_types_mr.c
> index 0c72f801e0d32..8d281d13d2105 100644
> --- a/drivers/infiniband/core/uverbs_std_types_mr.c
> +++ b/drivers/infiniband/core/uverbs_std_types_mr.c
[ ... ]
> @@ -373,12 +521,26 @@ static int UVERBS_HANDLER(UVERBS_METHOD_REG_MR)(
>  	mr->device = pd->device;
>  	mr->pd = pd;
>  	mr->type = IB_MR_TYPE_USER;
> +	mr->iova = iova;
> +	mr->length = length;
> +	ib_mr_init_core_refs(mr);
>  	mr->uobject = uobj;
>  	atomic_inc(&pd->usecnt);
>  	if (dmah) {
>  		mr->dmah = dmah;
>  		atomic_inc(&dmah->usecnt);
>  	}
[ ... ]
> diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c
> index 9359318141b79..a80d750777f43 100644
> --- a/drivers/infiniband/core/verbs.c
> +++ b/drivers/infiniband/core/verbs.c
> @@ -2405,6 +2406,8 @@ int ib_dereg_mr_user(struct ib_mr *mr, struct ib_udata *udata)
>  	struct ib_pd *pd = mr->pd;
>  	struct ib_dm *dm = mr->dm;
>  	struct ib_dmah *dmah = mr->dmah;
> +	struct ib_jkey *jkey = mr->jkey;
> +	struct ib_mr *parent = mr->parent;
>  	struct ib_sig_attrs *sig_attrs = mr->sig_attrs;
>  	int ret;
>  
> @@ -2417,6 +2420,10 @@ int ib_dereg_mr_user(struct ib_mr *mr, struct ib_udata *udata)
>  			atomic_dec(&dm->usecnt);
>  		if (dmah)
>  			atomic_dec(&dmah->usecnt);
> +		if (jkey)
> +			atomic_dec(&jkey->usecnt);
> +		if (parent)
> +			atomic_dec(&parent->usecnt);
>  		kfree(sig_attrs);
>  	}
[ ... ]
> diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
> index 8be766b80ce08..07fd5731d4587 100644
> --- a/include/rdma/ib_verbs.h
> +++ b/include/rdma/ib_verbs.h
[ ... ]
> @@ -4818,6 +4850,20 @@ static inline u32 ib_inc_rkey(u32 rkey)
>  	return ((rkey + 1) & mask) | (rkey & ~mask);
>  }
>  
> +/**
> + * ib_mr_init_core_refs - clears what core reads back when an MR is
> + *   deregistered.
> + * @mr: struct ib_mr pointer to initialize.
> + *
> + * A driver's MR need not be zeroed, so every path that makes an MR calls this.
> + */
> +static inline void ib_mr_init_core_refs(struct ib_mr *mr)
> +{
> +	mr->jkey = NULL;
> +	mr->parent = NULL;
> +	atomic_set(&mr->usecnt, 0);
> +}

[Severity: Medium]
Does ib_mr_init_core_refs() leave mr->dm, mr->dmah, and mr->sig_attrs
uninitialized when a driver returns a non-zeroed struct ib_mr?

The comment states that a driver's MR need not be zeroed and that
ib_mr_init_core_refs() clears what the core reads back on deregistration.
However, ib_dereg_mr_user() also reads mr->dm, mr->dmah, and mr->sig_attrs,
while UVERBS_HANDLER(UVERBS_METHOD_REG_MR),
UVERBS_HANDLER(UVERBS_METHOD_REG_DMABUF_MR), and ib_uverbs_rereg_mr() do not
clear them (and UVERBS_METHOD_REG_MR only sets mr->dmah when dmah is
non-NULL).  If a driver implementing reg_user_mr_ex or reg_user_mr does not
zero-initialize struct ib_mr, deregistering the MR will pass uninitialized
pointers to atomic_dec(&dm->usecnt), atomic_dec(&dmah->usecnt), and
kfree(sig_attrs), corrupting memory or crashing.

If ib_mr_init_core_refs() is updated to also clear mr->dm, mr->dmah, and
mr->sig_attrs, note that UVERBS_HANDLER(UVERBS_METHOD_DM_MR_REG) and
ib_alloc_mr_integrity() must call it before assigning mr->dm and
mr->sig_attrs.

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